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.

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: Auto-redact ... after last build at ...; Migrate freshness to Snapbox #14161

Merged

Conversation

choznerol
Copy link
Contributor

What does this PR try to resolve?

Part of #14039.

Migrate tests/testsuite/freshness.rs to snapbox.

How should we test and review this PR?

I followed #14039. The #![allow(deprecated)] was removed and tests/testsuite/freshness.rs is still passing.

Additional information

The redaction for "dirty reason" was a bit tricky

write!(f, "{new_time}, {diff} after {what} at {old_time}")

Current implementation would redact

  • 1719325877.527949100s, 61549498ns after last build at 1719325877.466399602s
  • 1719503592.218193216s, 1h 1s after last build at 1719499991.982681034s

into [DIRTY_REASON_NEW_TIME], [DIRTY_REASON_DIFF] after last build at [DIRTY_REASON_OLD_TIME]

Please let me know if that seems right.

@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2024

r? @epage

rustbot has assigned @epage.
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-testing-cargo-itself Area: cargo's tests S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2024
@choznerol choznerol force-pushed the issue-14039-snapshot-port-freshness branch from 4e3688f to 3f80722 Compare June 27, 2024 16:12
@choznerol choznerol changed the title Issue 14039 snapshot port freshness test: Migrate freshness to Snapbox Jun 27, 2024
tests/testsuite/freshness.rs Outdated Show resolved Hide resolved
tests/testsuite/freshness.rs Outdated Show resolved Hide resolved
Comment on lines +204 to +207
// Following 3 subs redact:
// "1719325877.527949100s, 61549498ns after last build at 1719325877.466399602s"
// "1719503592.218193216s, 1h 1s after last build at 1719499991.982681034s"
// into "[DIRTY_REASON_NEW_TIME], [DIRTY_REASON_DIFF] after last build at [DIRTY_REASON_OLD_TIME]"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weihanglo what are your thoughts on these redactions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we instead reduct everything in () that contains "after last build"?

Copy link
Contributor Author

@choznerol choznerol Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That turns out to make it much simpler 😅
Will squash 75f6f5f and 14cbe9e into previous commits later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with the current redaction. Would could remove/change it when needed.

@choznerol choznerol changed the title test: Migrate freshness to Snapbox test: Auto-redact ... after last build a ...; Migrate freshness to Snapbox Jun 29, 2024
@choznerol choznerol changed the title test: Auto-redact ... after last build a ...; Migrate freshness to Snapbox test: Auto-redact ... after last build at ...; Migrate freshness to Snapbox Jun 29, 2024
tests/testsuite/freshness.rs Outdated Show resolved Hide resolved
tests/testsuite/freshness.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Jun 29, 2024

Could you squash your "fixup" commits into their appropriate commit?

@choznerol choznerol force-pushed the issue-14039-snapshot-port-freshness branch from cbf4b1b to 56ca35f Compare June 29, 2024 14:56
@choznerol choznerol marked this pull request as ready for review June 29, 2024 14:57
Comment on lines 1615 to 1631
.with_stderr_data(
str![[r#"
[LOCKING] 3 packages to latest compatible versions
[COMPILING] bar [..]
[RUNNING] `rustc --crate-name bar --edition=2015 bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]
[RUNNING] `rustc --crate-name bar --edition=2015 bar/src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C debuginfo=2 [..]
[COMPILING] somepm [..]
[RUNNING] `rustc --crate-name somepm [..]
[COMPILING] foo [..]
[RUNNING] `rustc --crate-name foo --edition=2015 src/lib.rs [..]-C panic=abort[..]
[FINISHED] [..]
",
[COMPILING] bar v0.0.1 ([ROOT]/foo/bar)
[RUNNING] `rustc --crate-name bar [..] bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]
[RUNNING] `rustc --crate-name bar [..] bar/src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C debuginfo=2 [..]
[COMPILING] somepm v0.0.1 ([ROOT]/foo/somepm)
[RUNNING] `rustc --crate-name somepm [..]`
[COMPILING] foo v0.0.1 ([ROOT]/foo)
[RUNNING] `rustc --crate-name foo [..]-C panic=abort[..]`
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]]
Copy link
Contributor Author

@choznerol choznerol Jun 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to fix this reuse_panic_pm CI failure, which passed on Tests on macOS aarch64 stable but failed on Test on macOS x86_64 stable with below:

---- expected: tests/testsuite/freshness.rs:1616:13
++++ actual:   stderr
   1    1 | [LOCKING] 3 packages to latest compatible versions
   2    2 | [COMPILING] bar v0.0.1 ([ROOT]/foo/bar)
   3    3 | [RUNNING] `rustc --crate-name bar [..] bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]
-  4      - [RUNNING] `rustc --crate-name bar [..] bar/src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C debuginfo=2 [..]
   5    4 | [COMPILING] somepm v0.0.1 ([ROOT]/foo/somepm)
   6    5 | [RUNNING] `rustc --crate-name somepm [..]`
   7    6 | [COMPILING] foo v0.0.1 ([ROOT]/foo)
   8    7 | [RUNNING] `rustc --crate-name foo [..]-C panic=abort[..]`
   9    8 | [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+       9 + [RUNNING] `rustc --crate-name bar --edition=2015 bar/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values())' -C metadata=9229e8e86d572a0d -C extra-filename=-9229e8e86d572a0d --out-dir [ROOT]/foo/target/debug/deps -L dependency=[ROOT]/foo/target/debug/deps`

Some thought & findings so far:

Since the test logic was unchanged, and it passed on Test on macOS x86_64 stable in next rerun (CI), I think the arch difference can be ruled out first.

Me next suspect is flakiness, which leads me to this with_stderr_unordered caveat from its nice comment:

/// Be careful when using patterns such as `[..]`, because you may end up
/// with multiple lines that might match, and this is not smart enough to
/// do anything like longest-match. For example, avoid something like:
///
/// ```text
///  [RUNNING] `rustc [..]
///  [RUNNING] `rustc --crate-name foo [..]
/// ```
///
/// This will randomly fail if the other crate name is `bar`, and the
/// order changes.

The original L1531-L1532 seems to already match this scenario (A line matching L1532 could also match L1531). However, I assume reuse_panic_pm has been stable until this something introduced in my PR.

According to the comment form test:

bar is built once without panic (for proc-macro) and once with (for the normal dependency).

If the order of two bar build is non-deterministic this could be a problem; In contrast, if the order is deterministic, that makes me wonder why using unordered in the first place.

In fact, I removed .unordered, reran and passed reuse_panic_pm a few times locally, so let my try this first 👉 a453efd

I also tried to reproduce any randomness introduce by Snapbox unordered,

but no finding:

use snapbox::prelude::*;
use snapbox::str;
use snapbox::assert_data_eq;

#[cargo_test]
fn test_snapbox_unorder_randomness() {
    let actual = str![[r#"
rustc --crate-name foo --a=b --ccc=ddd
rustc --crate-name foo --a=b --panic=abort --ccc=ddd
eee fff
"#]]
    .unordered();
    let expected = str![[r#"
rustc --crate-name foo [..]
rustc --crate-name foo [..] --panic=abort [..]
eee fff
"#]]
    .unordered();
    assert_data_eq!(actual, expected); // Always pass. Not flaky.
}

P.s. For reference, the full output from my local runs are like:

[LOCKING] 3 packages to latest compatible versions
[COMPILING] bar v0.0.1 ([ROOT]/foo/bar)
[RUNNING] `rustc --crate-name bar --edition=2015 bar/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values())' -C metadata=3eb56625d2059feb -C extra-filename=-3eb56625d2059feb --out-dir [ROOT]/foo/target/debug/deps -L dependency=[ROOT]/foo/target/debug/deps`
[RUNNING] `rustc --crate-name bar --edition=2015 bar/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C panic=abort -C embed-bitcode=no -C debuginfo=2 -C split-debuginfo=unpacked --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values())' -C metadata=e34f62847e3b5fb0 -C extra-filename=-e34f62847e3b5fb0 --out-dir [ROOT]/foo/target/debug/deps -L dependency=[ROOT]/foo/target/debug/deps`
[COMPILING] somepm v0.0.1 ([ROOT]/foo/somepm)
[RUNNING] `rustc --crate-name somepm --edition=2015 somepm/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type proc-macro --emit=dep-info,link -C prefer-dynamic -C embed-bitcode=no --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values())' -C metadata=a7ef9925b27e01b6 -C extra-filename=-a7ef9925b27e01b6 --out-dir [ROOT]/foo/target/debug/deps -L dependency=[ROOT]/foo/target/debug/deps --extern bar=[ROOT]/foo/target/debug/deps/libbar-[HASH].rlib --extern proc_macro`
[COMPILING] foo v0.0.1 ([ROOT]/foo)
[RUNNING] `rustc --crate-name foo --edition=2015 src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C panic=abort -C embed-bitcode=no -C debuginfo=2 -C split-debuginfo=unpacked --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values())' -C metadata=ebe9fb0a09bbfc5f -C extra-filename=-ebe9fb0a09bbfc5f --out-dir [ROOT]/foo/target/debug/deps -L dependency=[ROOT]/foo/target/debug/deps --extern bar=[ROOT]/foo/target/debug/deps/libbar-[HASH].rmeta --extern somepm=[ROOT]/foo/target/debug/deps/libsomepm-[HASH].dylib`
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

I guess some of my questions are:

  • Just in case, was there any similar reuse_panic_pm flakiness you recall seeing prior to this PR?
  • Does reuse_panic_pm really need with_stderr_unordered or .unordered()?
  • Anything else I should look into?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: a453efd CI passes reuse_panic_pm on all stable pipeline: linux, mac x86, mac aarch64, Windows

Copy link
Contributor Author

@choznerol choznerol Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: The same error happened again after a453efd on Tests Windows x86_64 gnu nightly, so a453efd didn't fix the problem 😞

CI

https://github.com/rust-lang/cargo/actions/runs/9747539766/job/26900484318#step:11:4053

thread 'freshness::reuse_panic_pm' panicked at tests\testsuite\freshness.rs:1608:7:

---- expected: tests\testsuite\freshness.rs:1597:42
++++ actual:   stderr
   1    1 | [LOCKING] 3 packages to latest compatible versions
   2    2 | [COMPILING] bar v0.0.1 ([ROOT]/foo/bar)
   3    3 | [RUNNING] `rustc --crate-name bar [..] bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]
-   4      - [RUNNING] `rustc --crate-name bar [..] bar/src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C debuginfo=2 [..]
+        4 + [RUNNING] `rustc --crate-name bar --edition=2015 bar/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no --check-cfg cfg(docsrs) --check-cfg "cfg(feature, values())" -C metadata=7b256d6ddbd506b7 -C extra-filename=-7b256d6ddbd506b7 --out-dir [ROOT]/foo/target/debug/deps -L dependency=[ROOT]/foo/target/debug/deps`
   5    5 | [COMPILING] somepm v0.0.1 ([ROOT]/foo/somepm)
   6    6 | [RUNNING] `rustc --crate-name somepm [..]`
   7    7 | [COMPILING] foo v0.0.1 ([ROOT]/foo)
   8    8 | [RUNNING] `rustc --crate-name foo [..]-C panic=abort[..]`
   9    9 | [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

https://github.com/rust-lang/cargo/actions/runs/9781022885/job/27004092959?pr=14161

---- expected: tests/testsuite/freshness.rs:1620:13
++++ actual:   stderr
   1    1 | [LOCKING] 3 packages to latest compatible versions
   2    2 | [COMPILING] bar v0.0.1 ([ROOT]/foo/bar)
   3    3 | [RUNNING] `rustc --crate-name bar [..] bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]
-   4      - [RUNNING] `rustc --crate-name bar [..] bar/src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C debuginfo=2 [..]
   5    4 | [COMPILING] somepm v0.0.1 ([ROOT]/foo/somepm)
   6    5 | [RUNNING] `rustc --crate-name somepm [..]`
   7    6 | [COMPILING] foo v0.0.1 ([ROOT]/foo)
   8    7 | [RUNNING] `rustc --crate-name foo [..]-C panic=abort[..]`
   9    8 | [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
+        9 + [RUNNING] `rustc --crate-name bar --edition=2015 bar/src/lib.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --crate-type lib --emit=dep-info,metadata,link -C embed-bitcode=no --check-cfg 'cfg(docsrs)' --check-cfg 'cfg(feature, values())' -C metadata=9229e8e86d572a0d -C extra-filename=-9229e8e86d572a0d --out-dir [ROOT]/foo/target/debug/deps -L dependency=[ROOT]/foo/target/debug/deps`

This seems like a flaky error either already exist or somehow related to my change on reuse_panic_pm.

Dropping a453efd and revert my change to reuse_panic_pm for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need a deeper investigation of it. Fine with holding off for now.

@choznerol choznerol force-pushed the issue-14039-snapshot-port-freshness branch from c826531 to 133ea7e Compare July 3, 2024 15:50
@choznerol choznerol force-pushed the issue-14039-snapshot-port-freshness branch from 133ea7e to b8a62da Compare July 3, 2024 16:11
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not block this. Thank you!

Comment on lines +204 to +207
// Following 3 subs redact:
// "1719325877.527949100s, 61549498ns after last build at 1719325877.466399602s"
// "1719503592.218193216s, 1h 1s after last build at 1719499991.982681034s"
// into "[DIRTY_REASON_NEW_TIME], [DIRTY_REASON_DIFF] after last build at [DIRTY_REASON_OLD_TIME]"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with the current redaction. Would could remove/change it when needed.

Comment on lines 1615 to 1631
.with_stderr_data(
str![[r#"
[LOCKING] 3 packages to latest compatible versions
[COMPILING] bar [..]
[RUNNING] `rustc --crate-name bar --edition=2015 bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]
[RUNNING] `rustc --crate-name bar --edition=2015 bar/src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C debuginfo=2 [..]
[COMPILING] somepm [..]
[RUNNING] `rustc --crate-name somepm [..]
[COMPILING] foo [..]
[RUNNING] `rustc --crate-name foo --edition=2015 src/lib.rs [..]-C panic=abort[..]
[FINISHED] [..]
",
[COMPILING] bar v0.0.1 ([ROOT]/foo/bar)
[RUNNING] `rustc --crate-name bar [..] bar/src/lib.rs [..]--crate-type lib --emit=[..]link[..]
[RUNNING] `rustc --crate-name bar [..] bar/src/lib.rs [..]--crate-type lib --emit=[..]link -C panic=abort[..]-C debuginfo=2 [..]
[COMPILING] somepm v0.0.1 ([ROOT]/foo/somepm)
[RUNNING] `rustc --crate-name somepm [..]`
[COMPILING] foo v0.0.1 ([ROOT]/foo)
[RUNNING] `rustc --crate-name foo [..]-C panic=abort[..]`
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s

"#]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might need a deeper investigation of it. Fine with holding off for now.

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 3, 2024

📌 Commit b8a62da has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2024
@bors
Copy link
Collaborator

bors commented Jul 3, 2024

⌛ Testing commit b8a62da with merge 753ae28...

@bors
Copy link
Collaborator

bors commented Jul 3, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 753ae28 to master...

@bors bors merged commit 753ae28 into rust-lang:master Jul 3, 2024
22 checks passed
@choznerol choznerol deleted the issue-14039-snapshot-port-freshness branch July 4, 2024 02:20
choznerol added a commit to choznerol/cargo that referenced this pull request Jul 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 5, 2024
Update cargo

13 commits in a515d463427b3912ec0365d106791f88c1c14e1b..f86ce56396168edf5d0e1c412ddda0b2edba7d46
2024-07-02 20:53:36 +0000 to 2024-07-05 17:52:05 +0000
- test: Migrate jobserver to snapbox (rust-lang/cargo#14191)
- chore(deps): update msrv (3 versions) to v1.77 (rust-lang/cargo#14186)
- test: migrate build_plan and build_script to snapbox (rust-lang/cargo#14193)
- test: migrate cfg and check to snapbox (rust-lang/cargo#14185)
- test: migrate install* and inheritable_workspace_fields to snapbox (rust-lang/cargo#14170)
- Pass rustflags to artifacts built with implicit targets when using target-applies-to-host (rust-lang/cargo#13900)
- test: Migrate network tests to snapbox (rust-lang/cargo#14187)
- test: migrate some files to snapbox (rust-lang/cargo#14113)
- test: Auto-redact `... after last build at ...`; Migrate `freshness` to Snapbox (rust-lang/cargo#14161)
- chore: fix some typos (rust-lang/cargo#14182)
- fix: improve message for inactive weak optional feature with edition2024 through unused dep collection (rust-lang/cargo#14026)
- test:migrate `doc/directory/docscrape` to snapbox (rust-lang/cargo#14171)
- test: Migrate git_auth to snapbox (rust-lang/cargo#14172)
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 7, 2024
Update cargo

20 commits in a515d463427b3912ec0365d106791f88c1c14e1b..154fdac39ae9629954e19e9986fd2cf2cdd8d964
2024-07-02 20:53:36 +0000 to 2024-07-07 01:28:23 +0000
- test: relax redactions for rust-lang/rust (rust-lang/cargo#14203)
- use "bootstrap" instead of "rustbuild" (rust-lang/cargo#14207)
- test: migrate serveral files to snapbox (rust-lang/cargo#14180)
- Add rustdocflags to Unit's Debug impl (rust-lang/cargo#14201)
- Allow enabling `config-include` feature in config (rust-lang/cargo#14196)
- fix(test): Restore `does_not_contain` for check (rust-lang/cargo#14198)
- test: migrate patch, pkgid, proc_macro and progress to snapbox (rust-lang/cargo#14181)
- test: Migrate jobserver to snapbox (rust-lang/cargo#14191)
- chore(deps): update msrv (3 versions) to v1.77 (rust-lang/cargo#14186)
- test: migrate build_plan and build_script to snapbox (rust-lang/cargo#14193)
- test: migrate cfg and check to snapbox (rust-lang/cargo#14185)
- test: migrate install* and inheritable_workspace_fields to snapbox (rust-lang/cargo#14170)
- Pass rustflags to artifacts built with implicit targets when using target-applies-to-host (rust-lang/cargo#13900)
- test: Migrate network tests to snapbox (rust-lang/cargo#14187)
- test: migrate some files to snapbox (rust-lang/cargo#14113)
- test: Auto-redact `... after last build at ...`; Migrate `freshness` to Snapbox (rust-lang/cargo#14161)
- chore: fix some typos (rust-lang/cargo#14182)
- fix: improve message for inactive weak optional feature with edition2024 through unused dep collection (rust-lang/cargo#14026)
- test:migrate `doc/directory/docscrape` to snapbox (rust-lang/cargo#14171)
- test: Migrate git_auth to snapbox (rust-lang/cargo#14172)
@rustbot rustbot added this to the 1.81.0 milestone Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing-cargo-itself Area: cargo's tests S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants