Put the core unit tests in a separate coretests package#135937
Conversation
rustbot
added
A-testsuite
|
There's a libcore comment that should be updated: Lines 46 to 47 in fc0094f |
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
|
|
||
| fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { | ||
| run.crate_or_deps("sysroot").path("library") | ||
| run.crate_or_deps("sysroot").crate_or_deps("coretests").path("library") |
There was a problem hiding this comment.
cc @onur-ozkan: do you know if this invocation (and the one in test) is correct?
|
FYI @rust-lang/libs since this is restructuring libcore tests |
|
I'll do some more local testing tmrw. |
This comment has been minimized.
This comment has been minimized.
|
libs-nominating for awareness. |
rustbot
added
the
I-libs-nominated
There was a problem hiding this comment.
Thanks for working on this, I believe this is definitely an improvement over the current setup. I did some local manual testing, and here are my findings:
With profile = "library"1:
- Basic
library/coreteststest:./x test library/coretests... OK - Basic
library/coreteststest (unmodified re-run):./x test library/coretests... RERUNS TESTS- Is it expected to re-run all coretests tests even though nothing changed?
- Alias:
./x test coretests... OK - Alias (unmodified re-run):
./x test coretests... RERUNS TESTS - Basic test:
./x test core... OK2 - Basic test (unmodified re-run):
./x test core... RERUNS TESTS- Is it expected to re-run all core tests even though nothing changed?
The test reruns here are not very problematic because they are very fast to run (at least on my
machine).
Unrelated sanity checks:
- Basic doc:
./x doc core... OK - Basic build:
./x build library/core... OK - Basic build (unmodified re-run):
./x build library/core... OK
The bootstrap changes look good to me (even with the test re-run because they are really fast anyway) and @onur-ozkan, but I'll hand this PR off to a libs reviewer for final sign-off (to really make sure libs contributors are aware of the change).
Footnotes
|
Note that unfortunately this also has a merge conflict against latest master because a @bors rollup=never p=10 (conflict-prone, and also non-trivial coretests movement) |
|
r? libs (for final sign-off) |
tgross35
left a comment
tgross35
left a comment
There was a problem hiding this comment.
Everything looks correct to me from a technical perspective, but this definitely needs an ack from a few team members (was there a discussion on Zulip?)
Opened a T-libs zulip thread: https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/Moving.20libcore.20unit.20tests.20into.20dedicated.20.60coretests.60.20package. Marking PR as waiting on T-libs consensus. |
jieyouxu
added
S-waiting-on-team
and removed
S-waiting-on-review
|
There seems to be some consensus, so @rustbot review |
| $(Q)MIRIFLAGS="-Zmiri-disable-isolation" \ | ||
| $(BOOTSTRAP) miri --stage 2 \ | ||
| library/core \ | ||
| library/coretests \ |
There was a problem hiding this comment.
I think this here entirely disabled running the libcore doctests in Miri...
10 participants
Having standard library tests in the same package as a standard library crate has bad side effects. It causes the test to have a dependency on a locally built standard library crate, while also indirectly depending on it through libtest. Currently this works out fine in the context of rust's build system as both copies are identical, but for example in cg_clif's tests I've found it basically impossible to compile both copies with the exact same compiler flags and thus the two copies would cause lang item conflicts.
This PR moves the tests of libcore to a separate package which doesn't depend on libcore, thus preventing the duplicate crates even when compiler flags don't exactly match between building the sysroot (for libtest) and building the test itself. The rest of the standard library crates do still have this issue however.