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

Update sinon to > 6.1.1 #17190

Closed
prateekbh opened this issue Jul 30, 2018 · 6 comments · Fixed by #17197
Closed

Update sinon to > 6.1.1 #17190

prateekbh opened this issue Jul 30, 2018 · 6 comments · Fixed by #17197

Comments

@prateekbh
Copy link
Member

What's the issue?

I am trying to add a tests for a file of mine which uses XHR. In order to do this, I tried setting up a small testfile for my code and mocked the XHR in exact same way as https://github.com/ampproject/amphtml/blob/master/test/functional/test-xhr.js#L76-L93.

Adding this in a filename test-document-fetcher.js in test/functional folder lead to breaking of test in test-preconnect.js which modifies XHRHttpRequest.prototype https://github.com/ampproject/amphtml/blob/master/test/functional/test-preconnect.js#L88

I tried removing as much code as possible from my test, which made me zero down the cause to using sandbox.useFakeXMLHttpRequest().

Looks like there was a bug in sinon itself: sinonjs/sinon#1840

And was fixed in 6.1.1

How do we reproduce the issue?

  1. Make a file test-dummy.js in test/functional folder.
  2. Use sandbox.useFakeXMLHttpRequest() in it and write a dummy test around XMLHttpRequest.
  3. Allow travis to run this in saucelabs.
  4. test-preconnect.js will fail.

What browsers are affected?

None

Which AMP version is affected?

Not sure

@rsimha
Copy link
Contributor

rsimha commented Aug 2, 2018

@prateekbh Let me know if useFakeXMLHttpRequest works now.

@rsimha rsimha added this to the Sprint H2 July 2018 milestone Aug 2, 2018
@prateekbh
Copy link
Member Author

@rsimha thank you for the work. Sadly it didn't solved what I was looking for.

I made a dummy PR to repro this: #17263

@rsimha
Copy link
Contributor

rsimha commented Aug 2, 2018

What's the problem? I can get that test and others to pass.

@prateekbh
Copy link
Member Author

Everything's fine on local machine, it goes to travis -> saucelabs and breaks test-preconnect.js which is not even touched in this PR

@prateekbh
Copy link
Member Author

@rsimha
Copy link
Contributor

rsimha commented Aug 2, 2018

Seems like your test isn't properly mocking the global XMLHttpRequest, so you'll need to make sure that's being done correctly. If you can't get it to work, please file a separate issue.

See https://stackoverflow.com/a/33978113

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

Successfully merging a pull request may close this issue.

2 participants