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: deal with lockfile checksums dynamically #13739

Closed
wants to merge 1 commit into from

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Apr 11, 2024

When generating local registries in the testsuite, the specific package
checksums may vary from the original simply due to different zlib
versions. For example, Fedora 40 is now using zlib-ng-compat. We can
account for this dynamically by either formatting the checksum in the
expected output string, or using generate-lockfile on projects and
their expected output.

When generating local registries in the testsuite, the specific package
checksums may vary from the original simply due to different `zlib`
versions. For example, Fedora 40 is now using `zlib-ng-compat`. We can
account for this dynamically by either formatting the checksum in the
expected output string, or using `generate-lockfile` on projects and
their expected output.
@rustbot
Copy link
Collaborator

rustbot commented Apr 11, 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 Apr 11, 2024
@@ -33,5 +35,7 @@ fn case() {
.stdout_matches(str![""])
.stderr_matches(file!["stderr.term.svg"]);

assert_ui().subset_matches(current_dir!().join("out"), &project_root);
let expected = Project::from_expected(current_dir!().join("out"));
Copy link
Member

Choose a reason for hiding this comment

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

The dynamic check seems fine to me, though I am a bit unsure since most UI tests in Cargo only asserts one command invocation.

@epage, do you have any input for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Dynamically generating the snapshots like this runs counter to point of the snapshots

  • We are bringing code under test into the process for verifying the code under tests
  • This is brittle as a generate-lockfile happens to work right now for these specific tests but that likely won't be true always.

A good example of where this can fall flat in both cases is my on-going work with MSRV-aware resolver.

Something I've been considering is adding support for dynamic substitutions so a function could run and replace all checksums with a [CHECKSUM].

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that it's not just about expected output in the tests. In cargo_add::locked_unchanged, cargo itself complains that the input lockfile looks corrupt with its mismatched checksum.

@cuviper
Copy link
Member Author

cuviper commented Apr 11, 2024

Alternatively, I think setting Compression::none() in the cargo-test-support registry will sufficiently avoid zlib differences. At least, it appears to be consistent at first glance, but I'll do more testing and send a new PR if that works out.

@cuviper
Copy link
Member Author

cuviper commented Apr 11, 2024

Closing in favor of #13744.

@cuviper cuviper closed this Apr 11, 2024
@bors
Copy link
Contributor

bors commented Apr 11, 2024

☔ The latest upstream changes (presumably #13744) made this pull request unmergeable. Please resolve the merge conflicts.

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-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants