Skip to content

refactor(test): add new APIs for easier snapshot testing #4334

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

Merged
merged 3 commits into from
May 20, 2025

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented May 16, 2025

This PR is the foundation of #4359.

RA-powered inline snapshot updates seem to work without issues in VSCode with the given demo (thanks @roife!):

image

Concerns

  • Could the API design use some improvements (naming, signature, ...)?
  • Should we start to mark the old APIs as deprecated as Cargo once did with theirs?
  • What redactions should we include by default (this_host_triple, tempdir, ...)?
    • This can be added as we go; it does look like this_host_triple is a good inclusion at the start; and I've made it so that each test can add their own redactions.

@rami3l rami3l force-pushed the test/x-better-expect branch 3 times, most recently from 8af6551 to 7ae21c9 Compare May 16, 2025 13:56
@rami3l rami3l changed the title refactor(test): add new testing API for easier snapshot testing refactor(test): add new APIs for easier snapshot testing May 16, 2025
@rami3l rami3l requested review from djc and ChrisDenton May 16, 2025 13:58
@rami3l rami3l force-pushed the test/x-better-expect branch from 7ae21c9 to ea65a34 Compare May 16, 2025 19:03
@ChrisDenton
Copy link
Member

I really like this. Using a more principled testing framework makes a lot of sense rather than maintaining a bespoke solution. And it does seem to be a mostly drop-in replacement but allows for more exact matching (no need for contains) and easier updating of expected output when it's intentionally changed.

It'd be great if we can figure out how to make the old APIs deprecated while still making CI pass but I think this is worth doing even if that proves challenging.

I don't have a strong opinion on naming. Maybe "redactions" could be "replacements"? Because it replaces the matched string.

@rami3l rami3l force-pushed the test/x-better-expect branch 3 times, most recently from b23b07f to 5ae4ce8 Compare May 17, 2025 11:31
@rami3l
Copy link
Member Author

rami3l commented May 17, 2025

@ChrisDenton Many thanks for the kind words! Redaction is snapbox terminology so I'll probably stick with that...

My plan is that we can merge this as-is first (with #![allow(deprecated)]), and then move on with the migration one bit at a time.

@rami3l rami3l force-pushed the test/x-better-expect branch from 5ae4ce8 to 254f543 Compare May 17, 2025 14:43
@rami3l rami3l marked this pull request as ready for review May 17, 2025 14:43
@rami3l rami3l force-pushed the test/x-better-expect branch 2 times, most recently from 080f555 to 85ae90e Compare May 17, 2025 14:48
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Looks like a good direction!

@rami3l rami3l force-pushed the test/x-better-expect branch from 85ae90e to 1481e5d Compare May 19, 2025 13:31
@rami3l rami3l added this pull request to the merge queue May 20, 2025
Merged via the queue into rust-lang:master with commit c5bc336 May 20, 2025
29 checks passed
@rami3l rami3l deleted the test/x-better-expect branch May 20, 2025 15:11
FranciscoTGouveia added a commit to FranciscoTGouveia/rustup that referenced this pull request May 26, 2025
This change is done to be consistent with the new migration to the
`.expect()` API's that were defined in rust-lang#4334 and rust-lang#4342, and are
showcased in the latest PR for migration (rust-lang#4343).
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.

3 participants