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

Remove dependency on httpbin.org and run local responses server #4921

Merged
merged 3 commits into from Sep 12, 2016

Conversation

mkhatib
Copy link
Contributor

@mkhatib mkhatib commented Sep 11, 2016

Fixes #3101 and #4899

@mkhatib
Copy link
Contributor Author

mkhatib commented Sep 11, 2016

yay! no flakes!

@@ -22,6 +22,8 @@ import {
FetchResponse,
assertSuccess,
} from '../../src/service/xhr-impl';
import {getCookie} from '../../src/cookies';


describe('XHR', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to rebase. I turned off these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@cramforce
Copy link
Member

This is great!!! A few little comments (and the rebase and subsequent removal of the skip is important), but lets get this in quickly!!!

@mkhatib
Copy link
Contributor Author

mkhatib commented Sep 11, 2016

Done PTAL 👀

@cramforce
Copy link
Member

LGTM

@cramforce cramforce merged commit c97c84b into ampproject:master Sep 12, 2016
@cramforce
Copy link
Member

Merged it for you, so we get the tests back on ASAP.

@mkhatib mkhatib deleted the xhr-flake branch September 12, 2016 18:00
dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Sep 16, 2016
@lannka
Copy link
Contributor

lannka commented Dec 28, 2016

@mkhatib have you considered using Sinon FakeServer? I believe launching this additional test server does add some latency.

If you really need a real HTTP server for test, I think Karma custom server middleware is a better option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants