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: explicit failure if Valgrind is explicitly required but not installed #5791

Merged
merged 2 commits into from
Jul 19, 2023

Conversation

grom72
Copy link
Contributor

@grom72 grom72 commented Jul 10, 2023

Fail all Valgrind tests when --force-enable option is used for RUNTESTS.[sh|py] and Valgrind is not installed.

Resolves #5784


This change is Reviewable

@grom72 grom72 added this to the 2.0.0 milestone Jul 10, 2023
@grom72 grom72 requested review from janekmi and osalyk July 10, 2023 19:36
@grom72 grom72 added the QA: CI .github/ and utils/ related to automated testing label Jul 10, 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.

:lgtm:

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

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

Merging #5791 (92a229c) into master (a0be753) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 92a229c differs from pull request most recent head 1527cfc. Consider uploading reports for the commit 1527cfc to get more accurate results

@@            Coverage Diff             @@
##           master    #5791      +/-   ##
==========================================
- Coverage   70.99%   70.98%   -0.02%     
==========================================
  Files         131      131              
  Lines       19175    19175              
  Branches     3192     3193       +1     
==========================================
- Hits        13614    13612       -2     
- Misses       5561     5563       +2     

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


src/test/RUNTESTS.sh line 321 at r2 (raw file):

					FORCE_CHECK_TYPE=$checktype \
					CHECK_POOL=$check_pool \
					$special_params"

Suggestion:

				# FORCE_CHECK_TYPE is required for easy force-enable condition failure detection.
				# Please see require_valgrind() for details.
				export RUNTEST_EXTRA="CHECK_TYPE=$checktype \
					FORCE_CHECK_TYPE=$checktype \
					CHECK_POOL=$check_pool \
					$special_params"

src/test/unittest/unittest.sh line 1445 at r2 (raw file):

	restore_exit_on_error
	if [ $ret -ne 0 ]; then
		if [ "$FORCE_CHECK_TYPE" != "none" ]; then

Suggestion:

		# FORCE_CHECK_TYPE is basically a copy of CHECK_TYPE as it is provided by force-enable.
		# This copy allowed us to workaround the overcomplicated legacy Valgrind processing logic
		# and here just check whether any Valgrind tool has been force-enabled or not.
		# Lack of Valgrind when it is force-enabled causes test failure.
		# Lack of Valgrind, when it was just required by the given test, causes a skip.
		if [ "$FORCE_CHECK_TYPE" != "none" ]; then

src/test/unittest/valgrind.py line 196 at r2 (raw file):

        try:
            out = sp.check_output('which valgrind', shell=True,
                                  universal_newlines=True, stderr=sp.STDOUT)

Debug-only or left here intentionally?

Fail all Valgrind tests when --force-enable option is used
for RUNTESTS.[sh|py] and Valgrind is not installed.

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.

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


src/test/RUNTESTS.sh line 321 at r2 (raw file):

					FORCE_CHECK_TYPE=$checktype \
					CHECK_POOL=$check_pool \
					$special_params"

Done.


src/test/unittest/unittest.sh line 1445 at r2 (raw file):

	restore_exit_on_error
	if [ $ret -ne 0 ]; then
		if [ "$FORCE_CHECK_TYPE" != "none" ]; then

Done.


src/test/unittest/valgrind.py line 196 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Debug-only or left here intentionally?

Intentionally, to avoid the which valgrind command's srderr be added to the test log.

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

@janekmi janekmi merged commit 05f2041 into pmem:master Jul 19, 2023
janekmi added a commit that referenced this pull request 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

Signed-off-by: Jan Michalski <jan.michalski@intel.com>
janekmi added a commit that referenced this pull request Aug 22, 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

Signed-off-by: Jan Michalski <jan.michalski@intel.com>
janekmi added a commit that referenced this pull request Aug 22, 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

Signed-off-by: Jan Michalski <jan.michalski@intel.com>
@grom72 grom72 deleted the test-fail-no-valgrind branch October 24, 2023 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA: CI .github/ and utils/ related to automated testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests with --force-enable must not SKIP because of missing Valgrind
3 participants