Begin working on a revamped test harness #1697

sgrif · 2019-03-25T23:16:32Z

This is yet another attempt to improve our test harness. This is
intended to be a replacement for TestApp which currently lives in
tests/util.rs

This new structure tries to address several pain points I've had using
the existing harness:

Abstracting into smaller pieces

Most of the functions in the current harness give no opportunity to
modify the request, require a type to parse the response into, and
assume the request is a 2xx or 3xx response. This has made testing our
edge cases difficult, and they're less well tested as a result. I've
attempted to make it easier to do: "just like this other thing but
with an additional header set", or "I don't care about the response
body"

What was previously anon.get::<T>("...") will now be 3 separate
pieces:
- app.get("...") returns a request object that can be modified if
  needed
- request.send() returns a Result, where a 4xx or 5xx response is
  considered an error
- response.json<T>() will parse the body into the given type
The API is roughly modeled after reqwest, which I've enjoyed using
in our app.
More explicit handling of auth

The current API for auth has felt really funky to me. Having an object
called User constructing HTTP requests felt backwards, and it also
meant we had to decide up front whether we were going to send some
requests signed in or not, and also how we were authenticating. It
also didn't have a good way to send requests as more than one user.

The new API moves auth to be a part of request construction. So
user.get("...") becomes app.get("...").as_user(&user)

Finally, I've structured this new API around returning Result
everywhere, so that we can keep assertions closer to the tests where
they matter (giving line numbers in the test, not the util file). This
is a bit tricky, and I'll probably be tweaking as I move tests over,
since results bubbled all the way up with ? will completely lose the
backtrace. Right now, I'm figuring that things we really just don't
expect to fail ever get bubbled up with ?, while the things we're
actually testing will remain as assertions.

The tests I've moved over don't have a ton to abstract, but I'd also
like to keep helpers like publish off of the App struct, and instead
keep them in the test file where they're relevant.

I've started by just moving tests/read_only_mode over, since it's a
relatively small file, and also one where the old API was more painful
for me to use. I'll be porting other files over in the next few days.

jtgeibel

In general I'm supportive of adapting to an API similar to reqwest. I'd like to point out some things I observed when building out parts of the v2 harness that I think are worth considering in the final design.

Auth

The current API for auth has felt really funky to me. Having an object
called User constructing HTTP requests felt backwards, and it also
meant we had to decide up front whether we were going to send some
requests signed in or not, and also how we were authenticating. It
also didn't have a good way to send requests as more than one user.

One of the huge issues I had in following some scenarios in the v1 harness was that in many tests a single request was created and then mutated throughout the test run. Sometimes it was necessary to trace through the logic to determine which combination of HTTP method, url, and user were in play for a particular request.

While I like the idea of building partial requests and then being able to add authentication along the way, I'd like to make sure that it isn't possible (in the common workflow) to create a request with multiple cookies/tokens or even both a cookie and a token. Maybe authentication can normally be provided in the final step, such as .send_with(cookie) and .send_with(token), with .send() producing an anonymous request.

It also didn't have a good way to send requests as more than one user.

This didn't work well in the v1 design, but this should be addressed by the v2 implementation. Calling app.db_new_user("user2") will add a user to the database and return a MockCookieUser value that represents that user and their session cookie. Calling user2.db_new_token("bar") creates a token for that user and gives a value that authenticates with the new token. These can be initialized at the beginning of the test and used to issue multiple requests.

Response handling

request.send() returns a Result, where a 4xx or 5xx response is
considered an error

response.json<T>() will parse the body into the given type

While I support having a better split between handling response codes and deserializing the response, there is one area where I think it is helpful to keep these somewhat coupled. Until we can switch to 5xx for all api errors on cargo endpoints, it is very helpful to require a status code when deserializing to the Bad JSON response value with the current helper. In the v2 test harness forces the test to assert the expected response code for that API endpoint.

Error handling

This is a bit tricky, and I'll probably be tweaking as I move tests over,
since results bubbled all the way up with ? will completely lose the
backtrace. Right now, I'm figuring that things we really just don't
expect to fail ever get bubbled up with ?, while the things we're
actually testing will remain as assertions.

