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

Not resilient to modification of globals #516

Closed
dubzzz opened this issue Dec 5, 2024 · 9 comments · Fixed by #517
Closed

Not resilient to modification of globals #516

dubzzz opened this issue Dec 5, 2024 · 9 comments · Fixed by #517

Comments

@dubzzz
Copy link
Contributor

dubzzz commented Dec 5, 2024

The library does not resist destructive changes of the globals. As I initially reported the issue at vitest-dev/vitest#7024 I wanted to make you aware of the problem.

One fix, if you consider that it should be fixed, would be to capture the globals for Intl and Date when the code got imported.

Feel free to close, if the behavior is expected.


If the suggestion is relevant I can propose a fix.

@fatso83
Copy link
Contributor

fatso83 commented Dec 6, 2024

This is absolutely true and we have done similar changes in the main Sinon project to prevent pollution to globals (like methods on the Array.prototype) affecting us. If you want to supply a fix, it should just be a handful of lines.

@fatso83
Copy link
Contributor

fatso83 commented Dec 9, 2024

Meh, the run failed: https://github.com/sinonjs/fake-timers/actions/runs/12237218608/job/34132458756

452 passing (514ms)
  8 pending
  2 failing

  1) globally configured browser objects
       correctly instantiates and tears down:
     TypeError: Cannot set property navigator of #<Object> which has only a getter
      at setUpGlobal (integration-test/fake-clock-integration-test.js:7:[28](https://github.com/sinonjs/fake-timers/actions/runs/12237218608/job/34132458756#step:5:29)3)
      at Context.<anonymous> (integration-test/fake-clock-integration-test.js:7:1643)
      at process.processImmediate (node:internal/timers:491:21)

  2) globally configured browser objects
       correctly instantiates and tears down:
     Error: global leak(s) detected: 'window', 'document'
      at Runner.emit (node:events:5[30](https://github.com/sinonjs/fake-timers/actions/runs/12237218608/job/34132458756#step:5:31):35)
      at process.processImmediate (node:internal/timers:491:21)

@fatso83 fatso83 reopened this Dec 9, 2024
@dubzzz
Copy link
Contributor Author

dubzzz commented Dec 9, 2024

Arf strange! Sorry for the inconvenience, I'll have to rework it

@dubzzz
Copy link
Contributor Author

dubzzz commented Dec 9, 2024

Please feel free to revert in case it's blocking you. I'll try to do my best to have something soon

@fatso83
Copy link
Contributor

fatso83 commented Dec 9, 2024

So weird. Works locally:

 npx mocha --timeout 200 test/ integration-test/  -R dot --check-leaks

@fatso83
Copy link
Contributor

fatso83 commented Dec 9, 2024

Ah, works in Node 20. Fails in Node 22.12.0

@fatso83
Copy link
Contributor

fatso83 commented Dec 9, 2024

Sorry, this is not on you. This is from #515 #518

@fatso83
Copy link
Contributor

fatso83 commented Dec 9, 2024

I did not notice this had failed on merge: https://github.com/sinonjs/fake-timers/commits/main/

@fatso83
Copy link
Contributor

fatso83 commented Dec 9, 2024

Found the issue: #518

Sorry for the noise!

@fatso83 fatso83 closed this as completed Dec 9, 2024
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.

2 participants