BackburnerJS / backburner.js

A rewrite of the Ember.js run loop as a generic microlibrary
MIT License
392 stars 80 forks source link

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

Closed bastimeyer closed 5 years ago

bastimeyer commented 5 years ago

This bug was introduced by the following commit and is included in all backburner releases since 2.2.0: backburnerjs/backburner.js@27690deba7a9122e036088a603f4f0374c3ba2af

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 https://github.com/BackburnerJS/backburner.js/commit/27690deba7a9122e036088a603f4f0374c3ba2af#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 commented 5 years ago

352 should address

rwjblue commented 5 years ago

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 commented 5 years ago

https://github.com/emberjs/ember.js/pull/17029 brings the fix back to Ember (should be backported to 3.4.x).

BillyRayPreachersSon commented 5 years ago

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 commented 5 years ago

@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.