search

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
797 stars 104 forks source link

Copy all property descriptors to installed methods #349

Closed kevinoid closed 3 years ago

kevinoid commented 3 years ago

Purpose (TL;DR) - mandatory

Fix #347 by copying symbol own-properties to installed methods.

Background (Problem in detail) - optional

The for...in loop used to copy properties only copies string own-properties to installed methods, which omits util.promisify.custom necessary for util.promisify support on Node as added in #292.

Solution - optional

This PR copies all own-property descriptors to get both Symbol own-properties and to copy any getters/setters that may be added in the future.

If there's a reason getters/setters should not be copied, let me know and I can add an Object.assign polyfill to sinonjs/commons and use that instead.

Thanks for considering, Kevin

Fixes: #347

codecov[bot] commented 3 years ago

Codecov Report

Merging #349 (d7b3e11) into master (bdf8984) will decrease coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
- Coverage   93.15%   93.12%   -0.03%     
==========================================
  Files           1        1              
  Lines         555      553       -2     
==========================================
- Hits          517      515       -2     
  Misses         38       38
Flag Coverage Δ
unit 93.12% <100.00%> (-0.03%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/fake-timers-src.js 93.12% <100.00%> (-0.03%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bdf8984...d7b3e11. Read the comment docs.

fatso83 commented 3 years ago

I don't know why this has been dormant for five months, but I see @benjamingr approved it, so I guess it's good. Just merge it the next time, Benjamin, unless there's something specific you want a second pair of eyes on (in which case you just ping that someone) 🙂