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

testsuite: fix setup error in system tests #5102

Merged
merged 1 commit into from Apr 17, 2023

Conversation

chu11
Copy link
Member

@chu11 chu11 commented Apr 17, 2023

Problem: In t9000-system.t the FLUX_TEST_INSTALLED_PATH variable is setup if 'debug' is set. However, the check for 'debug' is done before sharness is sourced, so the -d option when running the tests doesn't matter. This lead to the t9000-system.t tests failing when running make check in a docker container.

Solution: Instead of checking for 'debug' to set FLUX_TEST_INSTALLED_PATH, check for FLUX_ENABLE_SYSTEM_TESTS instead.

@chu11 chu11 changed the title testsuit: fix setup error in system tests testsuite: fix setup error in system tests Apr 17, 2023
@chu11
Copy link
Member Author

chu11 commented Apr 17, 2023

re-pushed, noticed a typo in the commit message :-)

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

Great! This should have been how the previous fix was done 🤦.

One commit message nit, there's a double use of "Instead" in the Solution statement :-)

Problem: In t9000-system.t the FLUX_TEST_INSTALLED_PATH variable
is setup if 'debug' is set.  However, the check for 'debug' is
done before sharness is sourced, so the `-d` option when running
the tests doesn't matter.  This lead to the t9000-system.t tests
failing when running make check in a docker container.

Solution: Instead of checking for 'debug' to set FLUX_TEST_INSTALLED_PATH,
check for FLUX_ENABLE_SYSTEM_TESTS.
@chu11
Copy link
Member Author

chu11 commented Apr 17, 2023

@grondo thanks, fixed up that typo in the commit message, rebased, and re-pushed. I'll set MWP.

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #5102 (85e78a9) into master (1e4dbdf) will not change coverage.
The diff coverage is n/a.

❗ Current head 85e78a9 differs from pull request most recent head 947f040. Consider uploading reports for the commit 947f040 to get more accurate results

@@           Coverage Diff           @@
##           master    #5102   +/-   ##
=======================================
  Coverage   83.15%   83.15%           
=======================================
  Files         444      444           
  Lines       76623    76623           
=======================================
  Hits        63715    63715           
  Misses      12908    12908           

see 12 files with indirect coverage changes

@mergify mergify bot merged commit f36664d into flux-framework:master Apr 17, 2023
30 of 31 checks passed
@chu11 chu11 deleted the t9000_workaround branch April 18, 2023 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants