-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Migrate output-type-permutations
run-make
test to rmake
#127098
Conversation
This PR modifies cc @jieyouxu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, what a fun test this is. I think that the "middle section" (at least lines 48 - 78, but maybe even until line 123) could be made more readable by introducing some helper, that would receive rustc
in a closure, set flags for it, fill in the emit combinations, and remove the created files between each invocation. But maybe the test structure is too irregular for that, it would need to be tried :)
@rustbot author |
☔ The latest upstream changes (presumably #127121) made this pull request unmergeable. Please resolve the merge conflicts. |
The run-make-support library was changed cc @jieyouxu |
81a8bf8
to
da50aae
Compare
@rustbot review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I do find the new structure easier to follow because each sub-test is cleanly grouped. I have some more suggestions which I think makes the test easier to read.
must_exist: &[&'static str], | ||
can_exist: &[&'static str], | ||
dir: &str, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: so I thought maybe we could have a param struct for must_exist
, can_exist
and dir
, something like
struct Expectations {
/// Name of the directory where the test happens.
test_dir: String,
/// Output files which must be found. The test fails if any are absent.
expected_files: Vec<String>,
/// Allowed output files which will not trigger a failure.
allowed_files: Vec<String>,
}
and callsite (each test call) can be like
assert_expected_output_files(
Expectations {
test_dir: "foo.bar".to_string(),
expected_files: s!["a.lib", static_lib_name("bar")],
allowed_files: s!["b.dylib"],
},
|| { todo!() }
);
I find this helpful because it clearly documents which each parameter means local to each assert_expected_output_files
call.
the s![]
macro is just a little helper so we can have a mixed list of various kinds of strings (so as long as they impl ToString
), it just converts them to a simple Vec<String>
.
macro_rules! s {
( $( $x:expr ),* ) => {
{
let mut temp_vec = Vec::new();
$(
temp_vec.push($x.to_string());
)*
temp_vec
}
};
}
This avoids having to do the Box::leak(Box::new(...))
dance you have below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I used a s![]
declarative macro here and not try to do something like I: IntoIterator, I::Item: AsRef<str>
because that can't handle heterogeneous element types (like having [&'a str, String]
wouldn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented, see latest commit. Waiting for green CI to switch to review.
@rustbot author |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, let's run some try jobs, r=me if they come back green.
@bors try |
Migrate `output-type-permutations` `run-make` test to rmake Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). try-job: x86_64-msvc try-job: armhf-gnu try-job: aarch64-apple try-job: test-various
This comment has been minimized.
This comment has been minimized.
Migrate `output-type-permutations` `run-make` test to rmake Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). try-job: x86_64-msvc try-job: armhf-gnu try-job: aarch64-apple try-job: test-various
☀️ Try build successful - checks-actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good, can you x fmt this test? (does x fmt currently work on rmake.rs, I forgor)
☔ The latest upstream changes (presumably #127360) made this pull request unmergeable. Please resolve the merge conflicts. |
@rustbot review |
@bors r=Kobzol,jieyouxu |
…kingjubilee Rollup of 5 pull requests Successful merges: - rust-lang#125751 (Add `new_range_api` for RFC 3550) - rust-lang#127098 (Migrate `output-type-permutations` `run-make` test to rmake) - rust-lang#127369 (Match ergonomics 2024: align with RFC again) - rust-lang#127383 (Use verbose style for argument removal suggestion) - rust-lang#127392 (Use verbose suggestion for changing arg type) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#127098 - Oneirical:big-test-party, r=Kobzol,jieyouxu Migrate `output-type-permutations` `run-make` test to rmake Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html). try-job: x86_64-msvc try-job: armhf-gnu try-job: aarch64-apple try-job: test-various
Part of #121876 and the associated Google Summer of Code project.
try-job: x86_64-msvc
try-job: armhf-gnu
try-job: aarch64-apple
try-job: test-various