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

Port tests/run-make/libtest-json to tests/ui #126773

Closed
wants to merge 5 commits into from
Closed

Port tests/run-make/libtest-json to tests/ui #126773

wants to merge 5 commits into from

Conversation

Zalathar
Copy link
Contributor

This was another example of a run-make test that was almost a ui test, but needed a little bit of extra help from compiletest to make that happen.

Specifically, this PR:

  • Adds support for escaped " in //@ normalize-* headers, to make it viable to normalize JSON object fields.
  • Deals with compiletest: Printing process output panics if the output contains unknown JSON-like text #126373 by giving extract_rendered the option to not panic when it sees unfamiliar JSON-like output.
    • It's possible that this behaviour can just be unconditional, but I haven't done the extra work of verifying whether this is OK for all callers.
  • Adds a //@ check-run-stdout-is-json-lines header to verify that run-stdout contains well-formed JSON on each line.
    • The run-make version was calling out to Python to achieve this, but since we already have serde in compiletest we can easily integrate the same check ourselves.

Part of #121876; fixes #126373.

@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
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-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc 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 Jun 21, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Zalathar
Copy link
Contributor Author

Not sure why the meta test is unhappy in CI; I haven't been able to reproduce it locally.

Maybe I can hack things to make the check unconditionally fail, and see what result that has.

@Zalathar
Copy link
Contributor Author

As a tangent, this kind of makes me want to rip out the automatic detection of JSON-like output, and replace it with one or more opt-in headers.

@Zalathar
compiletest: Allow escaped " in //@ normalize-* headers
9275ae5 
This makes it easier to normalize output containing double-quotes, such as JSON
strings.
@Zalathar
compiletest: Don't panic when trying to print unknown JSON after failure
ca4befb 
If we're printing process output after some other check has failed, and the
output is JSON-like, we shouldn't necessarily assume that it is valid JSON
produced by the compiler. If it isn't, printing it as-is is more helpful than
printing a less-relevant error message.
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Zalathar
Copy link
Contributor Author

This is baffling.

If I run the meta test on my machine, everything is as expected.

When the same test runs in CI, it complains that the //@ should-fail revisions don't fail.

But if I remove any of those //@ should-fail headers, then the affected revision fails, as it should.

@Kobzol Kobzol mentioned this pull request Jun 21, 2024
Open
@Zalathar
Copy link
Contributor Author

This also seems to work fine in x86_64-gnu (link), so I have no idea what's going wrong in x86_64-gnu-llvm-17.

@Zalathar
Copy link
Contributor Author

Ah, maybe this is related to --check=pass, and I need to add //@ ignore-pass to the test.

@bors
Copy link
Contributor

bors commented Jun 24, 2024

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

@Oneirical
Copy link
Contributor

r? @jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned wesleywiser Aug 12, 2024
@Zalathar Zalathar marked this pull request as draft August 13, 2024 03:12
@Zalathar Zalathar mentioned this pull request Aug 13, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2024
@bors
Auto merge of rust-lang#129037 - Zalathar:rmake-libtest, r=<try>
6359654 
Port `run-make/libtest-json` and `run-make/libtest-junit` to rmake

Unlike rust-lang#126773, this is just a straightforward port to `rmake`, without attempting to switch to compiletest or get rid of the (trivial) Python scripts.

Part of rust-lang#121876.

r? `@jieyouxu`

try-job: x86_64-msvc
try-job: i686-mingw
try-job: test-various
try-job: aarch64-gnu
try-job: aarch64-apple
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 15, 2024
@matthiaskrgr
Rollup merge of rust-lang#129037 - Zalathar:rmake-libtest, r=jieyouxu
13b3a96 
Port `run-make/libtest-json` and `run-make/libtest-junit` to rmake

Unlike rust-lang#126773, this is just a straightforward port to `rmake`, without attempting to switch to compiletest or get rid of the (trivial) Python scripts.

Part of rust-lang#121876.

r? `@jieyouxu`

try-job: x86_64-msvc
try-job: i686-mingw
try-job: test-various
try-job: aarch64-gnu
try-job: aarch64-apple
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 16, 2024
@workingjubilee
Rollup merge of rust-lang#129037 - Zalathar:rmake-libtest, r=jieyouxu
48af62f 
Port `run-make/libtest-json` and `run-make/libtest-junit` to rmake

Unlike rust-lang#126773, this is just a straightforward port to `rmake`, without attempting to switch to compiletest or get rid of the (trivial) Python scripts.

Part of rust-lang#121876.

r? `@jieyouxu`

try-job: x86_64-msvc
try-job: i686-mingw
try-job: test-various
try-job: aarch64-gnu
try-job: aarch64-apple
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 16, 2024
@workingjubilee
Rollup merge of rust-lang#129037 - Zalathar:rmake-libtest, r=jieyouxu
8fd4899 
Port `run-make/libtest-json` and `run-make/libtest-junit` to rmake

Unlike rust-lang#126773, this is just a straightforward port to `rmake`, without attempting to switch to compiletest or get rid of the (trivial) Python scripts.

Part of rust-lang#121876.

r? ``@jieyouxu``

try-job: x86_64-msvc
try-job: i686-mingw
try-job: test-various
try-job: aarch64-gnu
try-job: aarch64-apple
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 16, 2024
@workingjubilee
Rollup merge of rust-lang#129037 - Zalathar:rmake-libtest, r=jieyouxu
c2b6a7e 
Port `run-make/libtest-json` and `run-make/libtest-junit` to rmake

Unlike rust-lang#126773, this is just a straightforward port to `rmake`, without attempting to switch to compiletest or get rid of the (trivial) Python scripts.

Part of rust-lang#121876.

r? ```@jieyouxu```

try-job: x86_64-msvc
try-job: i686-mingw
try-job: test-various
try-job: aarch64-gnu
try-job: aarch64-apple
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Aug 16, 2024
@workingjubilee
Rollup merge of rust-lang#129037 - Zalathar:rmake-libtest, r=jieyouxu
eb6ecf1 
Port `run-make/libtest-json` and `run-make/libtest-junit` to rmake

Unlike rust-lang#126773, this is just a straightforward port to `rmake`, without attempting to switch to compiletest or get rid of the (trivial) Python scripts.

Part of rust-lang#121876.

r? ````@jieyouxu````

try-job: x86_64-msvc
try-job: i686-mingw
try-job: test-various
try-job: aarch64-gnu
try-job: aarch64-apple
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 16, 2024
@rust-timer
Unrolled build for rust-lang#129037
a22adfc 
Rollup merge of rust-lang#129037 - Zalathar:rmake-libtest, r=jieyouxu

Port `run-make/libtest-json` and `run-make/libtest-junit` to rmake

Unlike rust-lang#126773, this is just a straightforward port to `rmake`, without attempting to switch to compiletest or get rid of the (trivial) Python scripts.

Part of rust-lang#121876.

r? ````@jieyouxu````

try-job: x86_64-msvc
try-job: i686-mingw
try-job: test-various
try-job: aarch64-gnu
try-job: aarch64-apple
@Zalathar
Copy link
Contributor Author

Superseded by #129037.

@Zalathar Zalathar closed this Aug 19, 2024
@Zalathar Zalathar deleted the libtest-json branch August 19, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jieyouxu jieyouxu

Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

compiletest: Printing process output panics if the output contains unknown JSON-like text
7 participants
@Zalathar @rustbot @rust-log-analyzer @bors @Oneirical @wesleywiser @jieyouxu