-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
test: migrate binary_name to snapbox #14041
Conversation
8c8bf63
to
a438457
Compare
// Run cargo build. | ||
p.cargo("build --message-format=json") | ||
.masquerade_as_nightly_cargo(&["different-binary-name"]) | ||
.with_json(output) | ||
.with_stdout_data(str![[r#" | ||
{"executable":"[ROOT]/foo/target/debug/007bar[EXE]","features":[],"filenames":"{...}","fresh":false,"manifest_path":"[ROOT]/foo/Cargo.toml","package_id":"path+[ROOTURL]/foo#0.0.1","profile":"{...}","reason":"compiler-artifact","target":"{...}"} |
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.
This is something worth fixing in snapbox. We need a pretty-printed JSON lines support.
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.
Technically, this is no longer jsonlines. It looks like cargo-test-support had a baked in policy of taking expected
and using double-newlines to delimit pretty-printed json. This is an important distinction because snapbox also supports loading jsonlines from file.
One trick I can look into is we could parse a json array and then convert that to jsonlines.
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.
Yeah I am aware of the double-newlines trick in cargo-test-support. I don't consider it as a blocker for this test file. For test suites like tests/testsuite/message_format.rs
we should consider postponing the migrations.
@bors r+ Checked in with @weihanglo on jsonlines
|
☀️ Test successful - checks-actions |
Update cargo 14 commits in b1feb75d062444e2cee8b3d2aaa95309d65e9ccd..4dcbca118ab7f9ffac4728004c983754bc6a04ff 2024-06-07 20:16:17 +0000 to 2024-06-11 16:27:02 +0000 - Add local registry overlays (rust-lang/cargo#13926) - docs(change): Don't mention non-existent workspace.badges (rust-lang/cargo#14042) - test: migrate binary_name to snapbox (rust-lang/cargo#14041) - Bump to 0.82.0; update changelog (rust-lang/cargo#14040) - tests: Migrate alt_registry to snapbox (rust-lang/cargo#14031) - fix: proc-macro example from dep no longer affects feature resolution (rust-lang/cargo#13892) - chore: Bump cargo-util-schemas to 0.5 (rust-lang/cargo#14038) - chore(deps): update rust crate pulldown-cmark to 0.11.0 (rust-lang/cargo#14037) - fix: remove `__CARGO_GITOXIDE_DISABLE_LIST_FILES` env var (rust-lang/cargo#14036) - chore(deps): update rust crate itertools to 0.13.0 (rust-lang/cargo#13998) - fix(toml): remove `lib.plugin` key support and make it warning (rust-lang/cargo#13902) - chore(deps): update compatible (rust-lang/cargo#13995) - fix: using `--release/debug` and `--profile` together becomes an error (rust-lang/cargo#13971) - fix(toml): Convert warnings that `licence` and `readme` files do not exist into errors (rust-lang/cargo#13921) r? ghost
What does this PR try to resolve?
Part of #14039.
Migrate
tests/testsuite/binary_name.rs
to snapbox.How should we test and review this PR?
Two things need to beware of
[ELAPSED]
for libtest output.JsonLines
doesn't seems to support pretty printed JSON.Additional information