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

backburner >=2.2.0 (ember >=3.2.0) breaks tests which are using fake timers of sinon/lolex #351

Closed
bastimeyer opened this issue Sep 27, 2018 · 5 comments · Fixed by #352
Closed

Comments

@bastimeyer
Copy link

bastimeyer commented Sep 27, 2018

This bug was introduced by the following commit and is included in all backburner releases since 2.2.0:
27690de

I wasn't sure whether this issue should have been opened on the ember issue tracker or here, but since the bug is included in this repo, I thought this was the right place.

I've created a simple Ember app repo to demonstrate this issue. This demonstration-app is basically just a simple component with a debounce call and Sinon's useFakeTimers method for testing the debounce behavior on a simple class name binding.
bastimeyer/backburner-settimeout-bug@3739940aeb7768aa47e66a18ab4359c12275a8fb
bastimeyer/backburner-settimeout-bug@08cb10e02d3c391b83dc3c3ee1e7d10760f7bb73

Upgrading ember-source to >=3.2.0 which includes backburner >=2.2.0 breaks this test by throwing a TypeError:
Uncaught TypeError: Cannot read property 'apply' of undefined


The issue is caused by a static reference of setTimeout in some backburner methods which can't be replaced on the global/window context by sinon or lolex after the useFakeTimers method has been called:
https://github.com/BackburnerJS/backburner.js/blob/v2.3.0/lib/backburner/platform.ts#L9
https://github.com/BackburnerJS/backburner.js/blob/v2.3.0/lib/backburner/platform.ts#L33
https://github.com/BackburnerJS/backburner.js/blob/v2.3.0/lib/backburner/platform.ts#L38

If you replace the SET_TIMEOUT reference with simple setTimeout calls in the built ember-source/dist/... file and re-run the test, everything is working just fine.

I'm not sure why this SET_TIMEOUT constant was added, but since I don't know anything about the internals of this project either, I didn't want to submit a PR. This looks like a simple mistake, though, because there's an unused SET_TIMEOUT constant in the main backburner module here which lead to this bug:
https://github.com/BackburnerJS/backburner.js/blob/v2.3.0/lib/index.ts#L26
27690de#diff-13b5b151431c7e7a17f273559ed212d5L212


Backburner >=2.2.0 is now being used in all Ember releases since 3.2.0, which makes me unable to upgrade my applications from ember 3.1.4 to a newer release. I'd appreciate if older versions could get a patch release, too, after this gets fixed.

Thanks!

@rwjblue
Copy link
Collaborator

rwjblue commented Sep 27, 2018

#352 should address

@rwjblue
Copy link
Collaborator

rwjblue commented Sep 27, 2018

I'm not sure why this SET_TIMEOUT constant was added, but since I don't know anything about the internals of this project either, I didn't want to submit a PR. This looks like a simple mistake, though, because there's an unused SET_TIMEOUT constant in the main backburner module here which lead to this bug

A few things here, the caching of SET_TIMEOUT was definitely intentional. Unfortunately, it was only intended to be used inside the next implementation (not platform.setTimeout). It is quite important to ensure that fake timers cannot prevent Ember's auto runs from completing (which is what platform.next is used for), and it should therefore not be affected by fake timers.

Thank you for writing up such a detailed bug report! It made identifying and fixing the issue much simpler.

@rwjblue
Copy link
Collaborator

rwjblue commented Oct 2, 2018

emberjs/ember.js#17029 brings the fix back to Ember (should be backported to 3.4.x).

@BillyRayPreachersSon
Copy link

BillyRayPreachersSon commented Nov 15, 2018

Are there any plans to release an ember-cli > 3.4.3, so that LTS users can get this fix?

We've just finished the mammoth task of upgrading our app from 2.18.2 to 3.4.3, and have had to skip our session timeout tests that use Sinon.JS to fake the timers.

It would be nice to be able to have coverage while remaining on an LTS version of ember-cli.

@rwjblue
Copy link
Collaborator

rwjblue commented Nov 21, 2018

@BillyRayPreachersSon you shouldn't need an ember-cli version bump to be able to get ember-source@3.4.x, you can bump your apps own dependency on ember-source as needed.

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 a pull request may close this issue.

3 participants