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

[TRIVIAL] unsuppress test output #1017

Merged
merged 3 commits into from
Mar 11, 2024
Merged

Conversation

Yurlungur
Copy link
Collaborator

@Yurlungur Yurlungur commented Mar 8, 2024

PR Summary

Regression test outputs suppress both stdout and stderr from the driver. When debugging this is annoying. Since things are integrated with ctest and ctestsuppresses output for tests that pass (or fail) by default anyway, it's also redundant. This PR makes output (when you ask ctest to be verbose) significantly more verbose, by including driver stdout/stderr output.

The main benefit of this is we now see, e.g., PARTHENON_THROW, PARTHENON_FAIL, PARTHENON_REQUIRE outputs and any print statements we add by hand when debugging tests. The main disadvantage is that output is way more verbose. It may also slightly change performance but not by much I think.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • Change is breaking (API, behavior, ...)
    • Change is additionally added to CHANGELOG.md in the breaking section
    • PR is marked as breaking
    • Short summary API changes at the top of the PR (plus optionally with an automated update/fix script)
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@Yurlungur Yurlungur self-assigned this Mar 8, 2024
@Yurlungur Yurlungur enabled auto-merge March 8, 2024 20:36
Copy link
Collaborator

@BenWibking BenWibking left a comment

Choose a reason for hiding this comment

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

This would be very useful. LGTM.

@Yurlungur Yurlungur disabled auto-merge March 9, 2024 22:58
@Yurlungur Yurlungur enabled auto-merge March 9, 2024 22:58
Copy link
Collaborator

@pgrete pgrete left a comment

Choose a reason for hiding this comment

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

I love it!
Quite often I had to manually copy and paste the failed command to get to the error message.
Why didn't it occur to me earlier to suggest that kind of change...

Copy link
Collaborator

@lroberts36 lroberts36 left a comment

Choose a reason for hiding this comment

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

LGTM. This is definitely a significant quality of life improvement for debugging. I also have resorted to copying the command from ctest output and running it by hand.

@Yurlungur
Copy link
Collaborator Author

@pgrete and/or someone else who knows the CI: I'm very confused. The CI is complaining that files I didn't change are unformatted, and the relevant lines aren't in this branch. Any ideas what's happening?

@Yurlungur Yurlungur merged commit 314ac5c into develop Mar 11, 2024
50 of 68 checks passed
@Yurlungur Yurlungur deleted the jmm/more-verbose-regression branch March 11, 2024 19:44
@pgrete
Copy link
Collaborator

pgrete commented Mar 12, 2024

@pgrete and/or someone else who knows the CI: I'm very confused. The CI is complaining that files I didn't change are unformatted, and the relevant lines aren't in this branch. Any ideas what's happening?

Unclear. Maybe a cosmic ray particle hit the memory of the CI machine? :D

@Yurlungur
Copy link
Collaborator Author

Fixed now anyway lol

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.

5 participants