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 ex_libpmem2/TEST5 with memcheck #5667

Merged
merged 1 commit into from
May 22, 2023

Conversation

grom72
Copy link
Contributor

@grom72 grom72 commented May 22, 2023

Disable ex_libpmem2/TEST5 with memcheck until issue #5635 is fixed.


This change is Reviewable

@grom72 grom72 added this to the 1.13 on GHA milestone May 22, 2023
Copy link
Contributor

@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.

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


src/test/ex_libpmem2/TESTS.py line 101 at r1 (raw file):

# additionall test TEST501 has been added to cover non-pmemcheck configs.
@t.require_valgrind_disabled('memcheck', )  # to be removed when fixed
@t.require_valgrind_disabled('pmemcheck')

It shouldn't be: @t.require_valgrind_disabled('memcheck', 'pmemcheck')?

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 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72 and @t)


src/test/ex_libpmem2/TESTS.py line 78 at r1 (raw file):

# XXX Disable the test execution under pmemcheck with g.PAGE and
# g.CACHELINE until the issue

CACHELINE is required for TEST5. I think the previous condition in the comment was correct.


src/test/ex_libpmem2/TESTS.py line 101 at r1 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

It shouldn't be: @t.require_valgrind_disabled('memcheck', 'pmemcheck')?

👍

In case somebody will find it useful:

Granularity\Valgrind none memcheck pmemcheck drd/helgrind
page TEST501 TEST501 -> disabled disabled TEST501
cache TEST501 TEST501 -> disabled TEST5 TEST501
byte TEST501 TEST501 -> disabled TEST5 TEST501

It was too hard for me to validate it in my head. I had to draw a table.

Disable ex_libpmem2/TEST5 with memcheck until issue pmem#5635 is fixed.

Signed-off-by: Tomasz Gromadzki <tomasz.gromadzki@intel.com>
Copy link
Contributor Author

@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 1 of 1 files at r1.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @janekmi and @t)


src/test/ex_libpmem2/TESTS.py line 78 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

CACHELINE is required for TEST5. I think the previous condition in the comment was correct.

Done.


src/test/ex_libpmem2/TESTS.py line 101 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

👍

In case somebody will find it useful:

Granularity\Valgrind none memcheck pmemcheck drd/helgrind
page TEST501 TEST501 -> disabled disabled TEST501
cache TEST501 TEST501 -> disabled TEST5 TEST501
byte TEST501 TEST501 -> disabled TEST5 TEST501

It was too hard for me to validate it in my head. I had to draw a table.

Done.

Copy link
Contributor Author

@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 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @janekmi and @t)

Copy link
Contributor

@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.

:lgtm:

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi and @t)

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Merging #5667 (377f875) into stable-1.13 (83975ef) will increase coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           stable-1.13    #5667   +/-   ##
============================================
  Coverage        74.27%   74.28%           
============================================
  Files              145      145           
  Lines            22131    22131           
  Branches          3704     3705    +1     
============================================
+ Hits             16437    16439    +2     
+ Misses            5694     5692    -2     

Copy link
Contributor Author

@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 all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @grom72)

@janekmi janekmi merged commit cc241f0 into pmem:stable-1.13 May 22, 2023
@grom72 grom72 deleted the test-fix-5635 branch May 22, 2023 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants