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

Wrap global.setTimeout to work with mocks #165

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@dax

dax commented Mar 27, 2014

When a mock testing library (like sinonjs) override global.setTimeout, bluebird
was keeping the original version and was not using the mocked one.
By wrapping the call to global.setTimeout, bluebird will always use the
correct setTimeout implementation.

David Rousselie
Wrap global.setTimeout to work with mocks
When a mock library (like sinonjs) override global.setTimeout, bluebird
was keeping the original version and was not using the mocked one.
By wrapping the call to global.setTimeout, bluebird will always use the
correct setTimeout implementation.
@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Mar 27, 2014

Collaborator

Rather than every single library that saves setTimeout fixing this to become compatible with sinonjs, wouldn't it be much better if the fix was in sinonjs? It would be done by replacing setTimeout permanently and making sure that sinon.js is loaded first. It would choose whether to use the original or not internally depending on a flag)

Collaborator

spion commented Mar 27, 2014

Rather than every single library that saves setTimeout fixing this to become compatible with sinonjs, wouldn't it be much better if the fix was in sinonjs? It would be done by replacing setTimeout permanently and making sure that sinon.js is loaded first. It would choose whether to use the original or not internally depending on a flag)

@dax

This comment has been minimized.

Show comment
Hide comment
@dax

dax Mar 28, 2014

I am not fan of forcing module load order. Everytime I had to deal with that kind of assertion, it led to some time consuming bugs.

In the case of this PR, why not always use global.setTimeout, and create a wrapped global object (in the global.js module) with the expected setTimeout implementation only when it does not implement the signature bluebird expects ?

dax commented Mar 28, 2014

I am not fan of forcing module load order. Everytime I had to deal with that kind of assertion, it led to some time consuming bugs.

In the case of this PR, why not always use global.setTimeout, and create a wrapped global object (in the global.js module) with the expected setTimeout implementation only when it does not implement the signature bluebird expects ?

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Mar 28, 2014

Collaborator

Thats fine, I think that @petkaantonov will accept the patch, I was just wondering if it isn't extremely tedious to hunt down all libraries that save global functions. I think that it might be a failing of the testing library and should be examined.

Mutating shared globals automatically implies some kind of ordering - its an inevitable consequence. Setting up the fake environment before loading the modules that are going to be using it makes a lot of sense to me.

So rather than swapping functions around, sinon.js could just do this once:

function setTimeoutReplacement() {
  if (fakeMode) setTimeoutFake.call(void 0, arguments);
  else setTimeoutReal.call(void 0, arguments);
}

and if loaded first will always have complete control over what gets called.

Collaborator

spion commented Mar 28, 2014

Thats fine, I think that @petkaantonov will accept the patch, I was just wondering if it isn't extremely tedious to hunt down all libraries that save global functions. I think that it might be a failing of the testing library and should be examined.

Mutating shared globals automatically implies some kind of ordering - its an inevitable consequence. Setting up the fake environment before loading the modules that are going to be using it makes a lot of sense to me.

So rather than swapping functions around, sinon.js could just do this once:

function setTimeoutReplacement() {
  if (fakeMode) setTimeoutFake.call(void 0, arguments);
  else setTimeoutReal.call(void 0, arguments);
}

and if loaded first will always have complete control over what gets called.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Mar 28, 2014

Collaborator

I'm with @gorgi here, it is changing global state after all.

On 28 במרץ 2014, at 13:50, Gorgi Kosev notifications@github.com wrote:

Thats fine, I think that @petkaantonov will accept the patch, I was just wondering if it isn't extremely tedious to hunt down all libraries that save global functions. I think that its a failing of the testing library which should be examined.

Mutating shared globals automatically implies some kind of ordering - its an inevitable consequence. Setting up the fake environment before loading the modules that are going to be using it makes a lot of sense to me.

So rather than swapping functions around, sinon.js could just do this once:

function setTimeoutReplacement() {
if (fakeMode) setTimeoutFake.call(void 0, arguments);
else setTimeoutReal.call(void 0, arguments);
}
and if loaded first will always have complete control over what gets called.


Reply to this email directly or view it on GitHub.

Collaborator

benjamingr commented Mar 28, 2014

I'm with @gorgi here, it is changing global state after all.

On 28 במרץ 2014, at 13:50, Gorgi Kosev notifications@github.com wrote:

Thats fine, I think that @petkaantonov will accept the patch, I was just wondering if it isn't extremely tedious to hunt down all libraries that save global functions. I think that its a failing of the testing library which should be examined.

Mutating shared globals automatically implies some kind of ordering - its an inevitable consequence. Setting up the fake environment before loading the modules that are going to be using it makes a lot of sense to me.

So rather than swapping functions around, sinon.js could just do this once:

function setTimeoutReplacement() {
if (fakeMode) setTimeoutFake.call(void 0, arguments);
else setTimeoutReal.call(void 0, arguments);
}
and if loaded first will always have complete control over what gets called.


Reply to this email directly or view it on GitHub.

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