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.

Sign up for GitHub

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

Jump to bottom

compiletest's 'SKIPPED 246746 BYTES' logic breaks tests #94322

Closed
m-ou-se opened this issue Feb 24, 2022 · 2 comments · Fixed by #115706
Closed

compiletest's 'SKIPPED 246746 BYTES' logic breaks tests #94322

m-ou-se opened this issue Feb 24, 2022 · 2 comments · Fixed by #115706
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue.

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Feb 24, 2022

src/tools/compiletest truncates large outputs by replacing the middle by <<<<<< SKIPPED {} BYTES >>>>>>:

rust/src/tools/compiletest/src/read2.rs

Line 52 in 7ccfe2f

write!(&mut head, "\n\n<<<<<< SKIPPED {} BYTES >>>>>>\n\n", skipped).unwrap();

This can break the test runner which tries to parse the output as json, which will break on the <<< .. >>> message. For example here: https://github.com/rust-lang/rust/runs/4586149338?check_suite_focus=true:

thread '[ui] ui/suggestions/missing-lifetime-specifier.rs' panicked at 'explicit panic', src/tools/compiletest/src/json.rs:121:21

This is blocking #92123 (A workaround for this PR could be to simply increase the threshold, but we should probably have a better solution.)
@m-ou-se m-ou-se added A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue. labels Feb 24, 2022
@m-ou-se m-ou-se mentioned this issue Feb 24, 2022
Merged
@Mark-Simulacrum Mark-Simulacrum mentioned this issue Feb 24, 2022
Merged
@Aaron1011
Copy link
Member

See also #92211

@Mark-Simulacrum
Copy link
Member

#94327 should help with this specific instance, though is not a root cause fix. In general limiting the number of JSON bytes is a pretty poor estimate of the number of bytes in stderr files; it might make sense to avoid the abbreviated read abstraction for that part of compiletest. (Or, at least, set the limit to a few megabytes or so).

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 24, 2022
@Dylan-DPC
Rollup merge of rust-lang#94327 - Mark-Simulacrum:avoid-macro-sp, r=p…
39d8195 
…etrochenkov

Avoid emitting full macro body into JSON errors

While investigating rust-lang#94322, it was noted that currently the JSON diagnostics for macro backtraces include the full def_site span -- the whole macro body.

It seems like this shouldn't be necessary, so this PR adjusts the span to just be the "guessed head", typically the macro name. It doesn't look like we keep enough information to synthesize a nicer span here at this time.

Atop rust-lang#92123, this reduces output for the src/test/ui/suggestions/missing-lifetime-specifier.rs test from 660 KB to 156 KB locally.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 24, 2022
@Dylan-DPC
Rollup merge of rust-lang#94327 - Mark-Simulacrum:avoid-macro-sp, r=p…
7be2674 
…etrochenkov

Avoid emitting full macro body into JSON errors

While investigating rust-lang#94322, it was noted that currently the JSON diagnostics for macro backtraces include the full def_site span -- the whole macro body.

It seems like this shouldn't be necessary, so this PR adjusts the span to just be the "guessed head", typically the macro name. It doesn't look like we keep enough information to synthesize a nicer span here at this time.

Atop rust-lang#92123, this reduces output for the src/test/ui/suggestions/missing-lifetime-specifier.rs test from 660 KB to 156 KB locally.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Feb 24, 2022
@Dylan-DPC
Rollup merge of rust-lang#94327 - Mark-Simulacrum:avoid-macro-sp, r=p…
3bd163f 
…etrochenkov

Avoid emitting full macro body into JSON errors

While investigating rust-lang#94322, it was noted that currently the JSON diagnostics for macro backtraces include the full def_site span -- the whole macro body.

It seems like this shouldn't be necessary, so this PR adjusts the span to just be the "guessed head", typically the macro name. It doesn't look like we keep enough information to synthesize a nicer span here at this time.

