Test improvements, conversions to using the new API #1546
Conversation
tests/all.rs is big enough. Also add documentation.
This test, which logs in, logs out, then tries to publish a crate with an invalid token, isn't actually testing useful logic because the cookie logic for the tests is injected into the test app. Actual browser cookie logic isn't exercised in these tests; at best, this test is testing the tests. The other parts of this test function are fine.
Add a method to add a dependency on PublishBuilder
Renaming to better describe what's being tested. Added another method to PublishBuilder for adding a renamed dependency.
This gets rid of the renamed_dependency method on PublishBuilder because that should be the responsibility of the DependencyBuilder.
src/tests/krate.rs
Outdated
| let mut req = new_req(name, "1.0.0"); | ||
| sign_in(&mut req, &app); | ||
| let json = bad_resp!(middle.call(&mut req)); | ||
| let (_, _, _, token) = TestApp::with_proxy().with_token(); |
There was a problem hiding this comment.
I think you can get away with TestApp::init() here because the crates aren't actually published.
| @@ -694,27 +679,17 @@ fn new_renamed_crate() { | |||
|
|
|||
| #[test] | |||
| fn new_krate_non_canon_crate_name_dependencies() { | |||
| let (_b, app, middle) = app(); | |||
There was a problem hiding this comment.
The test name sounds like this should create foo-dep and then attempt to publish a crate depending on foo_dep, which I think would be expected to fail. You've ported the existing test faithfully, but currently this seems identical to the first half of test new_krate_with_dependency. Feel free to just throw a comment in for now if you agree.
There was a problem hiding this comment.
Yeah, I agree this test name is confusing and it took me a minute to figure out why it was doing what it was doing as well. The reason is we had a bug where you couldn't publish crates that depended on crates where their name didn't equal their canon_crate_name normalized in the database :(
I agree it's largely a duplicate with new_krate_with_dependency, I'm going to delete that test and leave this one with a comment that the dependency name is important :)
| @@ -636,9 +638,11 @@ fn new_with_renamed_dependency() { | |||
| CrateBuilder::new("package-name", user.as_model().id).expect_build(&conn); | |||
| }); | |||
|
|
|||
| let dependency = DependencyBuilder::new("package-name").rename("my-name"); | |||
src/tests/krate.rs
Outdated
| CrateBuilder::new("foo_wild", user.id).expect_build(&conn); | ||
| } | ||
| let json = bad_resp!(middle.call(&mut req)); | ||
| let (app, _, user, token) = TestApp::with_proxy().with_token(); |
There was a problem hiding this comment.
I think this can be TestApp::init() as well.
src/tests/krate.rs
Outdated
| let conn = app.diesel_database.get().unwrap(); | ||
| let user = new_user("foo").create_or_update(&conn).unwrap(); | ||
| sign_in_as(&mut req, &user); | ||
| let (app, _, user, token) = TestApp::with_proxy().with_token(); |
This tests the same functionality as the new_krate_non_canon_crate_name_dependencies test, but that test has an additional detail that tests for a specific regression so we'll keep that one.
This makes the name less confusing but still ensures we have a regression test for the specific bug mentioned.
|
Ready for rereview @jtgeibel ! Thank you! |
|
LGTM! Thanks for adding the link in the test to the original issue. bors: r+ |
Build succeeded |
More!