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: fix CHECK_TYPE behaviour #5868

Merged
merged 1 commit into from
Aug 22, 2023
Merged

test: fix CHECK_TYPE behaviour #5868

merged 1 commit into from
Aug 22, 2023

Conversation

janekmi
Copy link
Contributor

@janekmi janekmi commented Aug 21, 2023

  • By default, no Valgrind tool has been forced, and a Valgrind tool or lack of it will be picked depending on the particular test's requirements.
  • Providing 'force-enable [memcheck|pmemcheck|helgrind|drd]' will force all tests to be run under the forced Valgrind tool unless a test forced a different Valgrind tool in which case the given will test will be skipped.
  • Providing 'force-enable none' will run all tests that do not require explicitly any Valgrind tool and skip all other tests.

Ref: #5811
Ref: #5791

No force-enable provided - regression and fix

PMem Long (no force-enable provided)

Before regression: https://github.com/pmem/pmdk/actions/runs/5838427329/job/15835451743
After regression: https://github.com/pmem/pmdk/actions/runs/5908491536/job/16027995086
After fix: https://github.com/pmem/pmdk/actions/runs/5927643088/job/16071573582

Explicit force-enable is provided - no change

PMem Valgrind (force-enable: none)

Before fix: https://github.com/pmem/pmdk/actions/runs/5925859348/job/16066125162#step:6:125
After fix: https://github.com/pmem/pmdk/actions/runs/5927719561/job/16071794659#step:6:125

PMem Valgrind (force-enable: pmemcheck)

Before fix: https://github.com/pmem/pmdk/actions/runs/5914014610/job/16038896782
After fix: https://github.com/pmem/pmdk/actions/runs/5927719561/job/16071794960


This change is Reviewable

@janekmi janekmi added sprint goal This pull request is part of the ongoing sprint no changelog Add to skip the changelog check on your pull request labels Aug 21, 2023
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #5868 (0950541) into master (24ad25f) will increase coverage by 2.83%.
The diff coverage is n/a.

❗ Current head 0950541 differs from pull request most recent head 872a462. Consider uploading reports for the commit 872a462 to get more accurate results

@@            Coverage Diff             @@
##           master    #5868      +/-   ##
==========================================
+ Coverage   68.15%   70.99%   +2.83%     
==========================================
  Files         131      131              
  Lines       19682    19176     -506     
  Branches     3259     3193      -66     
==========================================
+ Hits        13415    13614     +199     
+ Misses       6267     5562     -705     

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


src/test/unittest/unittest.sh line 51 at r1 (raw file):

# Required for easy force-enable condition failure detection.
# Please see require_valgrind() for details.
CHECK_TYPE_COPY=$CHECK_TYPE

It is not just a 'copy'. It is expected Valgrind test to be used.

Suggestion:

SPECIFIED_CHECK_TYPE

@janekmi janekmi force-pushed the test-CHECK_TYPE-fix branch 2 times, most recently from 6505710 to 872a462 Compare August 22, 2023 08:04
Copy link
Contributor Author

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


src/test/unittest/unittest.sh line 51 at r1 (raw file):

Previously, grom72 (Tomasz Gromadzki) wrote…

It is not just a 'copy'. It is expected Valgrind test to be used.

Done.

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:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @janekmi)

@janekmi janekmi merged commit 1f5b9e1 into master Aug 22, 2023
6 checks passed
- By default, no Valgrind tool has been forced, and a Valgrind tool or
lack of it will be picked depending on the particular test's
requirements.
- Providing 'force-enable [memcheck|pmemcheck|helgrind|drd]' will force
all tests to be run under the forced Valgrind tool unless a test forced
a different Valgrind tool in which case the given will test will be skipped.
- Providing 'force-enable none' will run all tests that do not require
explicitly any Valgrind tool and skip all other tests.

Ref: #5811
Ref: #5791

Signed-off-by: Jan Michalski <jan.michalski@intel.com>
@janekmi janekmi deleted the test-CHECK_TYPE-fix branch August 23, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Add to skip the changelog check on your pull request sprint goal This pull request is part of the ongoing sprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants