Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: disable failing tests #5808

Merged
merged 1 commit into from
Jul 20, 2023
Merged

test: disable failing tests #5808

merged 1 commit into from
Jul 20, 2023

Conversation

osalyk
Copy link
Contributor

@osalyk osalyk commented Jul 19, 2023

This change is Reviewable

@osalyk osalyk added the sprint goal This pull request is part of the ongoing sprint label Jul 19, 2023
@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #5808 (bcbab9a) into master (1527cfc) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5808      +/-   ##
==========================================
- Coverage   70.97%   70.93%   -0.05%     
==========================================
  Files         131      131              
  Lines       19175    19175              
  Branches     3193     3192       -1     
==========================================
- Hits        13610    13602       -8     
- Misses       5565     5573       +8     

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)


src/test/pmem2_vm_reservation/TESTS.py line 290 at r1 (raw file):

# XXX - https://github.com/pmem/pmdk/issues/5705

#5707

Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @osalyk)

a discussion (no related file):
@janekmi :
Shall we have a separate src/test/obj_sync test w/o Valgrind, as we have lost almost all of them

At least one for m, one for c one for t, and one for r



src/test/obj_sync/TEST0 line 17 at r1 (raw file):

# XXX disable the test for 'pmemcheck'
# until https://github.com/pmem/pmdk/issues/5643 is fixed.
configure_valgrind pmemcheck force-disable

The previous solution disables this test only for pmemcheck, as it was claimed in #5643
I think it should stay as it was.

The alternative solution could be to split this test into two, one w/o pmemcheck and the second one.w/ pmemcheck and disable the second one using DISABLED.

Code quote:

# XXX disable the test for 'pmemcheck'
# until https://github.com/pmem/pmdk/issues/5643 is fixed.
configure_valgrind pmemcheck force-disable

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72 and @osalyk)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

@janekmi :
Shall we have a separate src/test/obj_sync test w/o Valgrind, as we have lost almost all of them

At least one for m, one for c one for t, and one for r

It is a nice to have. No big changes to libpmemobj are planned so we can survive without them.
At the same time, I would not like to keep them after these tests are re-enabled.

I think the easiest approach is to use the old disabling mechanism since it is more fine-grained.


Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @janekmi and @osalyk)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

It is a nice to have. No big changes to libpmemobj are planned so we can survive without them.
At the same time, I would not like to keep them after these tests are re-enabled.

I think the easiest approach is to use the old disabling mechanism since it is more fine-grained.

The problem I see is that all tests sometimes fail w/ Valgrind, and we do not have a version w/o it.


Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @grom72 and @osalyk)

a discussion (no related file):

Previously, grom72 (Tomasz Gromadzki) wrote…

The problem I see is that all tests sometimes fail w/ Valgrind, and we do not have a version w/o it.

Shall we have a force-disable build then?


Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @janekmi and @osalyk)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

Shall we have a force-disable build then?

Good point.


Copy link
Contributor

@grom72 grom72 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)

Copy link
Contributor Author

@osalyk osalyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 8 of 9 files reviewed, 2 unresolved discussions (waiting on @grom72 and @janekmi)


src/test/pmem2_vm_reservation/TESTS.py line 290 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

#5707

Done.

Copy link
Contributor

@janekmi janekmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)

@janekmi janekmi merged commit 8ff3689 into pmem:master Jul 20, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sprint goal This pull request is part of the ongoing sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants