ci: run test_bitcoin with DEBUG_LOG_OUT in RUN_UNIT_TESTS_SEQUENTIAL #28736
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
maflcko
reviewed
Oct 26, 2023
ci/test/06_script_b.sh
Outdated
Comment on lines
170
to
192
Member
There was a problem hiding this comment.
Not sure about 20 lines of bash code that are impossible to read, likely no one will review, and which are hard to maintain or change in the future.
On a second though, I wonder how often this feature will be needed, or if there is a simpler hack that achieves something similar.
Contributor
Author
There was a problem hiding this comment.
Some of this was already there before. The new code is:
# find all lines that match 'error: in test_suite/test_case' and extract the test suite and case
for t in \$(sed -E -n 's/.*error: in \"(.+\\/.+)\":.*/\\1/p' < ${LOG}) ; do
test_suite=\${t%/*} # e.g. net_tests
test_case=\${t#*/} # e.g. v2transport_test
ed -s ${LOG} <<EDCOMMANDS
/Entering test suite \"\${test_suite}\"/
/Entering test case \"\${test_case}\"/,/Leaving test case \"\${test_case}\"/p
q
EDCOMMANDSI do not think that is too different in terms of complexity from what we already have:
%.cpp.test: %.cpp
@echo Running tests: $$(\
cat $< | \
grep -E "(BOOST_FIXTURE_TEST_SUITE\\(|BOOST_AUTO_TEST_SUITE\\()" | \
cut -d '(' -f 2 | cut -d ',' -f 1 | cut -d ')' -f 1\
) from $<
$(AM_V_at)export TEST_LOGFILE=$(abs_builddir)/$$(\
echo $< | grep -E -o "(wallet/test/.*\.cpp|test/.*\.cpp)" | $(SED) -e s/\.cpp/.log/ \
) && \
$(TEST_BINARY) --catch_system_errors=no -l test_suite -t "$$(\
cat $< | \
grep -E "(BOOST_FIXTURE_TEST_SUITE\\(|BOOST_AUTO_TEST_SUITE\\()" | \
cut -d '(' -f 2 | cut -d ',' -f 1 | cut -d ')' -f 1\
)" -- DEBUG_LOG_OUT > "$$TEST_LOGFILE" 2>&1 || (cat "$$TEST_LOGFILE" && false)If you find ed hard to read then I could change it to another language (python?) but I suspect it is going to be more lines of code. I any case, I don't think that's prohibitively complex task: "find 'Entering test suite', after that print the lines between 'Entering test case' and 'Leaving test case'".
Some extra escaping is needed because this is inside bash -c "here;". What is the point of doing that, given that this is already a bash script? I guess doing just here; should have the same effect.
I wonder how often this feature will be needed
This I do not know, but I know that when a test fails on CI that depends on a particular seed, having that seed is a game changer for fixing the bug.
Another option would be to print the seed unconditionally, even if DEBUG_LOG_OUT is not defined. But having the debug log in case of failure is useful beyond the rng seed.
Member
There was a problem hiding this comment.
Another option would be to print the seed unconditionally
Sure, why not?
To extend my previous feedback, on why this bash code isn't ideal:
- It assumes a specific unit test output format, so if boost is removed or if the format or wording changes, the code will silently stop to work
- Putting the bash code into a string and then executing the string makes it impossible to check with shellcheck. And also manual review is hard, because everything is escaped.
- How does it interact with
set -exin the script? Is the error code properly propagated in all cases? We've had tests in the past that silently passed regardless of the result, so it would be good to not accidentally re-introduce that. Doing output parsing in bash makes it easy to discard the error code, for example when adding a pipe but no pipefail.
I do not think that is too different in terms of complexity from what we already have:
Pretty sure this will go away with cmake, no?
Contributor
Author
There was a problem hiding this comment.
Another option would be to print the seed unconditionally
Sure, why not?
Yeah, this will solve the original issue with the seed. But I think this PR has a wider benefit of seeing the debug log for any type of failure, like in the RUN_UNIT_TESTS case.
To extend my previous feedback, on why this bash code isn't ideal:
Thanks, that makes it easier to understand and address.
- It assumes a specific unit test output format, so if boost is removed or if the format or wording changes, the code will silently stop to work
True, but that will not remain unnoticed (as long as somebody cares to look at the log after a failure) and should be easy to adjust. This can use test_bitcoin --log_format=XML or JUNIT. Even with that the point is still valid - if the format is changed then this would have to be adjusted. I think the benefit of seeing the debug log outweights the need to possibly have to adjust this in the future.
- Putting the bash code into a string and then executing the string makes it impossible to check with shellcheck. And also manual review is hard, because everything is escaped.
I agree, removed the bash -c "..." surrounding. Why is that used all over the place in 06_script_b.sh?
- How does it interact with
set -exin the script? Is the error code properly propagated in all cases? We've had tests in the past that silently passed regardless of the result, so it would be good to not accidentally re-introduce that. Doing output parsing in bash makes it easy to discard the error code, for example when adding a pipe but no pipefail.
It works correctly - if ! command that fails ; then foo ; exit 1; fi the failing command will not trigger the exit due to set -e and will execute all commands inside the if body. I put exit 1 at the end to terminate the script because being inside the if means the command failed.
I do not think that is too different in terms of complexity from what we already have:
Pretty sure this will go away with cmake, no?
Good point. And actually my argument was flawed - already having some "bad" code is not enough justification, alone, to add more of the same.
Member
There was a problem hiding this comment.
I agree, removed the
bash -c "..."surrounding. Why is that used all over the place in06_script_b.sh?
It is a leftover from the travis integration that started to use Docker initially, I think.
Edit: Yes, see 59e9688
I was thinking about getting rid of it. Maybe even completely rewrite the whole script in a sane language, instead of "rewriting" half of it from bash to bash.
donPain
approved these changes
Oct 26, 2023
fd4ba89 to
67f3a76
Compare
Contributor
Author
|
|
67f3a76 to
1bc0914
Compare
Contributor
Author
|
|
1bc0914 to
aa66ab8
Compare
Member
|
CI: |
aa66ab8 to
58f6074
Compare
58f6074 to
73c31fd
Compare
Contributor
Author
|
|
73c31fd to
3cc3dfe
Compare
Contributor
Author
|
I deliberately broke some tests to see how this will report the failures. Looks good: CI log from artificial failures |
3cc3dfe to
e3ade1f
Compare
Member
|
Concept ACK. (Maybe unrelated, but the Win64 CI task is one I've often wished for more debug info from when a unit test fails.) |
maflcko
reviewed
Nov 28, 2023
maflcko
reviewed
Nov 29, 2023
e3ade1f to
2b7888e
Compare
Contributor
Author
|
|
2b7888e to
57e8bc6
Compare
Contributor
Author
|
|
Member
|
The PR didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open PRs. Closing due to lack of interest. |
Member
|
To add some context. I think it would be nice to print the seed. However, the changes here had to be pushed several times, because the bash code was wrong, and the current version passes CI, but it is unclear if the bash code is correct. For example, it seems better to fail CI on a test failure and not print the seed, than to print the seed and not fail the CI on a test failure. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Run the unit tests with
DEBUG_LOG_OUTin the case ofRUN_UNIT_TESTS_SEQUENTIAL.Because the output would be too big (about 80MB), redirect it to a file and only show relevant bits from it in case of errors.
Resolves #28466