Skip to content
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

Simplify coverage tests #83755

Merged
merged 1 commit into from
Apr 2, 2021
Merged

Simplify coverage tests #83755

merged 1 commit into from
Apr 2, 2021

Conversation

richkadel
Copy link
Contributor

This change reduces the risk of impacting coverage tests on unrelated
changes (such as MIR and Span changes), and reduces the burden when
blessing coverage changes in case it is necessary.

  • Remove all spanview tests. The spanview tests were useful during
    development, but they can be generated as needed, via compiler command
    line flags. They aren't critical to confirming coverage results. (The
    coverage report tests are sufficient.)

    When spanview regeneration was necessary, the diffs were way too hard
    to read to be useful anyway. So I'm removing them to reduce friction
    from a feature that is no longer useful.

  • Remove the requirement for llvm-cov show --debug when blessing
    tests. The --debug flag is, unfortunately, only available if LLVM is
    built with optimize = false (in Rust's config.toml). This adds
    significant time and resource burdens to the contributor's build. As
    it turns out, for other reasons in the past, I wasn't actually using
    the debug output (counter info) to validate coverage anymore either,
    so it was required for no reason, I now realize.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 1, 2021
@richkadel
Copy link
Contributor Author

r? tmandry

@richkadel
Copy link
Contributor Author

This addresses the problems discussed in #83663 (comment)

@jyn514
Copy link
Member

jyn514 commented Apr 1, 2021

r? @tmandry

@richkadel
Copy link
Contributor Author

oops! Thanks Josh.

@tmandry
Copy link
Member

tmandry commented Apr 2, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Apr 2, 2021

📌 Commit 9c0eda76a1edb871f652f34fd0bb898ed2c02e12 has been approved by tmandry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 2, 2021
@bors
Copy link
Contributor

bors commented Apr 2, 2021

☔ The latest upstream changes (presumably #83663) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 2, 2021
@petrochenkov
Copy link
Contributor

Why are the tests run-make-fulldeps rather than run-make though?

Fulldeps tests are very special and should only be used for legacy plugins and other uses of the compiler as a library, any regular compiler use doesn't need it. Unnecessary use of fulldeps is a huge waste of time due to the compiler being built twice.

@richkadel
Copy link
Contributor Author

Why are the tests run-make-fulldeps rather than run-make though?

I'm surprised no one has ever said anything about this in the past year, but thanks for suggesting it. I'll try moving them and if there are no side effects, I'll include the move in this cleanup PR.

Thanks!

This change reduces the risk of impacting coverage tests on unrelated
changes (such as MIR and Span changes), and reduces the burden when
blessing coverage changes in case it is necessary.

* Remove all spanview tests. The spanview tests were useful during
  development, but they can be generated as needed, via compiler command
  line flags. They aren't critical to confirming coverage results. (The
  coverage report tests are sufficient.)

  When spanview regeneration was necessary, the diffs were way too hard
  to read to be useful anyway. So I'm removing them to reduce friction
  from a feature that is no longer useful.

* Remove the requirement for `llvm-cov show --debug` when blessing
  tests. The `--debug` flag is, unfortunately, only available if LLVM is
  built with `optimize = false` (in Rust's config.toml). This adds
  significant time and resource burdens to the contributor's build. As
  it turns out, for other reasons in the past, I wasn't actually using
  the debug output (counter info) to validate coverage anymore either,
  so it was required for no reason, I now realize.
@richkadel
Copy link
Contributor Author

if there are no side effects, I'll include the move in this cleanup PR.

For some reason, the llvm-cov tool is failing. I'm going to look into this some other time, but will leave it out of this PR for now.

@richkadel
Copy link
Contributor Author

@tmandry (or anyone) I've resolved the conflicts. Can someone please resubmit this to bors for me?

Thanks!

@jyn514
Copy link
Member

jyn514 commented Apr 2, 2021

@bors r=tmandry

@bors
Copy link
Contributor

bors commented Apr 2, 2021

📌 Commit fad5388 has been approved by tmandry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 2, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 2, 2021
Simplify coverage tests

This change reduces the risk of impacting coverage tests on unrelated
changes (such as MIR and Span changes), and reduces the burden when
blessing coverage changes in case it is necessary.

* Remove all spanview tests. The spanview tests were useful during
  development, but they can be generated as needed, via compiler command
  line flags. They aren't critical to confirming coverage results. (The
  coverage report tests are sufficient.)

  When spanview regeneration was necessary, the diffs were way too hard
  to read to be useful anyway. So I'm removing them to reduce friction
  from a feature that is no longer useful.

* Remove the requirement for `llvm-cov show --debug` when blessing
  tests. The `--debug` flag is, unfortunately, only available if LLVM is
  built with `optimize = false` (in Rust's config.toml). This adds
  significant time and resource burdens to the contributor's build. As
  it turns out, for other reasons in the past, I wasn't actually using
  the debug output (counter info) to validate coverage anymore either,
  so it was required for no reason, I now realize.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 2, 2021
Rollup of 7 pull requests

Successful merges:

 - rust-lang#83065 (Rework `std::sys::windows::alloc`)
 - rust-lang#83478 (rustdoc: Add unstable option to only emit shared/crate-specific files)
 - rust-lang#83629 (Fix double-drop in `Vec::from_iter(vec.into_iter())` specialization when items drop during panic)
 - rust-lang#83673 (give full path of constraint in suggest_constraining_type_param)
 - rust-lang#83755 (Simplify coverage tests)
 - rust-lang#83757 (2229: Support migration via rustfix)
 - rust-lang#83771 (Fix stack overflow detection on FreeBSD 11.1+)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7009117 into rust-lang:master Apr 2, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 2, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 2, 2021
Move rustdoc run-make-fulldeps tests to run-make

This cuts the time to run the tests in half, because they don't require
building a stage 2 compiler. They were all added to fulldeps before rust-lang#82802 because rustdoc wasn't available in run-make tests.

This doesn't change coverage tests, which will be changed soon in a
separate PR (rust-lang#83755 (comment)).

This also changes some of the `-include` directives, see rust-lang#83773 for what's going on there.

r? `@petrochenkov`
@jyn514
Copy link
Member

jyn514 commented Apr 16, 2021

For some reason, the llvm-cov tool is failing. I'm going to look into this some other time, but will leave it out of this PR for now.

@richkadel Did you remember to build the demangler for run-make, not just run-make-fulldeps?

rust/src/bootstrap/test.rs

Lines 1180 to 1182 in e87c4dd

if mode == "run-make" && suite.ends_with("fulldeps") {
let rust_demangler = builder
.ensure(tool::RustDemangler { compiler, target, extra_features: Vec::new() })

Not sure if llvm-cov is related to the demangler, but that stood out as needing changes.

@richkadel
Copy link
Contributor Author

Did you remember to build the demangler for run-make, not just run-make-fulldeps?

Thanks @jyn514 !

Right, I also noticed that when I was updating bootstrap a couple of weeks ago. I'm sure that is a problem. I'm not sure if it's the only reason it failed, but possibly. (The error I saw didn't seem related to rust-demangler, but who knows how that might have manifested.)

I did create a new github Issue for this, and mentioned the bootstrap change there: #83830

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants