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

refactor(tests): Make publish tests work with wait-for-publish #11217

Closed
wants to merge 3 commits into from

Conversation

epage
Copy link
Contributor

@epage epage commented Oct 12, 2022

In #11062, cargo publish will start blocking until the package is in the index.

This PR includes:

Switches some tests from using the local registry to the remote registry. The local registry translates PUT calls to writing to the file system and we have no where to inject code to move that into the index. The remote registry was updated in #11111 to handle this.

Updates some tests to directly inject a no-op package into the registry before publish so that when we merge #11062, the blocking code will find that no-op package and be satisfied. This is an ugly hack that we were trying to avoid (hence #11111 and the above change). The problem comes with trying to verify we hit the end point with all of the right data. Alternatively, we could have either In some cases, I removed the validation of the PUT because it didn't look like that was a focus of the test case; they just cared that a publish happens

  • Parsed the index and checked the Summaries and the Crates to ensure the data matched what we expected but that requires the Summaries to always be 100% the same as the PUT call (which they are, for now), could likely hit cargo caching issues much like the implementation of fix(publish): Block until it is in index #11062 did
  • Allowed the remote test registry to accept a list of expected PUT requests and verify them. This would be fairly clean but was going to be a bit of work to setup all of that infrastructure and tweaking to make sure we get all of the error reporting just right.

In the end, I went the more surgical route rather continuing to let this get bogged down in test changes and conflicts.

This was verified by rebasing the blocking branch on this and now all tests pass and I don't see signs of 60 second tests.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 12, 2022
@@ -94,6 +94,16 @@ fn warn_both_token_and_process() {
.file("src/lib.rs", "")
.build();

// HACK: Inject `foo` directly into the index so `publish` won't block for it to be in
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about disabling the blocking for these tests instead? Can we set publish.timeout = 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or if we had a --no-wait option to cargo publish that could be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for calling that out; I had forgotten to include it in my list.

I had considered it but it would require "nightly" until #11222 is stablized and was wanting to keep the tests focused on "stable" for now.

I had also considered making the default be determined by the source so a local registry could always specify 0 but that was going to require plumbing a fairly specific use case through several traits and I was hesitant on whether that was worth it.

@epage epage force-pushed the pub2 branch 2 times, most recently from aa2c702 to d103edc Compare October 13, 2022 21:17
@ehuss
Copy link
Contributor

ehuss commented Nov 9, 2022

Am I correct that this can be closed, as this was included with #11062?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants