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

Jump to bottom

Skip test if NDEBUG is set #770

Merged
merged 2 commits into from Jul 31, 2017
Merged

Skip test if NDEBUG is set #770

merged 2 commits into from Jul 31, 2017

Conversation

jougs
Copy link
Contributor

@jougs jougs commented Jun 29, 2017

This fixes #272.

@jougs jougs requested review from heplesser and gtrensch June 29, 2017 20:47
@jougs jougs added ZC: Infrastructure DO NOT USE THIS LABEL I: Behavior changes Introduces changes that produce different results for some users ZP: Pending DO NOT USE THIS LABEL S: Normal Handle this with default priority T: Bug Wrong statements in the code or documentation labels Jun 29, 2017
@jougs jougs added this to the NEST 2.12.1 milestone Jun 29, 2017
heplesser
heplesser requested changes Jun 29, 2017
View reviewed changes
Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

Neat solution, but I think you changed more than needed, see inline comment.

testsuite/do_tests.sh.in Outdated
@@ -493,7 +493,8 @@ CODES_FAILURE=\

run_test selftests/test_fail_or_die.sli "${CODES_SUCCESS}" "${CODES_SKIPPED}" "${CODES_FAILURE}"

CODES_SUCCESS=' 3 Success'
CODES_SUCCESS=' 3 Success,'\
' 200 Skipped,'
Copy link
Contributor

Choose a reason for hiding this comment

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

Skipping is not success. I think CODES_SUCCESS should remain unchanged. The 200 exit code from the script will then be registered and reported as a skipped test. CODES_SKIPPED is defined for the entire file around line 435.

@jougs
Copy link
Contributor Author

jougs commented Jun 29, 2017

Ah, OK. Then I misunderstood how skipping works. I've reverted my changes to testsuite/do_tests.sh.in in b3eeb7c.

@gtrensch
Copy link
Contributor

Tested and approved !

@heplesser heplesser merged commit 271d59f into nest:master Jul 31, 2017
@jougs jougs deleted the fix-272 branch August 31, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heplesser heplesser heplesser approved these changes

@gtrensch gtrensch gtrensch approved these changes

Assignees
No one assigned
Labels
I: Behavior changes Introduces changes that produce different results for some users S: Normal Handle this with default priority T: Bug Wrong statements in the code or documentation ZC: Infrastructure DO NOT USE THIS LABEL ZP: Pending DO NOT USE THIS LABEL
Projects
None yet
Milestone
NEST 2.14.0
Development

Successfully merging this pull request may close these issues.

make installcheck fails when building with -DNDEBUG
3 participants
@jougs @gtrensch @heplesser