-
Notifications
You must be signed in to change notification settings - Fork 207
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
Add unit tests from the smoke test PR #570
Conversation
This PR is best reviewed commit-by-commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments, but overall this seems good to me :) I'm sure these tests will prove to be helpful in the future.
src/test/mod.rs
Outdated
@@ -43,7 +43,11 @@ pub(crate) fn assert_redirect(path: &str, expected_target: &str, web: &TestFront | |||
let status = response.status(); | |||
|
|||
// Reqwest follows redirects automatically |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a bit nit-picky... but I find this comment confusing now. This comment originally had a slightly different place/context. Now it seems to tell us something about the conversion of response
to redirect_target
, but the redirect has already happened by the time we get here.
I would propose:
- move this comment up a bit, maybe at
let response = web.get(path).send()?;
? - perhaps add a (short) comment at
let redirect_target = if ...
to clarify why we sometimes take the full URL and sometimes the path only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a change that comments this, does that make it more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is great 👍
- add `source_files()` - add /:crate/:version/:name/index.html if it doesn't already exist - add source files as rustdoc files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick fixes. Looks good to me!
Obsoletes #524.
This is ready for review.
Things to do:
/crate/:crate/:version