chore: grep for failures in CI#6339
Conversation
mvadari
added
Trivial
There was a problem hiding this comment.
Pull request overview
This PR aims to make CI failures easier to spot by capturing embedded unit test output into a log file and printing a small “failure” summary, alongside a small wording tweak to a parse-error log line in the JTx test environment.
Changes:
- Update a JTx parse error log line from “parse failed” to “failure to parse”.
- Pipe embedded unit test output through
teeintounittest.logwhile preserving the original test exit code. - Add a post-failure workflow step to grep the unit test log for failure indicators.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/test/jtx/impl/Env.cpp |
Adjusts parse failure log phrasing in Env::autofill. |
.github/workflows/reusable-build-test-config.yml |
Captures embedded unittest output to unittest.log and adds a grep-based failure summary step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/app/Vault_test.cpp
Outdated
| tx[sfAssetsMaximum] = "18446744073709551617e5"; // uint64 max + 1 | ||
| env(tx, THISLINE); | ||
| BEAST_EXPECT(false); | ||
| fail("Expected parse_error for mantissa larger than uint64 max"); |
There was a problem hiding this comment.
fail("Expected parse_error...") doesn’t include file/line context in the failure reason (unlike BEAST_EXPECT / BEAST_EXPECTS). Since this is in a negative-test path meant to aid debugging, consider using BEAST_EXPECTS(false, ...) (or the fail(reason, __FILE__, __LINE__) overload) so the output includes the source location when the parse_error is not thrown.
| fail("Expected parse_error for mantissa larger than uint64 max"); | |
| BEAST_EXPECTS(false, "Expected parse_error for mantissa larger than uint64 max"); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6339 +/- ##
=========================================
- Coverage 79.8% 79.8% -0.0%
=========================================
Files 848 848
Lines 67757 67757
Branches 7554 7559 +5
=========================================
- Hits 54073 54063 -10
- Misses 13684 13694 +10 see 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| ./xrpld --unittest --unittest-jobs "${BUILD_NPROC}" 2>&1 | tee unittest.log | ||
| exit ${PIPESTATUS[0]} |
There was a problem hiding this comment.
My understanding was that an exit command will prevent the rest of the commands to run, even if the exit code is 0.
The reason it works here is because there are no non-failure() steps that follow this, but if we ever need to add another step then this might lead to unexpected behavior (until the developer notices the exit statement which, since it's unusual to see them outside of if-statements, is easy to overlook).
Would the following work?
| ./xrpld --unittest --unittest-jobs "${BUILD_NPROC}" 2>&1 | tee unittest.log | |
| exit ${PIPESTATUS[0]} | |
| ./xrpld --unittest --unittest-jobs "${BUILD_NPROC}" 2>&1 | tee unittest.log || exit ${PIPESTATUS[0]} |
There was a problem hiding this comment.
Augment was concerned that PIPESTATUS might be overwritten in this case, so I changed it to something related.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mvadari
added
the
Ready to merge
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
High Level Overview of Change
This PR adjusts the CI tests to make it easier to spot errors without needing to sift through the thousands of lines of output.
Context of Change
Easier to sort through the test output, especially on mobile where there is no ctrl-f
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
N/A
Test Plan
https://github.com/XRPLF/rippled/actions/runs/22004407043/job/63584713622?pr=6339
See "Show test failure summary" section