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

some fixes for intermittent tests failing #1626

Merged
merged 1 commit into from
Sep 27, 2017

Conversation

snapwich
Copy link
Collaborator

@snapwich snapwich commented Sep 26, 2017

Type of change

  • Bugfix

Description of change

Lately the tests have been intermittently failing more often, this pull-request solves some of the issues.

I think one of the big issues was related to #1536; it added an asynchronous test that was not treated asynchronously and also included a potential race-condition with between two setTimeouts. This had the effect of causing other suites to fail randomly (some of the "before each hook" or "after each hook" failures), which was pretty hard to track down.

I think some of the other issues are the result of improper cleanup of stubbed methods and the global XMLHttpRequest object, of which I have a few fixes in this pull-request.

Even with the changes in this branch, I still had one of these fails:

HeadlessChrome 0.0.0 (Mac OS X 10.11.6) Trion adapter tests "after each" hook FAILED
	Error: Uncaught TypeError: Fake XHR onreadystatechange handler threw exception: Cannot read property 'seatbid' of undefined (node_modules/sinon/pkg/sinon.js:4469)
HeadlessChrome 0.0.0 (Mac OS X 10.11.6): Executed 1053 of 1430 (1 FAILED) (skipped 1) (0 secs / 2.904 secs)
HeadlessChrome 0.0.0 (Mac OS X 10.11.6) Trion adapter tests "after each" hook FAILED
	Error: Uncaught TypeError: Fake XHR onreadystatechange handler threw exception: CannotHeadlessChrome 0.0.0 (Mac OS X 10.11.6): Executed 1053 of 1430 (1 FAILED) (skipped 1) (18.494 secs / 2.904 secs)

But only once after many runs. I wasn't able to track down the true source of that error as a stack trace would be necessary considering many adapters use XHR handlers that contain a seatbid property, and considering the non-deterministic nature of these errors (I think it might depend on file load-order) I wasn't able to catch this bug as it happened.

Other information

It may be worthwhile to make a requirement of all future testing suites added to Prebid.js that they must use the beforeEach(() => {sandbox = sinon.sandbox.create() }) and afterEach(() => { sandbox.restore() }) at the root level of the their suite, and not allow any other uses of the sinon global, only sandbox. This includes using the sandbox.useFakeServer() for any server request testing. Doing so makes auditing of of proper test cleanup much easier.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

thanks!

@dbemiller
Copy link
Contributor

dbemiller commented Sep 27, 2017

You're the best!

Re: "require use of sandbox", I came up with a pattern which makes this one-liner for the spec files. The util is implemented in videoCacheStub, and you can see it used in the bidmanager_spec. I haven't used it in too many places, but so far I'm happy with how convenient it makes things.

These issues might also go away as adapters migrate to the 1.0 bidderFactory, since all the the async stuff is outside of their code now.

@dbemiller dbemiller merged commit 1d2a7c6 into prebid:master Sep 27, 2017
@snapwich snapwich deleted the failing-tests branch September 27, 2017 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants