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

fix: support timers getting stubbed #1125

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

jonnycornwell
Copy link
Contributor

  • If using a library to stub timers it interferes with the timing
    functions used by Suites and Tests
  • Tests were written using TDD

@jonnycornwell
Copy link
Contributor Author

Not sure what went on there in the build, did run the tests on node, chrome and firefox locally. Also if you want me to remove the api.json changes let me know

Copy link
Member

@jason0x43 jason0x43 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable. I just have a couple of API requests.

@@ -1028,3 +1028,10 @@ export function errorToJSON(error?: InternError): InternError | undefined {
...(showDiff ? { actual, expected } : {})
};
}

export const originalTimers = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we export these from a new lib/common/time module, with a comment at the top explaining why we need it? Also, rather than dateNow, how about just now? Then we could do

import { setTimeout, now } from './common/time';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is much nicer and changes have been made

@jonnycornwell jonnycornwell force-pushed the support_stubbed_timers branch from 3fe7075 to a0ea32e Compare April 1, 2020 08:14
- If using a library to stub timers it interferes with the timing
functions used by Suites and Tests
- Tests were written using TDD
@jonnycornwell jonnycornwell force-pushed the support_stubbed_timers branch from a0ea32e to 1c670fd Compare April 1, 2020 08:20
@codecov-io
Copy link

codecov-io commented Apr 1, 2020

Codecov Report

Merging #1125 into master will increase coverage by 0.25%.
The diff coverage is 94.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1125      +/-   ##
==========================================
+ Coverage   57.44%   57.69%   +0.25%     
==========================================
  Files          95       96       +1     
  Lines        8909     8962      +53     
  Branches     2046     2059      +13     
==========================================
+ Hits         5118     5171      +53     
  Misses       3791     3791              
Impacted Files Coverage Δ
src/core/bin/intern.ts 26.47% <33.33%> (+1.11%) ⬆️
src/core/lib/node/util.ts 27.55% <50.00%> (+0.46%) ⬆️
src/core/lib/Suite.ts 98.34% <100.00%> (+<0.01%) ⬆️
src/core/lib/Test.ts 98.43% <100.00%> (-0.23%) ⬇️
src/core/lib/common/time.ts 100.00% <100.00%> (ø)
src/core/lib/executors/Node.ts 90.35% <100.00%> (+0.01%) ⬆️
src/tunnels/BrowserStackTunnel.ts 81.41% <0.00%> (-0.89%) ⬇️

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 8fb1208...1c670fd. Read the comment docs.

@jason0x43 jason0x43 merged commit ad7cd35 into theintern:master Apr 6, 2020
@jonnycornwell
Copy link
Contributor Author

@jason0x43 Is there any chance of getting this change into the next 4.8.X release?

@jason0x43
Copy link
Member

Yes, that should be doable

@jonnycornwell
Copy link
Contributor Author

Great, thanks!

jason0x43 added a commit that referenced this pull request Apr 21, 2020
jason0x43 added a commit that referenced this pull request Apr 21, 2020
jason0x43 added a commit that referenced this pull request May 13, 2020
jason0x43 added a commit that referenced this pull request Jul 21, 2020
jason0x43 added a commit that referenced this pull request Aug 10, 2020
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 this pull request may close these issues.

3 participants