sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
806 stars 107 forks source link

lolex.install({target: otherContext}) injects properties from original global #276

Closed fatso83 closed 4 years ago

fatso83 commented 4 years ago

What did you expect to happen? That lolex.install({target: myContext, ...otherProps}) would basically work like lolex.withGlobal(target).install({...otherProps}).

What actually happens If your environment supports performance and you do lolex.install({target: { toFake: ['setTimeout'] } }) it still fakes and injects performance into the returned clock.

assert.isUndefined(clock.performance);

Will therefore fail. This happens because the checks on whether to add the prop references the prop that was set using the default lolex instance.

How to reproduce

function noop(){}
global.performance = {now:function(){}};
var lolex = require('./src/lolex-src');
var clock2 = lolex.install({target: {Date: noop}});
assert.isUndefined(clock2.performance); // fails
fatso83 commented 4 years ago

An easy fix would be to either disallow the target prop and explicitly say "Update your code to uselolex.withGlobal(target).install(config)"` or just do it under the covers in the install method:

if(config.target) {
   var target = config.target;
   delete config.target;
   return withGlobal(target)(config);
}
SimenB commented 4 years ago

What if I want to install fake versions of timers from one thing into some other object? Is that a valid use case?

fatso83 commented 4 years ago

@SimenB I can't really come up with any reason to do that ... but I guess someone might want that ... for some unknown reason?

const myTarget = { Date: sinon.fake() };
lolex.withGlobal(global.jsDomWindow).install({target: myTarget});

That would install fake performance timers into myTarget, so you could tamper with that object without worrying about messing too much with global.jsDomWindow. Not sure who would need such a use case, but we can keep it. Then we would need to document it, with a hint to why you would use it.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

SimenB commented 4 years ago

I'm down with throwing on target usage and directing to use the factory function for that use case. It works with any use case I can think of at least

fatso83 commented 4 years ago

318

SimenB commented 4 years ago

This is fixed now, right?