In my experience I would stick to calling unwrap everywhere. I never find the source reference in the top-level error to be helpful without a backtrace. When using unwrap the error points to src/libcore/result.rs:997:5 and when using ? you get /cargo/registry/src/git.colasdn.top-1ecc6299db9ec823/libtest-0.0.1/lib.rs:335:5. As you mention, the downside of ? is that when you do request that backtrace, much of the associated call stack context is lost. If an error condition is unexpected in the tests, I think it usually makes sense to panic as quickly as possible.

More conversions

I've started by just moving tests/read_only_mode over, since it's a
relatively small file, and also one where the old API was more painful
for me to use. I'll be porting other files over in the next few days.

There are about a dozen tests remaining against the v1 harness (calling app() or simple_app() directly). It may be helpful to convert those next so that everything is either v2 or v3 going forward. I'd also like to land #1686 soon, which includes some finishing touches on the v2 harness in terms of orchestrating the index, workers, and playback proxy.

jtgeibel · 2019-03-27T01:16:42Z

src/tests/read_only_mode.rs


    app.db(|conn| {
-        CrateBuilder::new("foo_download_read_only", user.as_model().id)
+        let user = app.create_user("new_user")?;


I'm not sure how this works here, because crate_user appears to get a separate database connection from the pool, but as far as I can tell this is working. I can even add app.get("/api/v1/users/new_user").send().unwrap() within the db closure (after the user is created), and everything seems to work. I don't yet understand exactly what is going on here.

We only have the single connection in tests, it doesn't matter how many times we ask for it we'll always get back the same connection

carols10cents · 2019-03-27T23:32:10Z

src/tests/read_only_mode.rs

+        .delete("/api/v1/crates/foo_yank_read_only/1.0.0/yank")
+        .with_token(&token)
+        .send()
+        .allow_errors()?;


Does this mean every time we test for an error, we have to call allow_errors?

Right now, yeah. I'll likely end up with an expect_status function that returns the response if the status matched or panics otherwise. Erroring on 4xxx/5xx felt like the right default though, so this makes sense as a primitive to get out of that default behavior

carols10cents · 2019-03-27T23:58:12Z

Auth

The current API for auth has felt really funky to me. Having an object
called User constructing HTTP requests felt backwards, and it also
meant we had to decide up front whether we were going to send some
requests signed in or not, and also how we were authenticating. It
also didn't have a good way to send requests as more than one user.

One of the huge issues I had in following some scenarios in the v1 harness was that in many tests a single request was created and then mutated throughout the test run. Sometimes it was necessary to trace through the logic to determine which combination of HTTP method, url, and user were in play for a particular request.

Indeed, I'm wondering how the proposed changes in this PR would look with multiple requests done by the same user. I rather like how the current tests make you think about who is doing the requesting up front, and that it's who_is_requesting.get() rather than app.get().with_user(who_is_requesting) where .with_user() would need to be called on each new request OR we go back to the problem @jtgeibel mentioned where it wasn't clear what the current state of the reused request was.

It also didn't have a good way to send requests as more than one user.

This didn't work well in the v1 design, but this should be addressed by the v2 implementation.

@sgrif could you perhaps convert a test that sends requests as more than one user as part of this PR to explore what would be different?

The tests I've moved over don't have a ton to abstract, but I'd also
like to keep helpers like publish off of the App struct, and instead
keep them in the test file where they're relevant.

Do you propose having all tests that publish a crate be in one file, or do you propose duplicating the publish code in multiple files? There are multiple files that publish crates now, and I've also often considered splitting up the krate tests into multiple files because there's a lot of tests in that file and it's hard to navigate.

It may be helpful to convert those next so that everything is either v2 or v3 going forward.

👍 👍 👍 👍 👍

I do think there's room for improvement with the current test utils; I have an unfinished branch somewhere trying to make the construction of app/user/etc not need so many underscores to ignore unneeded parts.

sgrif · 2019-03-28T16:56:53Z

OR we go back to the problem @jtgeibel mentioned where it wasn't clear what the current state of the reused request was.

I've specifically made it so RequestBuilder takes self and not &mut self to avoid this. There shouldn't be any way to re-use a single Request with modifications. Tests which want similar behavior but with minor differences will need to put the duplicated bits in a function.

Do you propose having all tests that publish a crate be in one file, or do you propose duplicating the publish code in multiple files? There are multiple files that publish crates now, and I've also often considered splitting up the krate tests into multiple files because there's a lot of tests in that file and it's hard to navigate.

I'd expect the tests around publish behavior to live in one file, yes. This is a false dichotomy though, we can certainly put shared helpers in their own files. I'm just arguing we shouldn't have them live on App in a pile. Ideally they'd start in the file with the tests that need them, and if we find ourselves duplicating across multiple files we abstract from there.

This is yet another attempt to improve our test harness. This is intended to be a replacement for `TestApp` which currently lives in `tests/util.rs` This new structure tries to address several pain points I've had using the existing harness: - Abstracting into smaller pieces Most of the functions in the current harness give no opportunity to modify the request, require a type to parse the response into, and assume the request is a 2xx or 3xx response. This has made testing our edge cases difficult, and they're less well tested as a result. I've attempted to make it easier to do: "just like this other thing but with an additional header set", or "I don't care about the response body" What was previously `anon.get::<T>("...")` will now be 3 separate pieces: - `app.get("...")` returns a request object that can be modified if needed - `request.send()` returns a `Result`, where a 4xx or 5xx response is considered an error - `response.json<T>()` will parse the body into the given type The API is roughly modeled after `reqwest`, which I've enjoyed using in our app. - More explicit handling of auth The current API for auth has felt really funky to me. Having an object called `User` constructing HTTP requests felt backwards, and it also meant we had to decide up front whether we were going to send some requests signed in or not, and also how we were authenticating. It also didn't have a good way to send requests as more than one user. The new API moves auth to be a part of request construction. So `user.get("...")` becomes `app.get("...").as_user(&user)` Finally, I've structured this new API around returning `Result` everywhere, so that we can keep assertions closer to the tests where they matter (giving line numbers in the test, not the util file). This is a bit tricky, and I'll probably be tweaking as I move tests over, since results bubbled all the way up with `?` will completely lose the backtrace. Right now, I'm figuring that things we really just don't expect to fail ever get bubbled up with `?`, while the things we're actually testing will remain as assertions. The tests I've moved over don't have a ton to abstract, but I'd also like to keep helpers like `publish` off of the `App` struct, and instead keep them in the test file where they're relevant. I've started by just moving `tests/read_only_mode` over, since it's a relatively small file, and also one where the old API was more painful for me to use. I'll be porting other files over in the next few days.

sgrif · 2019-04-01T20:45:29Z

I've added a test (krate::only_owners_can_yank) which demonstrates what it looks like when requests are sent as multiple users. The old version didn't try sending a request as more than one user -- it was never verifying the crate in question could be yanked by the owner, and as such it wasn't actually even testing what it thought it was. The new version does this (side note: I'm thinking we should probably have CrateBuilder update the index so we can have a fully "published" crate in tests that aren't about the publish endpoint without having to deal with the S3 proxy, but that change is out of scope for this PR)

sgrif · 2019-04-02T16:16:07Z

Just to address a few remaining comments that I don't think have been addressed in the code or could use further explanation:

I'd like to make sure that it isn't possible (in the common workflow) to create a request with multiple cookies/tokens or even both a cookie and a token.

Following the builder pattern, it's possible to override the previous value by calling the function multiple times, but I've made sure that it never sends both a token and a cookie.

Until we can switch to 5xx for all api errors on cargo endpoints, it is very helpful to require a status code when deserializing to the Bad JSON response value with the current helper.

As of Rust 1.34 (which is releasing this week), errors are nicely displayed regardless of response code. As part of migrating tests over to this harness, I think we should also go through the process of converting the endpoints being tested to non-2xx response codes on error (and also probably ensuring its error conditions are sufficiently tested). I'd really like to have send return Ok only if we receive a 2xx or 3xx response, so that for calls that don't care about the response body, only that it succeeded, we never have to look at the body. ?/.unwrap should be enough to know that whatever we wanted to do at that endpoint succeeded.

In my experience I would stick to calling unwrap everywhere. I never find the source reference in the top-level error to be helpful without a backtrace.

I'm fine with using .unwrap in the tests, but if we go that route I'd like to make sure we only do it in the top level test function, and not in the test harness. When we start unwrapping in the test harness, it gives the tests no opportunity to handle errors or expect them (which is part of the motivation for this in the first place). Rather than switching to unwrap right now though, I'd like to see if we can possibly start including the backtrace in the error itself, which would also improve our ability to debug where the error came from in the request (which is currently always lost to us). For that reason I've left this using ? for now. I can block this on investigating adding backtraces if we think it's necessary, but do you think you'd be comfortable with moving forward noting that we will either add backtraces to CargoResult or converting tests that return Result to unwrap instead in order to keep this small/reasonable to review?

OR we go back to the problem @jtgeibel mentioned where it wasn't clear what the current state of the reused request was.

Just to make sure I've communicated this well, all methods around building a request, including .send take self by value intentionally to prevent this. For cases where we want to re-use bits of behavior, this should encourage pulling out helper functions instead of mutating the same request.

With the changes I've made and these notes, are we comfortable moving forward with this direction? There will certainly be more iteration on this as more tests get converted (as was the case with the v2 harness), but hopefully we're on the same page about the general direction this is headed.

bors · 2019-05-22T23:54:14Z

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

Fix yank test This test wasn't set up correctly, so the error message that was being checked wasn't quite right. Also, [based on this comment](#1697 (comment)) it seems like the names of these tests weren't as descriptive as they could be-- the previous test covers the success case of an owner being able to yank, and the next text checks that nonowners aren't allowed to yank.

kzys · 2019-11-19T15:15:07Z

@jtgeibel / @carols10cents - if the rebasing and resolving conflicts are the only remaining actions for this PR, I'm happy to help and reduce our PR backlog before 2020 :)

jtgeibel · 2020-04-04T16:50:15Z

I'm going to close this for now. We can always reopen this PR if we have the bandwidth to convert over a larger portion of the tests.

sgrif requested a review from jtgeibel March 25, 2019 23:16

jtgeibel reviewed Mar 27, 2019

View reviewed changes

carols10cents reviewed Mar 27, 2019

View reviewed changes

sgrif added 2 commits April 1, 2019 14:17

Add a test demonstrating requests sent as two users

86a9270

sgrif force-pushed the sg-test-harness-v3 branch from 72214c7 to 86a9270 Compare April 1, 2019 20:42

sgrif added 2 commits April 2, 2019 09:53

Fix warnings

e60ae07

Ensure that we never send both a cookie and a token for auth

2e3ee04

rustfmt

9e6d3a3

carols10cents added the S-waiting-on-review label Apr 7, 2019

carols10cents mentioned this pull request May 23, 2019

Fix yank test #1753

Merged

jtgeibel closed this Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Begin working on a revamped test harness #1697

Begin working on a revamped test harness #1697

sgrif commented Mar 25, 2019

jtgeibel left a comment

jtgeibel Mar 27, 2019

sgrif Mar 27, 2019

carols10cents Mar 27, 2019

sgrif Mar 28, 2019

carols10cents commented Mar 27, 2019

Auth

sgrif commented Mar 28, 2019

sgrif commented Apr 1, 2019

sgrif commented Apr 2, 2019

bors commented May 22, 2019

kzys commented Nov 19, 2019

jtgeibel commented Apr 4, 2020

Begin working on a revamped test harness #1697

Begin working on a revamped test harness #1697

Conversation

sgrif commented Mar 25, 2019

jtgeibel left a comment

Choose a reason for hiding this comment

Auth

Response handling

Error handling

More conversions

jtgeibel Mar 27, 2019

Choose a reason for hiding this comment

sgrif Mar 27, 2019

Choose a reason for hiding this comment

carols10cents Mar 27, 2019

Choose a reason for hiding this comment

sgrif Mar 28, 2019

Choose a reason for hiding this comment

carols10cents commented Mar 27, 2019

Auth

sgrif commented Mar 28, 2019

sgrif commented Apr 1, 2019

sgrif commented Apr 2, 2019

bors commented May 22, 2019

kzys commented Nov 19, 2019

jtgeibel commented Apr 4, 2020