CI: refactoring#4812
Merged
Merged
Conversation
Our CI test system invokes ctest with the name of the given tests it wishes to invoke. ctest (with the `-R` flag) treats this name as a regular expression. Provide anchors in the regular expression to avoid matching additional tests in this search.
This reverts commit a2d73f5. Using clar to propagate the XML settings was a mistake.
Add the clar flags to produce JUnit-style XML output before invocation.
Introduce SKIP_*_TEST variables for Windows builds to match POSIX builds.
154dfac to
44383dc
Compare
pks-t
previously requested changes
Sep 28, 2018
pks-t
left a comment
Member
There was a problem hiding this comment.
Looks good to me, except for the one hardcoded path 👍
| # here as well. But it allows us to wrap our test harness with a leak checker like valgrind. | ||
| run_test() { | ||
| TEST_CMD=$(ctest -N -V -R $1 | sed -n 's/^[0-9]*: Test command: //p') | ||
| TEST_CMD=$(ctest -N -V -R "^${1}$" | sed -n 's/^[0-9]*: Test command: //p') |
Member
There was a problem hiding this comment.
Ohhh, I love this. It has bothered me too many times
| $TestName = $args[0] | ||
|
|
||
| $TestCommand = (ctest -N -V -R "^$TestName$") -join "`n" -replace "(?ms).*\n^[0-9]*: Test command: ","" -replace "\n.*","" | ||
| $TestCommand = "C:\LibGit2\libgit2-2\build\Debug\libgit2_clar.exe -sa" |
Member
There was a problem hiding this comment.
This doesn't look intended? You first set TestCommand to some magic voodoo and then overwrite it with a hard-coded path. I guess the second line was for debugging purposes?
Member
There was a problem hiding this comment.
Oh, I guess you accidentally squashed the fix into the wrong commit. Feel free to fix it up if you want to, I'm happy to have this merged either way.
Similar to the way we parse the ctest output on POSIX systems, do the same on Windows. This allows us to append the `-r` flag to clar after we've identified the command to run.
Don't stop on test failures; run all the tests, even when a test fails.
44383dc to
429c7f1
Compare
Member
Author
|
Thanks, I did fixup the wrong commit. I've updated this. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
When I introduced #4723, @pks-t asked why we were stopping on the first failure instead of continuing to run all the tests and simply report the errors. Now that our CI run is faster, I've changed our CI to continue and simply report an error at the end.
In addition, I've reverted the CMake changes to enable XML output. It was a gross hack, and we can instead append
-rduring the CI test runs. This is still a bit of a hack, but a less gross one. 😉