-
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
Add compare-output-lines-by-subset flag to compiletest #110444
Conversation
r? @ozkanonur (rustbot has picked a reviewer for you, use r? to override) |
let mut used = expected_lines.clone(); | ||
used.retain(|line| actual.lines().any(|l| l == *line)); | ||
// check if `expected` contains a subset of the lines of `actual` | ||
if used.len() == expected_lines.len() && (expected.is_empty() == actual.is_empty()) { |
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.
I think you can get this with putting both sets of lines into HashSet and then calling https://doc.rust-lang.org/nightly/std/collections/hash_set/struct.HashSet.html#method.is_subset, while also avoiding both the - if I'm getting the math right - O(n^3) (n^2 for each set and another n for the copies as we remove lines, I think, in the worst case) retain loop.
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.
Isn't this already n^2? For each expected line this iterates through the actual output once. That aside, using a HashSet
doesn't work as it join over the vecs in line 3840 which is order dependent. Unless IndexSet
is used which requires adding it as a dependency to compiletest
. I suppose actual.lines()
could be collected into a HashSet
at least.
This comment has been minimized.
This comment has been minimized.
r=me with commits squashed |
@bors r=Mark-Simulacrum |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
@Veykril Would you mind posting a PR to update the documentation to include this header at https://github.com/rust-lang/rustc-dev-guide/blob/master/src/tests/ui.md#output-comparison and add a link to that at https://github.com/rust-lang/rustc-dev-guide/blob/master/src/tests/headers.md#header-commands? |
Finished benchmarking commit (a57fa08): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. |
Sure, will do that tomorrow 👍 |
For ferrocene we have some compiletests that check the output of the cli arguments to the compiler, including printing things like the target list (
--print target-list
). Unfortunately those tend to change quite often so when we sync we end up with some outputs we have to re-bless constantly, even though the exact output doesn't really matter.We added a new compiletest flag to aid writing these kinds of tests:
compare-output-lines-by-subset
. It checks whether the lines of the expected output are a subset (or equal) to the lines of the actual output. If the expected output is empty it will fail unless the actual output is also empty. We opened this PR hoping the flag might be helpful for other tests in the future (especially if CLI-related tests are added in the future in the rust-lang/rust repo itself).