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

Additional testing around XHR events to ensure only expected events are fired #1043

Merged
merged 1 commit into from
May 25, 2016

Conversation

fearphage
Copy link
Member

Purpose (TL;DR) - mandatory

Added additional tests around the events fired from fake XHR. There are no code changes -- just additional tests.

Background (Problem in detail) - optional

These tests are related to and build on tests added in #1041. There are partially in response to the conversation in #432.

How to verify - mandatory

  1. Check out this branch (see github instructions below)
  2. npm install
  3. npm test

@fearphage fearphage force-pushed the additional-fake-xhr-tests branch from a51b48a to 57f9082 Compare May 7, 2016 16:18
@fatso83 fatso83 merged commit 9266e42 into sinonjs:master May 25, 2016
@fatso83
Copy link
Contributor

fatso83 commented May 25, 2016

P.S. The Travis build fails when doing the cloud tests, but this is not this test's fault (we think). We have unfortunately have had failing builds for a while (history) ...

@fatso83
Copy link
Contributor

fatso83 commented May 25, 2016

@mantoni any idea on how to get some more info out of mochify? The current build just seems to stall and time out without any error messages.

@fearphage fearphage deleted the additional-fake-xhr-tests branch May 25, 2016 11:39
@mantoni
Copy link
Member

mantoni commented May 25, 2016

You can try to reproduce locally with a local WebDriver install. It's easy to set up and instruction are provided here.

There is also an option in min-webdriver to keep the browser open on failures (set closeOnError to false). This gives you a chance to inspect the browser console output.

@fearphage
Copy link
Member Author

I'm on master and fully up to date, but the command run is not even the same:

➜  Sinon.JS git:(master) npm run test-headless

> [email protected] test-headless /home/phred/src/git/Sinon.JS
> mochify --recursive -R dot --grep WebWorker --invert test/

# phantomjs:



  ...............................................................................................................................................................................................
  ...............................................................................................................................................................................................
  ...............................................................................................................................................................................................
  ......................................test
.........................................................................................................................................................
  ...............................................................................................................................................................................................
  ...............................................................................................................................................................................................
  ...............................................................................................................................................................................................
  ....................................................................................................................................................

  1477 passing (856ms)
  8 pending

Compared to Travis:

> [email protected] test-headless /home/travis/build/sinonjs/sinon
> mochify --recursive -R dot --grep WebWorker --invert test/ "--wd"

@fearphage
Copy link
Member Author

I found the error in my ways. I should be running npm run test-cloud instead.

New problem: SAUCE_ACCESS_KEY

I pulled the SAUCE_USERNAME from the Travis logs.

@mantoni
Copy link
Member

mantoni commented May 25, 2016

@fearphage The easiest is to create a free SauceLabs account and put your user and access key into your environment. Then you can always run any mochify browser tests from your local console. It doesn't have to be the one used for the Sinon travis builds.

@mantoni
Copy link
Member

mantoni commented May 26, 2016

@fearphage @fatso83 I tried to reproduce the build issue on my machine, but I can't reproduce. Travis seems to time out after 10 minutes, and what I see is that IE 10 and 11 take a long time (around 2 minutes) to run. That still does not explain why Travis does not receive any output and fails with a timeout eventually.

However, there are two test cases that currently fail on IE 10 and IE 11:

1479 passing (2m)
8 pending
2 failing

1) sinon.clock .tick fires timers in correct order:
   timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.


2) sinon.clock .tick passes 2 hours, 34 minutes and 12 seconds:
   timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

There have been a few releases of Mocha recently – that's the only external change that comes to mind.

@fatso83
Copy link
Contributor

fatso83 commented May 27, 2016

@mantoni This did not start recently. Those two tests started failing 2 months ago with this build that merged pull request #1023. Ref my comment in that issue.

@fearphage
Copy link
Member Author

If you wait long enough, they do pass:

Fixes

  • timers in correct order - Set timeout for test to 5 seconds (this.timeout(5000);). It generally takes 2.5 seconds in IE.
  • passes 2 hours, 34 minutes and 12 seconds - Set test timeout to 50 seconds (this.timeout(50000)). I'm seeing it generally takes roughly 25 seconds, but I'm just being safe here. I think this is a bug in lolex. I'm investigating.

@fatso83
Copy link
Contributor

fatso83 commented May 27, 2016

@fearphage That's great news. I have been scratching my head about this for some time, but haven't looked into it really hard.

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.

3 participants