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

Improve test suite for -Zbuild-std #7350

Merged
merged 5 commits into from
Sep 16, 2019
Merged

Conversation

alexcrichton
Copy link
Member

This commit is aimed directly at rust-lang/wg-cargo-std-aware#33 and in
general making the -Zbuild-std tests more robust. The main change here
is that a new source tree is checked in, tests/testsuite/mock-std,
which mirrors rust-lang/rust's own tree for libstd. This mock tree is as
empty as it can be, ideally duplicating almost nothing but for not
requiring duplication of Cargo metadata about patches and such.

The end result here looks like:

  • All -Zbuild-std tests are now run in parallel
  • All tests run much more quickly since they're compiling tiny crates
    instead of actually compiling libstd/libcore
  • No tests require network access
  • We verify that crates have access to the "custom" libraries
    that we build

Coverage of tests is not currently expanded, but it's hoped that we
could add that shortly afterwards. Coverage has actually gone down
slightly since the custom target test was commented out temporarily and
the full integration test of running -Zbuild-std isn't run on CI any
more.

Closes rust-lang/wg-cargo-std-aware#33

@rust-highfive
Copy link

r? @ehuss

(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 Sep 10, 2019
Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! It's a bit elaborate, but is pleasantly fast.

Will this still work if we switch cargo to create its own sysroot? I assume we'd just remove the line that adds --sysroot.

r=me with the above addressed, and if you're comfortable moving forward.

tests/testsuite/standard_lib.rs Outdated Show resolved Hide resolved
tests/testsuite/standard_lib.rs Outdated Show resolved Hide resolved
@alexcrichton
Copy link
Member Author

It's a bit elaborate

I'm curious to get a bit more of your feeling on this, is this maybe too elaborate? I stared with a blank slate and basically just kept hitting "compile" and filled out all the dependencies and pub use and whatnot. It did end up needing a bit more than I was hoping for (especially having to mark something as #![panic_runtime]).

I'm not sure how we'd simplify this much more though without broader changes to rustc/cargo, which I'm pretty unwilling to do just for the sake of tests.

Will this still work if we switch cargo to create its own sysroot?

I believe so yeah, I made sure that the tests currently do e.env("RUSTFLAGS", "--sysroot=/path/to/nowhere"); which should make them resilient to a switching --sysroot.

@ehuss
Copy link
Contributor

ehuss commented Sep 12, 2019

is this maybe too elaborate?

Eh, I think the only concern is the maintenance burden going forward, updating it to match upstream as needed. I suspect the rate of change should be small, so I don't think it is too much of a problem. If the tests end up being fussy or break often, we can rethink it later if needed. Otherwise it will just be a directory full of files nobody will ever need to look at.

@alexcrichton
Copy link
Member Author

Ok that sounds good. If this is hard to manage then let's deal with it then.

@alexcrichton
Copy link
Member Author

Ok I've pushed up what I'm thinking for a full integration test doing everything, want to double check me @ehuss?

@ehuss
Copy link
Contributor

ehuss commented Sep 12, 2019

Looks like the rustup component add rust-src step needs to be moved around.

@alexcrichton
Copy link
Member Author

Ok green now!

@bors
Copy link
Collaborator

bors commented Sep 16, 2019

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

@bors
Copy link
Collaborator

bors commented Sep 16, 2019

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

This commit is aimed directly at rust-lang/wg-cargo-std-aware#33 and in
general making the `-Zbuild-std` tests more robust. The main change here
is that a new source tree is checked in, `tests/testsuite/mock-std`,
which mirrors rust-lang/rust's own tree for libstd. This mock tree is as
empty as it can be, ideally duplicating almost nothing but for not
requiring duplication of Cargo metadata about patches and such.

The end result here looks like:

* All `-Zbuild-std` tests are now run in parallel
* All tests run much more quickly since they're compiling tiny crates
  instead of actually compiling libstd/libcore
* No tests require network access
* We verify that crates have access to the "custom" libraries
  that we build

Coverage of tests is not currently expanded, but it's hoped that we
could add that shortly afterwards. Coverage has actually gone down
slightly since the custom target test was commented out temporarily and
the full integration test of running `-Zbuild-std` isn't run on CI any
more.

Closes rust-lang/wg-cargo-std-aware#33
Extract out all our test support code to its own standalone crate so it
can be shared between multiple test suites if necessary.
Only run these tests on one CI builder (not all platforms) though. This
is extremely resource intensive since it rebuilds libstd. Currently this
does not share a build directly like before because the number of tests
are supposed to be small, but if necessary we can add that in later too.
@ehuss
Copy link
Contributor

ehuss commented Sep 16, 2019

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 16, 2019

📌 Commit ebd1052 has been approved by ehuss

@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 Sep 16, 2019
@bors
Copy link
Collaborator

bors commented Sep 16, 2019

⌛ Testing commit ebd1052 with merge 13bc9a1...

bors added a commit that referenced this pull request Sep 16, 2019
Improve test suite for `-Zbuild-std`

This commit is aimed directly at rust-lang/wg-cargo-std-aware#33 and in
general making the `-Zbuild-std` tests more robust. The main change here
is that a new source tree is checked in, `tests/testsuite/mock-std`,
which mirrors rust-lang/rust's own tree for libstd. This mock tree is as
empty as it can be, ideally duplicating almost nothing but for not
requiring duplication of Cargo metadata about patches and such.

The end result here looks like:

* All `-Zbuild-std` tests are now run in parallel
* All tests run much more quickly since they're compiling tiny crates
  instead of actually compiling libstd/libcore
* No tests require network access
* We verify that crates have access to the "custom" libraries
  that we build

Coverage of tests is not currently expanded, but it's hoped that we
could add that shortly afterwards. Coverage has actually gone down
slightly since the custom target test was commented out temporarily and
the full integration test of running `-Zbuild-std` isn't run on CI any
more.

Closes rust-lang/wg-cargo-std-aware#33
@bors
Copy link
Collaborator

bors commented Sep 16, 2019

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 13bc9a1 to master...

@bors bors merged commit ebd1052 into rust-lang:master Sep 16, 2019
@alexcrichton alexcrichton deleted the mock-std branch September 16, 2019 23:46
@ehuss ehuss added this to the 1.39.0 milestone Feb 6, 2022
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.

Consider better testing strategy.
4 participants