Put the core unit tests in a separate coretests package #135937

bjorn3 · 2025-01-23T16:03:31Z

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.

rustbot · 2025-01-23T16:03:40Z

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

jieyouxu · 2025-01-23T16:10:25Z

There's a libcore comment that should be updated:

rust/library/core/src/lib.rs

Lines 46 to 47 in fc0094f

    
           // Since core defines many fundamental lang items, all tests live in a 
        
           // separate crate, libcoretest (library/core/tests), to avoid bizarre issues.

rustbot · 2025-01-23T16:12:31Z

These commits modify the library/Cargo.lock file. Unintentional changes to library/Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

jieyouxu · 2025-01-23T16:12:54Z

src/bootstrap/src/core/build_steps/check.rs

@@ -45,7 +45,7 @@ impl Step for Std {
    const DEFAULT: bool = true;

    fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> {
-        run.crate_or_deps("sysroot").path("library")
+        run.crate_or_deps("sysroot").crate_or_deps("coretests").path("library")


cc @onur-ozkan: do you know if this invocation (and the one in test) is correct?

Seems right to me

jieyouxu · 2025-01-23T16:14:08Z

FYI @rust-lang/libs since this is restructuring libcore tests

jieyouxu · 2025-01-23T16:14:54Z

I'll do some more local testing tmrw.

jieyouxu · 2025-01-24T07:32:03Z

libs-nominating for awareness.
@rustbot label: +I-libs-nominated

jieyouxu

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"¹:

Basic library/coretests test: ./x test library/coretests... OK
Basic library/coretests test (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... OK²
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).

No modified coretests/core flows were tested because they already rerun anyway. ↩
This is also weird because ./x build core is not recognized as a registered step, but ./x test core tests stuff. ↩

jieyouxu · 2025-01-24T07:40:37Z

Note that unfortunately this also has a merge conflict against latest master because a bstr test was changed.

@bors rollup=never p=10 (conflict-prone, and also non-trivial coretests movement)

jieyouxu · 2025-01-24T07:40:55Z

r? libs (for final sign-off)

tgross35

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?)

jieyouxu · 2025-01-24T07:57:13Z

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 · 2025-01-25T06:43:44Z

There seems to be some consensus, so @rustbot review

rust-timer · 2025-01-27T08:09:20Z

Finished benchmarking commit (633a3fe): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.4%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.4%	[-3.6%, -1.3%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.4%	[-3.6%, -1.3%]	2

Cycles

Results (primary 0.3%, secondary -1.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.7%	[2.7%, 2.7%]	1
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-1.8%	[-2.1%, -1.6%]	5
All ❌✅ (primary)	0.3%	[-2.0%, 2.7%]	2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 772.928s -> 773.067s (0.02%)
Artifact size: 328.22 MiB -> 328.21 MiB (-0.00%)

…=cuviper Put the alloc unit tests in a separate alloctests package Same rationale as rust-lang#135937. This PR has some extra complexity though as a decent amount of tests are testing internal implementation details rather than the public api. As such I opted to include the modules containing the types under test using `#[path]` into the alloctests package. This means that those modules still need `#[cfg(test)]`, but the rest of liballoc no longer need it.

…try> Put the alloc unit tests in a separate alloctests package Same rationale as rust-lang#135937. This PR has some extra complexity though as a decent amount of tests are testing internal implementation details rather than the public api. As such I opted to include the modules containing the types under test using `#[path]` into the alloctests package. This means that those modules still need `#[cfg(test)]`, but the rest of liballoc no longer need it. try-job: x86_64-gnu-aux

…=cuviper Put the alloc unit tests in a separate alloctests package Same rationale as rust-lang#135937. This PR has some extra complexity though as a decent amount of tests are testing internal implementation details rather than the public api. As such I opted to include the modules containing the types under test using `#[path]` into the alloctests package. This means that those modules still need `#[cfg(test)]`, but the rest of liballoc no longer need it.

RalfJung · 2025-02-23T11:07:06Z

I just spent 15min trying to figure out why ./x miri library/core no longer runs any tests. ./x test library/core has the same issue it turns out, and it's caused by this PR. That's quite the footgun! I feel like such a fundamental change to how we interact with the compiler build should be communicated a bit more broadly. But communication probably is not enough, this PR is a serious regression for the UX of testing library changes and I think that needs to be addressed before the same scheme is applied to more crates.

bjorn3 · 2025-02-26T15:38:01Z

#137679 will fix both ./x.py test library/core and ./x.py miri library/core to test coretests too.

…=cuviper Put the alloc unit tests in a separate alloctests package Same rationale as rust-lang#135937. This PR has some extra complexity though as a decent amount of tests are testing internal implementation details rather than the public api. As such I opted to include the modules containing the types under test using `#[path]` into the alloctests package. This means that those modules still need `#[cfg(test)]`, but the rest of liballoc no longer need it.

Rollup merge of rust-lang#136642 - bjorn3:separate_alloctest_crate, r=cuviper Put the alloc unit tests in a separate alloctests package Same rationale as rust-lang#135937. This PR has some extra complexity though as a decent amount of tests are testing internal implementation details rather than the public api. As such I opted to include the modules containing the types under test using `#[path]` into the alloctests package. This means that those modules still need `#[cfg(test)]`, but the rest of liballoc no longer need it.

rustbot assigned cjgillot Jan 23, 2025

jieyouxu self-assigned this Jan 23, 2025

jieyouxu reviewed Jan 23, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

bjorn3 mentioned this pull request Jan 23, 2025

Update emscripten std tests #135948

Merged

rustbot added the I-libs-nominated Nominated for discussion during a libs team meeting. label Jan 24, 2025

jieyouxu approved these changes Jan 24, 2025

View reviewed changes

rustbot assigned tgross35 and unassigned cjgillot and jieyouxu Jan 24, 2025

jieyouxu closed this Jan 24, 2025

jieyouxu reopened this Jan 24, 2025

tgross35 approved these changes Jan 24, 2025

View reviewed changes

jieyouxu added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2025

bjorn3 deleted the separate_coretests_crate branch February 1, 2025 16:39

bjorn3 mentioned this pull request Feb 6, 2025

Put the alloc unit tests in a separate alloctests package #136642

Merged

RalfJung mentioned this pull request Feb 23, 2025

Update to run "coretests" rust-lang/miri-test-libstd#88

Closed

This was referenced Feb 23, 2025

./x test library/core stopped working #137478

Closed

Can we get rid of the lib.miri.rs hack now? #137479

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put the core unit tests in a separate coretests package #135937

Put the core unit tests in a separate coretests package #135937

bjorn3 commented Jan 23, 2025

rustbot commented Jan 23, 2025

jieyouxu commented Jan 23, 2025

rustbot commented Jan 23, 2025

jieyouxu Jan 23, 2025

onur-ozkan Jan 23, 2025

jieyouxu commented Jan 23, 2025

jieyouxu commented Jan 23, 2025

This comment has been minimized.

jieyouxu commented Jan 24, 2025

jieyouxu left a comment •

edited

Loading

jieyouxu commented Jan 24, 2025 •

edited

Loading

jieyouxu commented Jan 24, 2025

tgross35 left a comment

jieyouxu commented Jan 24, 2025 •

edited

Loading

jieyouxu commented Jan 25, 2025

rust-timer commented Jan 27, 2025

RalfJung commented Feb 23, 2025 •

edited

Loading

bjorn3 commented Feb 26, 2025

Put the core unit tests in a separate coretests package #135937

Put the core unit tests in a separate coretests package #135937

Conversation

bjorn3 commented Jan 23, 2025

rustbot commented Jan 23, 2025

jieyouxu commented Jan 23, 2025

rustbot commented Jan 23, 2025

jieyouxu Jan 23, 2025

Choose a reason for hiding this comment

onur-ozkan Jan 23, 2025

Choose a reason for hiding this comment

jieyouxu commented Jan 23, 2025

jieyouxu commented Jan 23, 2025

This comment has been minimized.

jieyouxu commented Jan 24, 2025

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

Footnotes

jieyouxu commented Jan 24, 2025 • edited Loading

jieyouxu commented Jan 24, 2025

tgross35 left a comment

Choose a reason for hiding this comment

jieyouxu commented Jan 24, 2025 • edited Loading

jieyouxu commented Jan 25, 2025

rust-timer commented Jan 27, 2025

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

RalfJung commented Feb 23, 2025 • edited Loading

bjorn3 commented Feb 26, 2025

jieyouxu left a comment •

edited

Loading

jieyouxu commented Jan 24, 2025 •

edited

Loading

jieyouxu commented Jan 24, 2025 •

edited

Loading

RalfJung commented Feb 23, 2025 •

edited

Loading