Add flowId to TeamCity reporter's service messages - #3188
Open
mmakhalaf wants to merge 1 commit into
Open
Conversation
CTest's catch_discover_tests registers one test per Catch2 test case, each run as its own process. Under `ctest -j N` with `--reporter teamcity`, many such processes write into what TeamCity sees as one combined stream, and interleaved testSuiteStarted/testSuiteFinished pairs from different processes get attributed to the wrong flow, producing garbled test names in TeamCity's UI. TeamCity's flowId service-message attribute exists precisely to disambiguate concurrent flows. Tag every message with the process id, which is unique among any processes whose output could plausibly interleave in one build step.
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
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.
Description
catch_discover_tests()(Catch2's own CMake integration,extras/Catch.cmake)registers one CTest test per Catch2 test case, and each one runs the test
executable as its own separate process, filtered down to that single case.
When CTest runs many of those processes concurrently (
ctest -j N) with--reporter teamcity, every process writes its own##teamcity[testSuiteStarted ...]/testStarted/testFinished/testSuiteFinishedsequence into what TeamCity's build log ultimately sees asone combined stream.
TeamCity's service-message protocol has a
flowIdattribute specifically forthis: it lets TeamCity tell messages coming from different concurrent flows
(e.g. different processes) apart. A message with no
flowIdis treated asbelonging to a single implicit default flow. Without it, if two concurrent
test processes'
testSuiteStarted/testSuiteFinishedmessages interleaveeven slightly - which they will, under real parallel execution - TeamCity
attributes a later suite's
testSuiteStartedas nested inside an earlier,not-yet-closed suite in the same default flow. The result is garbled,
concatenated test names in TeamCity's UI, e.g.:
Pass/fail status is still individually correct for each process - this is a
display/attribution bug, not a correctness bug - but it makes results
unreadable, and breaks anything keyed on a stable test name (mute rules,
flaky-test history, etc). This isn't specific to any one project's CMake
setup; it's a general consequence of running Catch2's TeamCity reporter under
any one-process-per-test-case parallel test runner.
This PR tags all six
##teamcity[...]message sites emitted byTeamCityReporter(testSuiteStarted,testSuiteFinished,testStarted,testFinished,testStdOut/testStdErr, andtestFailed/testIgnored)with
flowId='<pid>', using a small process-id helper guarded by theexisting
CATCH_PLATFORM_WINDOWSmacro (the pattern already used forplatform-specific code elsewhere, e.g.
catch_debugger.cpp).A couple of design choices worth calling out for review:
flowIdas safeto add unconditionally - consumers that don't care about flows ignore it.
This seemed like the simplest, most defensible default rather than gating
it behind a new reporter option, but I'm happy to make it configurable if
that's preferred.
processes whose output could plausibly interleave within the same build
step - it doesn't need to be globally unique across machines or time. A
pid is stable for the lifetime of one process and satisfies that.
Tested: rebuilt with the
all-testspreset (examples, extra tests,benchmarks, CMake config tests) and ran the full suite via
ctest, allpassing. Updated the two TeamCity
ApprovalTestsbaselines to include thenew attribute, and added a normalizer (
teamcityFlowIdParser) totools/scripts/approvalTests.pyso the non-deterministic pid is replacedwith a
{pid}placeholder before diffing, the same way{duration}isalready handled. Manually confirmed with two concurrent processes emitting
to the same terminal that each gets a distinct, stable
flowId.GitHub Issues
Not tied to an existing issue - I searched the tracker for prior reports
(mangled/nested/duplicate TeamCity output,
flowId, parallelctest) anddidn't find one, so I'm submitting the fix directly. Happy to open an issue
first if that's preferred process.