Skip to content

make snapshot test more permissive#13251

Closed
Gankra wants to merge 1 commit intomainfrom
gankra/loose-test
Closed

make snapshot test more permissive#13251
Gankra wants to merge 1 commit intomainfrom
gankra/loose-test

Conversation

@Gankra
Copy link
Copy Markdown
Contributor

@Gankra Gankra commented May 1, 2025

fixes #13212

@Gankra Gankra added the internal A refactor or improvement that is not user-facing label May 1, 2025
r#""commits_since_last_tag": .*"#,
r#""commits_since_last_tag": [COUNT]"#,
),
// This last filter normalizes output for tarball builds of uv that lack commit info
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given this test won't really change a lot, should we just check if commit info is available and have two snapshots? Using filters this way makes me a bit uncomfortable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oh interesting, i never considered conditional snapshots! that's a great idea

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

They're just annoying during snapshot updates, so I try to avoid them in most cases.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but if the filter is replacing basically the entire expected output, I'd either just skip the test or have a conditional.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I've filed #13566 doing that.

@mgorny
Copy link
Copy Markdown
Contributor

mgorny commented May 16, 2025

Gentle ping. Can I do anything to help move this or any other solution forward?

mgorny added a commit to mgorny/uv that referenced this pull request May 21, 2025
Fix the two version json tests to account for the possibility that uv
was built outside a git checkout (e.g. from an unpacked git archive)
and therefore does not have the commit info available.  This approach
uses separate snapshots for the two cases, as suggested in discussion
of pull request astral-sh#13251.

Fixes astral-sh#13212
Gankra added a commit that referenced this pull request May 21, 2025
## Summary

Fix the two version json tests to account for the possibility that uv
was built outside a git checkout (e.g. from an unpacked git archive) and
therefore does not have the commit info available. This approach uses
separate snapshots for the two cases, as suggested in discussion of pull
request #13251.

Fixes #13212

## Test Plan

1. `cargo test` in a git clone.
2. `cargo clean`, moved `.git` away, `cargo test` again.

---------

Co-authored-by: Aria Desires <aria.desires@gmail.com>
@Gankra Gankra closed this Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal A refactor or improvement that is not user-facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

version::self_version_json and version::version_get_fallback_unmanaged_json test failures (outside git checkout?)

3 participants