-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add %p to LLVM_PROFILE_FILE pattern when running tests with coverage #8894
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
base: main
Are you sure you want to change the base?
Add %p to LLVM_PROFILE_FILE pattern when running tests with coverage #8894
Conversation
@swift-ci please test |
Thanks Si! Can you add some simple tests here to make sure the attributes in the path resolve correctly? |
@dschaefer2 sure thing. I couldn't see any tests that check for the coverage output, but I maybe missed them? Could you point me at them if they exist or, if they don't, suggest the best place to add the tests (don't super know my way around in SwiftPM). The PR description shows the impact of running with %p with an out of tree workaround. |
@dschaefer2 I added a test in 5290b2b. I couldn't see anywhere where there were already tests for how the coverage files got exported, merged, or validated, so I decided to put them in integration tests. |
Nice. Thanks! |
@swift-ci please test |
@swift-ci please test windows |
@dschaefer2 I notice the Linux jobs are still running with Swift 6.1, is that expected?
— https://ci.swift.org/job/swift-package-manager-self-hosted-Linux-smoke-test/6515/consoleText I will update the test to either skip on pre-6.2 (because it uses exit tests) or to try and validate some other way. But just checking that's what we expect. I have validated locally that they do pass with the 6.2 nightly Docker image. |
@swift-ci please test |
Motivation
The current setting for
LLVM_PROFILE_PATH
, used for code coverage, leads to corrupt profile data when tests are run in parallel or when writing "exit tests" with Swift Testing. This also results in theswift test --enable-code-coverage
command to fail.The
LLVM_PROFILE_PATH
environment variable is used by the runtime to write raw profile files, which are then processed when the test command finishes to produce the coverage results as JSON. The variable supports several pattern variables1, including%Nm
, which is currently set, and is documented to create a pool of files that the runtime will handle synchronisation of. This is fine for parallelism within the process but will not work across different processes. SwiftPM uses multiple invocations of the same binary for parallel testing and users may also fork processes within their tests, which is now a required workflow when using exit tests with Swift Testing, which will fork the process internally. Furthermore, the current setting for this variable uses only%m
(which impliesN=1
), which makes it even more likely that processes will stomp over each other when writing the raw profile data.We can see a discussion of this happening in practice in #8893.
The variable also supports
%p
1, which will expand to produce a per-process path for the raw profile, which is probably what we want here, since Swift PM is combining all the profiles in the configured directory.Modifications
Add %p to LLVM_PROFILE_FILE pattern when running tests with coverage.
Result
.serialized
#8893.Appendix: Demonstrating the merging of per-process profiles
Running with just one test results in one per-process profile and 50% coverage, as expected.
Running the other test also results in one per-process profile and 50% coverage, as expected.
Running both tests results in two per-process profile and 100% coverage, after merge.
Footnotes
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#running-the-instrumented-program ↩ ↩2