Atop rust-lang#92123, this reduces output for the src/test/ui/suggestions/missing-lifetime-specifier.rs test from 660 KB to 156 KB locally.
@ehuss ehuss mentioned this issue Apr 20, 2022
Closed
@nikic nikic mentioned this issue Apr 25, 2022
Closed
@pietroalbini pietroalbini mentioned this issue Apr 29, 2022
Merged
aarongable pushed a commit to chromium/chromium that referenced this issue May 16, 2022
@chbaker0
Update Rust revision, include src, and disable bad test
a6e76db 
Rolls a new Rust toolchain with the following changes in addition to
updating the revision:

With rust-lang/rust#96493, disables test
src/test/ui/numeric/numeric-cast.rs (which fails on our CI due to bug
rust-lang/rust#94322) and re-enables the rest
of the src/test/ui suite.

Includes Rust std sources and vendored dependencies in the package at
third_party/rust-toolchain/lib/rustlib/src/rust/

Fixed: 1320459
Change-Id: I6b25be163214c4408123f3abb7e0135e94ec642e

Cq-Include-Trybots: luci.chromium.try:linux-rust-x64-rel,linux-rust-x64-dbg
Change-Id: I6b25be163214c4408123f3abb7e0135e94ec642e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3635438
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Reviewed-by: Adrian Taylor <adetaylor@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1003924}
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 6, 2022
@bors
Auto merge of rust-lang#96551 - ferrocene:pa-ignore-paths-when-abbrev…
6609c67 
…iating, r=Mark-Simulacrum

[compiletest] Ignore known paths when abbreviating output

To prevent out of memory conditions, compiletest limits the amount of output a test can generate, abbreviating it if the test emits more than a threshold. While the behavior is desirable, it also causes some issues (like rust-lang#96229, rust-lang#94322 and rust-lang#92211).

The latest one happened recently, when the `src/test/ui/numeric/numeric-cast.rs` test started to fail on systems where the path of the rust-lang/rust checkout is too long. This includes my own development machine and [LLVM's CI](rust-lang#96362 (comment)). Rust's CI uses a pretty short directory name for the checkout, which hides these sort of problems until someone runs the test suite on their own computer.

When developing the fix I tried to find the most targeted fix that would prevent this class of failures from happening in the future, deferring the decision on if/how to redesign abbreviation to a later date. The solution I came up with was to ignore known base paths when calculating whether the output exceeds the abbreviation threshold, which removes this kind of nondeterminism.

This PR is best reviewed commit-by-commit.
@ehuss ehuss mentioned this issue Sep 8, 2023
Closed
@RalfJung RalfJung mentioned this issue Sep 9, 2023
Merged
@bors bors closed this as completed in 8e455db Sep 13, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 15, 2023
@bors
Auto merge of #115706 - RalfJung:compiletest-truncation, r=wesleywiser
3a488aa 
Make compiletest output truncation less disruptive

When the test output becomes too large, compiletest stops recording all of it. However:
- this can lead to invalid JSON, which then causes compiletest itself to throw further errors
- the note that output was truncated is in the middle of the output, with >100kb of text on each side; that makes it almost impossible to actually see that note in the terminal

So assuming that we do need to keep the output truncation, I propose that we only ever do a cut at the end, so that it is very clear by looking at the end of the log that truncation happened. I added a message at the beginning of the output as well. Also I added some logic to make it less likely that we'll cut things off in the middle of a JSON record. (I tested that successfully by reducing the output limit to something very low and running a few ui tests.) Furthermore I increased the max buffer size to 512KB; that's really not a lot of memory compared to how much RAM it takes to build rustc (it's ~25% more than the previous maximum HEAD+TAIL length). And finally, the information that things got truncated is now propagated to the higher levels, so that we can fail the test instead of comparing the truncated output with the reference.

Fixes rust-lang/rust#115675
Fixes rust-lang/rust#96229
Fixes rust-lang/rust#94322
Fixes rust-lang/rust#92211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make compiletest output truncation less disruptive RalfJung/rust
3 participants
@m-ou-se @Aaron1011 @Mark-Simulacrum