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

Make x test tests work #121372

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Make x test tests work #121372

merged 1 commit into from
Feb 22, 2024

Conversation

clubby789
Copy link
Contributor

Fixes #97314

This makes x test tests work, and be roughly equivalent to x test tests/*. The --dry-run output is identical, except for errors on the non-test items in tests and a couple of things being in a different order (where path != struct name).

This probably needs a test, but I'm not sure of the best way to do it.

@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2024

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
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

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 21, 2024
@rustbot

This comment was marked as outdated.

@clubby789
Copy link
Contributor Author

Whoops, left over commit from something else

@compiler-errors
Copy link
Member

I was about to ask why 😂

@clubby789 clubby789 removed the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Feb 21, 2024
@rust-log-analyzer

This comment has been minimized.

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.

I think we can make this work without adding a fake build step by improving the path remapping in builder.rs:

const PATH_REMAP: &[(&str, &str)] = &[("rust-analyzer-proc-macro-srv", "proc-macro-srv-cli")];
fn remap_paths(paths: &mut Vec<&Path>) {
for path in paths.iter_mut() {
for &(search, replace) in PATH_REMAP {
if path.to_str() == Some(search) {
*path = Path::new(replace)
}
}
}
}

if path.to_str() == Some(search) {
*path = Path::new(replace)
// Remove leading and trailing slashes so `tests/` and `tests` are equivalent
if path.trim_matches(std::path::is_separator) == search {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Different shells can tab-complete a folder name with or without a trailing slash so hopefully this can handle both

@bors
Copy link
Contributor

bors commented Feb 21, 2024

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

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.

Thanks!

@onur-ozkan
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 21, 2024

📌 Commit eee5672 has been approved by onur-ozkan

It is now in the queue for this repository.

@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 Feb 21, 2024
Comment on lines +297 to +300
"tests",
&[
"tests/assembly",
"tests/codegen",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing can go wrong with hardcoded paths.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe collects folders in /tests at runtime and assert that lists equal (excluding auxiliary)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll follow this one up with a test

Copy link
Contributor

Choose a reason for hiding this comment

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

And now this list missing tests/crashes

Copy link
Member

@onur-ozkan onur-ozkan May 10, 2024

Choose a reason for hiding this comment

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

We have a test for checking the existance paths. Seems like we need a test for missing paths too.

Copy link
Member

Choose a reason for hiding this comment

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

We have a test for checking the existance paths. Seems like we need a test for missing paths too.

Here it is: #124969

@bors
Copy link
Contributor

bors commented Feb 22, 2024

⌛ Testing commit eee5672 with merge 933a05b...

@bors
Copy link
Contributor

bors commented Feb 22, 2024

☀️ Test successful - checks-actions
Approved by: onur-ozkan
Pushing 933a05b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 22, 2024
@bors bors merged commit 933a05b into rust-lang:master Feb 22, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 22, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (933a05b): comparison URL.

Overall result: ✅ improvements - 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
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-2.4%, -2.4%] 1
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)
4.3% [4.3%, 4.3%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 4.3% [4.3%, 4.3%] 1

Cycles

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

Binary size

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

Bootstrap: 649.076s -> 649.98s (0.14%)
Artifact size: 310.97 MiB -> 310.97 MiB (-0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x.py test tests/ doesn't work
8 participants