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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented May 16, 2025

Borrowing from Cargo's previous effort, this marks the beginning of a new testing DSL for better overall dev experience.

RA-powered inline snapshot updates seems to work without issues in VSCode with the given demo:

image

I have a more ambitious goal of removing dependency injections altogether, but we must start from somewhere, so here it is.

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 from 254f543 to 080f555 Compare May 17, 2025 14:44
@rami3l rami3l force-pushed the test/x-better-expect branch 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!

@@ -1,3 +1,4 @@
#![allow(deprecated)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: move these below the module-level docstrings, please?

@@ -21,6 +21,7 @@ use rustup::test::{
use rustup::test::{RegistryGuard, RegistryValueId, USER_PATH};
use rustup::utils::{self, raw};
use rustup::{DUP_TOOLS, TOOLS, for_host};
use snapbox::str;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love this because I usually expect str to be std::str. Maybe alias this or just use snapbox::str!() in callers?

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