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

Move src/test to the root #106458

Merged
merged 2 commits into from
Jan 11, 2023
Merged

Move src/test to the root #106458

merged 2 commits into from
Jan 11, 2023

Conversation

albertlarsan68
Copy link
Member

@albertlarsan68 albertlarsan68 commented Jan 4, 2023

See MCP at rust-lang/compiler-team#573

There may be more changes needed.

The first commit is just the move of the files:
You can check that the first commit did not do anything else than renames by running

git diff --diff-filter=r -M100% <rust-lang remote>/master <first commit hash>

The output should be empty, because the filter excludes renames, and the match threshold for qualifying a rename is 100%.

The second one is mostly a "find and replace" of src/test to tests and whatever is needed to make CI pass.

What is left to do:

  • Move directory
  • Change references to src/test
    • Change references in-tree
    • Change references in submodules / out-of-tree docs
  • Make CI pass:
    • Fix tidy
    • Fix tests
    • Bless tests if needed (shouldn't normally)
  • Merge it !

@jyn514 jyn514 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 4, 2023
@jyn514
Copy link
Member

jyn514 commented Jan 4, 2023

@albertlarsan68 in addition to fixing the CI failures, please also make sure you update src/bootstrap/test.rs to add .path("test/suite_name") to all the test suites. I think for now we should keep x test src/test/ui as an alias for x test test/ui, but we should definitely support the latter.

@jyn514 jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-meta Area: Issues about the rust-lang/rust repository. labels Jan 4, 2023
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

I guess the only question is whether to keep the name test, or rename it to tests which is the cargo convention for integration tests.

@jyn514
Copy link
Member

jyn514 commented Jan 4, 2023

I would prefer tests, both for consistency with cargo and to avoid ambiguity with library/test in case we ever want to make x test tests work (i.e. run only compiletest suites and not unit tests or tidy).

@the8472
Copy link
Member

the8472 commented Jan 5, 2023

This should split into multiple commits? one that only moves files and the rest for adjustments to the tools. That'll make it possible to review the code changes on GH.

@bors
Copy link
Contributor

bors commented Jan 5, 2023

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

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 5, 2023
@rust-log-analyzer

This comment has been minimized.

@albertlarsan68
Copy link
Member Author

albertlarsan68 commented Jan 5, 2023

This PR will be very rebase-heavy, as CI refuses to run if there are merge conflicts, and almost every merged PR will modify tests.

@albertlarsan68 albertlarsan68 force-pushed the move-tests branch 6 times, most recently from b29c500 to 9e5d9d2 Compare January 5, 2023 13:00
@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Jan 5, 2023

@albertlarsan68 you may want to temporarily disable the x86_64-gnu-tools job until #106449 lands to avoid CI spuriously failing while you're trying to fix other things.

Also, given the rebase problem, you may want to run the tests locally first instead of primarily using CI. If that's prohibitively slow I can look into getting you access to one of the cloud dev machines.

@albertlarsan68
Copy link
Member Author

albertlarsan68 commented Jan 5, 2023

I just wanted to see if this x86_64-gnu-tools job builds, to verify that I did not break it, this is why I disabled the known failing x86_64-llvm-13 job.

Concerning the cloud dev machine, I am not in desperate need, but it will allow much faster iterations.
Having to rebase every time I want to test something allows to minimize bitrot.

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 11, 2023
@bors bors merged commit b22c152 into rust-lang:master Jan 11, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 11, 2023
@albertlarsan68 albertlarsan68 deleted the move-tests branch January 11, 2023 14:43
@JohnTitor
Copy link
Member

The diff is huge and I cannot check on web UI, but is there a tidy check that prevents contributors from adding a test to src/test?
A PR modifying an existing test now has a merge conflict e.g. #106167 but a PR adding a new test doesn't e.g. #106660, and I'm worried if a new test is added to src/test by accident.

@albertlarsan68
Copy link
Member Author

No, there isn't.
Can you open an issue for it ?
I will add it.

@albertlarsan68
Copy link
Member Author

Can you also open one to update the references in triagebot.toml if it isn't already done ?

@JohnTitor
Copy link
Member

Sure, opened #106724.

@JohnTitor
Copy link
Member

Can you also open one to update the references in triagebot.toml if it isn't already done ?

Couldn't find any reference:
image

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b22c152): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

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)
2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)
-2.9% [-3.2%, -2.5%] 2
Improvements ✅
(secondary)
-4.5% [-4.5%, -4.5%] 1
All ❌✅ (primary) -2.9% [-3.2%, -2.5%] 2

Cycles

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

@jhpratt
Copy link
Member

jhpratt commented Jan 11, 2023

@albertlarsan68 Can you double check tidy? This error is definitely not accurate, as there are test files with the required lines.

@jyn514
Copy link
Member

jyn514 commented Jan 11, 2023

@jhpratt that error looks correct, the tests should be in tests/ui/restrictions, not tests/restrictions

@jhpratt
Copy link
Member

jhpratt commented Jan 11, 2023

Duh...thanks for the response!

@rust-log-analyzer

This comment has been minimized.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 6, 2023
…mulacrum

Fix the test directories suggested by `./x.py suggest`

It seems that these paths were correct when rust-lang#106249 was being written, but since then rust-lang#106458 has been merged (moving `src/test/` to `tests/`), making the tool's suggestions incorrect.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 6, 2023
…mulacrum

Fix the test directories suggested by `./x.py suggest`

It seems that these paths were correct when rust-lang#106249 was being written, but since then rust-lang#106458 has been merged (moving `src/test/` to `tests/`), making the tool's suggestions incorrect.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 6, 2023
…mulacrum

Fix the test directories suggested by `./x.py suggest`

It seems that these paths were correct when rust-lang#106249 was being written, but since then rust-lang#106458 has been merged (moving `src/test/` to `tests/`), making the tool's suggestions incorrect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues about the rust-lang/rust repository. A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.