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

Test redirect_count boundaries in Fetch #9391

Merged
merged 1 commit into from Jan 20, 2016

Conversation

@nikkisquared
Copy link
Contributor

nikkisquared commented Jan 20, 2016

I've written two new tests for Fetch: one to test the highest possible number of redirects succeeds; and another to ensure a failure in Fetch by requesting too many redirects. I also wrote a helper function to be used by each test, since the main difference is how many times they try to redirect.

I've also changed the check against redirect_count in http_network fetch to compare it as greater than or equal to 20, as opposed to being only equal to 20. That's outside of the spec, but in my experience testing for pure equality can easily create errors. Even though it's technically not possible for redirect_count be above 20, bizarre bugs during runtime certainly happen.

Review on Reviewable


let fetch_response = test_fetch_redirect_count(MESSAGE, redirect_cap);

if Response::is_network_error(&fetch_response) {

This comment has been minimized.

@jdm

jdm Jan 20, 2016

Member

assert_eq!(Response::is_network_error(&fetch_response), false);


let fetch_response = test_fetch_redirect_count(MESSAGE, redirect_cap);

if !Response::is_network_error(&fetch_response) {

This comment has been minimized.

@jdm

jdm Jan 20, 2016

Member

assert_eq!(Response::is_network_error(&fetch_response), true);

@jdm jdm self-assigned this Jan 20, 2016
@jdm
Copy link
Member

jdm commented Jan 20, 2016

Go ahead and address my comments and squash all the commits together.

@nikkisquared nikkisquared force-pushed the nikkisquared:test_redirect branch from 0ca19f1 to 5426df3 Jan 20, 2016
@nikkisquared
Copy link
Contributor Author

nikkisquared commented Jan 20, 2016

@jdm updated and squashed! I also ran tidy-test to make sure I didn't miss anything.

@jdm
Copy link
Member

jdm commented Jan 20, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 20, 2016

📌 Commit 5426df3 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jan 20, 2016

Testing commit 5426df3 with merge c80fa33...

bors-servo added a commit that referenced this pull request Jan 20, 2016
Test redirect_count boundaries in Fetch

I've written two new tests for Fetch: one to test the highest possible number of redirects succeeds; and another to ensure a failure in Fetch by requesting too many redirects. I also wrote a helper function to be used by each test, since the main difference is how many times they try to redirect.

I've also changed the check against redirect_count in http_network fetch to compare it as greater than or equal to 20, as opposed to being only equal to 20. That's outside of the spec, but in my experience testing for pure equality can easily create errors. Even though it's technically not possible for redirect_count be above 20, bizarre bugs during runtime certainly happen.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9391)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 20, 2016

💔 Test failed - linux-rel

@jdm
Copy link
Member

jdm commented Jan 20, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jan 20, 2016

Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-css are reusable. Rebuilding only linux-rel, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Jan 20, 2016

@bors-servo bors-servo merged commit 5426df3 into servo:master Jan 20, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@nikkisquared nikkisquared deleted the nikkisquared:test_redirect branch Jan 21, 2016
@emilio emilio removed the S-tests-failed label Jan 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.