Skip to content

Breakup test.rs into more-manageable sub-modules #137224

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

Closed
wants to merge 25 commits into from

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Feb 18, 2025

Part of Tracking issue for bootstrap test step cleanups #137178.

Summary of changes

Split up test.rs into smaller, more logically organized sub-modules.

  • Primarily code motion, instead of a massive hard-to-navigate and hard-to-comprehend test.rs file.
    • One of the purposes of this is to highlight test steps that seem "special" or "unusual". Hopefully, this makes it easier to add new test steps correctly by making it easier to find similar test steps, and compare the differences w/ more "special" test steps.
  • One small change to change testdir to use builder.test_out (to prevent reinventing builder.test_out and having yet another place that might desync in the future).
  • Left a couple of FIXMEs backlinking to Tracking issue for bootstrap test step cleanups #137178, intended for follow-ups.
  • Added a few module-level doc comments for some of the new test submodules.

More context

Review advice

  • There should be no functional changes.
  • The commit history is not as clean as I would like (there's a few instances where I realized a while later that I missed a step or two for a given broken-out module, but rebasing meant a bunch of merge conflicts that were tricky to resolve). That being said, I tried to keep the commits as atomic1 as possible.
  • Best reviewed commit-by-commit.

r? @onur-ozkan

Footnotes

  1. For some definition of atomic, I suppose

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Feb 18, 2025
@jieyouxu jieyouxu marked this pull request as ready for review February 18, 2025 16:04
@jieyouxu jieyouxu changed the title Breakup test.rs into more-manageable modules Breakup test.rs into more-manageable sub-modules Feb 18, 2025
@jieyouxu
Copy link
Member Author

I may redo this after #137215.

Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Looking at the new files, I think they seem more complicated than before. The policy or motivation behind this splitting logic is unclear.

Example of things that are unclear:

  • std_tests can also contain cargo tests, but there is a module called cargotest. Where should we put cargo tests of a program?
  • Why some tools (like compiletest and rustdoc) have separate module for its tests but not other tools?
  • What does cargotest mean? Is it for running cargo tests on projects, or is it for running tests of cargo tool?
  • When to create a separate module for tests? What is the right time for it?
  • ...the list can go very long.

The separation logic should be very clear and not require us to think too much about when to create a test module or where to place a test.

I'm pretty sure that if we use this structure people will start asking in Zulip threads "Which module should I use to add this test?".

I suggest to keep the module separation as simple and basic as possible.

@onur-ozkan
Copy link
Member

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2025
@jieyouxu
Copy link
Member Author

Yeah, I'm not super happy about how this ended up. I'll have to rethink this a bit.

@jieyouxu
Copy link
Member Author

jieyouxu commented Feb 19, 2025

What does cargotest mean? Is it for running cargo tests on projects, or is it for running tests of cargo tool?

cargotest is actually src/tools/cargotest (a mini-crater-at-home thing), which is very confusing indeed (I find this name very unfortunate).

@jieyouxu
Copy link
Member Author

I'll probably revisit this in the future, so closing for the time being.

@jieyouxu jieyouxu closed this Feb 19, 2025
@jieyouxu jieyouxu deleted the breakup-test branch February 19, 2025 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants