Make some blocking tests non-blocking

[hi-rustin]

Signed-off-by: hi-rustin rustin.liu@gmail.com

What does this PR try to resolve?

close #11615

Make some blocking tests non-blocking.

Check the unit tests.

Additional information

None

r? @ehuss

[hi-rustin]

Use -Zpublish-timeout and .cargo/config to disable publish timeout for some tests.

@ehuss Do you think this is reasonable?

[ehuss]

Just checking in @hi-rustin if you had any questions. I think enabling the API for publish is probably the right way to go.

[hi-rustin]

I think enabling the API for publish is probably the right way to go.

Yeah, I am going to fix some tests that do not need to check the content first. Then I will make the HTTP mock server support assert its content to support other tests.

[hi-rustin]

@ehuss @epage Do you mind taking another look at this PR first? I need more time to support checking the content of the HTTP mock server. So maybe we can review and merge this PR first?

[ehuss]

I need more time to support checking the content of the HTTP mock server. So maybe we can review and merge this PR first?

I would prefer to not remove test coverage just to merge a PR sooner.

For the tests that want to validate the JSON of the API request, I would suggest doing the following:

Factor out the code from _validate_upload so that it works with a &[u8] instead of a File.

In tests, add a responder to the registry builder:

.add_responder("/api/v1/crates/new", 
    validate_api_publish(
        r#"{
            "authors": [],
            ...
        }"#,
        "foo-0.0.1.crate",
        &["Cargo.lock", "Cargo.toml", "Cargo.toml.orig", "src/main.rs"],
    ))

The validate_api_publish function should return a function:

fn validate_publish(...) -> impl Fn(&Request, &HttpServer) -> Response {
    |request, server| -> Response {
        // code here should take request.body and feed that into _validate_upload
        // and return the appropriate response 
        server.ok(request)
    }
}

Let me know if you have any questions or thoughts on that approach.

[hi-rustin]

I would prefer to not remove test coverage just to merge a PR sooner.

Sorry I wasn't clear, my current changes are for tests that don't require checking content. Because they don't have any specificity in their content. We can just check it via stdout. So I think these changes sould not change any test coverages.

But if you prefer, we can include any changes in this PR. That's fine with me.

[hi-rustin]

Let me know if you have any questions or thoughts on that approach.

Thanks a lot! I'll try it.

[ehuss]

@rustbot author

[hi-rustin]

@ehuss I added validate_api_publish and validate_api_publish_with_contents to support checking the HTTP request for a crate publish.

But I found if we use normalize_expected in another thread it will panic. Because we try to normalize TEST_ID, and it is a thread-local variable, it causes panic.

Do you have any suggestions about how to fix it?

[ehuss]

Oh, that's unfortunate! Ok, new strategy: In check_authorized_publish, save the contents of req.body into api_path().join("v1/crates/new"). No need to add a custom responder, or modify validate_upload. That should significantly reduce the number of changes needed here. Does that sound like it should work?

[hi-rustin]

Ok, new strategy: In check_authorized_publish, save the contents of req.body into api_path().join("v1/crates/new"). No need to add a custom responder, or modify validate_upload. That should significantly reduce the number of changes needed here. Does that sound like it should work?

Yes. I think it would work. I'll try it tomorrow. Thanks!

[hi-rustin]

@rustbot ready

[hi-rustin]

For CI failed, see more at zulip.

[epage]

I rebased this on top of #11738 so I could use cargo nextests test-time reporting and I am not seeing any slow registery tests, so I believe that confirms the issue is resolved with this change.

Of note, the lto tests are very slow, taking 17-35s each

@bors r+

[bors]

📌 Commit fbe7ac2 has been approved by epage

It is now in the queue for this repository.

[bors]

⌛ Testing commit fbe7ac2 with merge 65cab34...

[hi-rustin]

Thanks for your review! 💚 💙 💜 💛 ❤️

[ehuss]

Can you say why this validation was removed? I would not expect it to be removed.

[epage]

sigh I was specifically looking for any still removed and missed this. Thanks for spotting it!

[ehuss]

@hi-rustin Can you post a PR to return the publish_to_alt_registry test so that it validates the upload?

[bors]

☀️ Test successful - checks-actions
Approved by: epage
Pushing 65cab34 to master...

[bors]

☀️ Test successful - checks-actions
Approved by: epage
Pushing 65cab34 to master...

[bors]

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

[ehuss]

@hi-rustin Were your concerns about the order of the output in inherit_dependencies resolved? I'm concerned that this is introducing non-deterministic behavior. Do you know why the order was different? Is it still having issues on your machine? Do you know what was behind that?

[hi-rustin]

Were your concerns about the order of the output in inherit_dependencies resolved?

I didn't meet it anymore after I updated the output and rebased from the master branch.

Do you know why the order was different? Is it still having issues on your machine?

It's always been stable on my computer. It's only in CI that I've encountered instability. But the past few tests have passed stably. I'm not quite sure what exactly is affecting it. Maybe there was a mistake in my code at the time. I'll keep watching. I'll keep trying to fix it if there's a problem.