Skip to content

Conversation

@ada4a
Copy link
Contributor

@ada4a ada4a commented Nov 1, 2025

TODO (check if already done)

  • Add tests
    • .. but wasn't able to bless them as the test suite seems to hang for some reason
  • Add CHANGELOG.md entry
  • Bumped minor version and committed lockfiles in case a release is desired

@oli-obk
Copy link
Owner

oli-obk commented Nov 1, 2025

hmm I'll have to try the test suite locally, but are you sure it hangs and doesn't just take forever (it rebuilds a lot of things on the first run)

@ada4a
Copy link
Contributor Author

ada4a commented Nov 1, 2025

Thanks for starting CI:)

are you sure it hangs and doesn't just take forever (it rebuilds a lot of things on the first run)

I mean my laptop is admittedly quite slow, but AFAICT it isn't the rebuilding part that fails, just the integration test in particular:

     Running tests/integration.rs (target/debug/deps/integration-a917ca08f79bf777)
tests/integrations/basic/Cargo.toml ⠤
tests/integrations/basic-bin/Cargo.toml ⠤
tests/integrations/basic-fail-mode/Cargo.toml ⠠
tests/integrations/cargo-run/Cargo.toml ⠠
tests/integrations/ui_test_dep_bug/Cargo.toml ⠠
tests/integrations/basic-fail/Cargo.toml ⠠
tests/integrations/dep-fail/Cargo.toml ⠠ 

@ada4a
Copy link
Contributor Author

ada4a commented Nov 1, 2025

Oh, it did move after a while! The thing is that it completely maxed out all the CPUs, which is why I assumed that it got into an infinite loop somewhere

EDIT: Okay, yeah, the following runs are much smoother

@oli-obk
Copy link
Owner

oli-obk commented Nov 1, 2025

Yea it rebuilds the crate for each of those tests

@ada4a ada4a force-pushed the single_diagnostic branch 2 times, most recently from fe024ef to 50a8838 Compare November 1, 2025 11:47
@ada4a
Copy link
Contributor Author

ada4a commented Nov 1, 2025

I also got a bunch of seemingly unrelated changes when running cargo test -- -- --bless; I removed them, I think the committed changes should be the correct ones

EDIT: oh no, apparently some unrelated changes did end up sneaking into Cargo.stdout

@oli-obk
Copy link
Owner

oli-obk commented Nov 1, 2025

Maybe I need to do a version bump again of the rust version we use, let's see what CI says

@ada4a
Copy link
Contributor Author

ada4a commented Nov 1, 2025

That does sound right -- many of the changes seem to come from improved rustc diagnostics

EDIT: Ahh, right! That's because I have Cargo installed as a free-standing program (so not from rustup), which is why rust-toolchain.toml isn't respected and I end up running the latest stable instead of 1.80.1. Apologies for the confusion

@ada4a ada4a force-pushed the single_diagnostic branch from 50a8838 to f2fd3a3 Compare November 1, 2025 12:08
@oli-obk oli-obk merged commit ac73fd0 into oli-obk:main Nov 1, 2025
8 checks passed
@oli-obk
Copy link
Owner

oli-obk commented Nov 1, 2025

Great! Thanks for the fix

@ada4a
Copy link
Contributor Author

ada4a commented Nov 1, 2025

Sorry, I'd realized that my test case doesn't actually test what it was supposed to test -- I wanted to make it emit there were 2 unmatched diagnostics, but for some reason, it emits there was 1 unmatched diagnostic twice instead. Would you happen to know why that would be?.. I'd then open a follow-up PR to fix up the test

@oli-obk
Copy link
Owner

oli-obk commented Nov 1, 2025

Hmm I saw the test changes for 1 unmatched diagnostic and forgot to check that there's still a case for 2+


error: actual output differed from expected
Execute `DO NOT BLESS. These are meant to fail` to update `tests/actual_tests/bad_pattern_multiple.stderr` to the actual output
--- tests/actual_tests/bad_pattern_multiple.stderr
Copy link
Owner

Choose a reason for hiding this comment

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

Could also bless the output so we only see the missing annotation error

error: there was 1 unmatched diagnostic
--> tests/actual_tests/bad_pattern_multiple.rs:4:9
|
4 | add("42", 3);
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe there need to be multiple per line for there to be just one diagnostic informing us about multiple cases

Copy link
Owner

Choose a reason for hiding this comment

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

Tho I think it maybe best if we had one big diagnostic for all unmatched diagnostics,

@ada4a ada4a deleted the single_diagnostic branch November 1, 2025 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants