Fix and re-enable two coverage tests on MacOS #78888
Conversation
|
r? tmandry |
|
@tmandry - I don't know why rust-highfive is not assigning you (or anyone) as reviewer, but can you take a look? Thanks! |
| # The `llvm-cov show` flag `--debug`, used to generate the `counters` output files, is only enabled | ||
| # if LLVM assertions are enabled. Some CI builds disable debug assertions. | ||
| ifndef NO_LLVM_ASSERTIONS | ||
| # if LLVM assertions are enabled. Requires Rust config `llvm/optimize` and not | ||
| # `llvm/release_debuginfo`. Note that some CI builds disable debug assertions (by setting | ||
| # `NO_LLVM_ASSERTIONS=1`), so it is not OK to fail the test, but `bless`ed test results cannot be | ||
| # generated without debug assertions. |
There was a problem hiding this comment.
I guess we can consider this like tests which only run on certain platforms, except it only runs in certain configurations. It's not ideal, because if someone makes a large change that affects these tests and isn't in the right configuration, they won't be able to update the tests.
That being said, I'm not sure what we can do differently. I wish there was an LLVM build option to enable only this flag.
One mitigation is to add a warning if someone tries to bless and we can't update all the files. (To keep it from being too noisy.. Maybe only print it if they specify bless and one of the other files doesn't match? This kind of logic is difficult in a Makefile which is another reason I think we should move away from those.)
|
@bors delegate+ |
|
✌️ @richkadel can now approve this pull request |
|
Actually, this change is an improvement overall, I just wanted you to see my comment :) @bors r+ |
|
📌 Commit 1e354de0c24c1bb54bf806147d9a42aaec8bda5b has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened |
bors
added
the
S-waiting-on-bors
|
⌛ Testing commit 1e354de0c24c1bb54bf806147d9a42aaec8bda5b with merge da3ad56562c56b537887bd8adbb199cd7a0e2bae... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors r- https://github.com/rust-lang-ci/rust/runs/1388536522 tests seems still failing. |
bors
added
S-waiting-on-author
Note, in the coverage-reports test, the comment about MacOS was wrong. The setting is based on config.toml llvm `optimize` setting. There doesn't appear to be any environment variable I can check, and I don't think we should add one. Testing the binary itself is a more reliable way to check anyway. For the coverage-spanview test, I removed the dependency on sed altogether, which is much less ugly than trying to work around the MacOS sed differences. I tested these changes on Linux, Windows, and Mac.
richkadel
force-pushed
the
llvm-coverage-tests
branch
from
1e354de to
fe56d26
Compare
|
Looks like I forgot to prefix @bors r=tmandry |
|
📌 Commit fe56d26 has been approved by |
bors
added
S-waiting-on-bors
|
forgot fmt @bors r=tmandry rollup=always |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit fe56d26 has been approved by |
|
@bors r=tmandry rollup- |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit fe56d26 has been approved by |
…andry Fix and re-enable two coverage tests on MacOS Note, in the coverage-reports test, the comment about MacOS was wrong. The setting is based on config.toml llvm `optimize` setting. There doesn't appear to be any environment variable I can check, and I don't think we should add one. Testing the binary itself is a more reliable way to check anyway. For the coverage-spanview test, I removed the dependency on sed altogether, which is much less ugly than trying to work around the MacOS sed differences. I tested these changes on Linux, Windows, and Mac. r? `@tmandry` FYI `@wesleywiser`
…andry Fix and re-enable two coverage tests on MacOS Note, in the coverage-reports test, the comment about MacOS was wrong. The setting is based on config.toml llvm `optimize` setting. There doesn't appear to be any environment variable I can check, and I don't think we should add one. Testing the binary itself is a more reliable way to check anyway. For the coverage-spanview test, I removed the dependency on sed altogether, which is much less ugly than trying to work around the MacOS sed differences. I tested these changes on Linux, Windows, and Mac. r? ``@tmandry`` FYI ``@wesleywiser``
|
☀️ Test successful - checks-actions |
Note, in the coverage-reports test, the comment about MacOS was wrong.
The setting is based on config.toml llvm
optimizesetting. Theredoesn't appear to be any environment variable I can check, and I
don't think we should add one. Testing the binary itself is a more
reliable way to check anyway.
For the coverage-spanview test, I removed the dependency on sed
altogether, which is much less ugly than trying to work around the
MacOS sed differences.
I tested these changes on Linux, Windows, and Mac.
r? @tmandry
FYI @wesleywiser