Fake timers #315

bpinto · 2021-09-01T17:07:18Z

Depends on #314
Closes #38

mAAdhaTTah · 2021-09-09T02:48:17Z

I haven't done this upgrade because this would be a major version bump, as we're exposing the underlying fake-timers API and I believe it only supports newer versions of node. It's not quite as simple as "bump a few versions", in this case, because all of our dependent packages also then need to get major version bumps, so the whole Kefir ecosystem would need a bump.

bpinto · 2021-09-09T11:21:55Z

Yeah, I think this would require a major version bump as @sinonjs/fake-timers has dropped support for node 8.

Many libraries are dropping support for node 10 already, but since this is only dropping support for node 8 which was last updated on 2019 I personally think the benefits of upgrading our stack vs maintaining support for node 8 are greater.

As a user of kefir and its related tools, I would like to see a bigger community so that more people are testing the code we use every day, I believe one way to achieve that is to constantly update the libraries and keep issues/PRs tidy.

mAAdhaTTah · 2021-09-09T13:44:58Z

As a user of kefir and its related tools, I would like to see a bigger community so that more people are testing the code we use every day, I believe one way to achieve that is to constantly update the libraries and keep issues/PRs tidy.

Fair. I don't use Kefir as much as I used to, but I would be open to refreshing the testing utilities and it's related suite of tools. The only issue is I don't think there's a good reason to release a major version of Kefir, and if we drop Node 8 & 10 support in KTU, then we'll probs need to drop support for (or at least stop testing) those versions, which is a major version bump. At that point, there are a handful of breaking changes we may want to consider as well, and now we're starting to unravel things a bit. So that's why I haven't really bothered with all of this.

bpinto · 2021-09-09T14:32:00Z

In theory, KTU depends on Kefir and not the other way around, so if we released a version bump of KTU, would we necessarily need to do the same with Kefir?

I mean, there is no reason a user using Kefir 3.x needs to use KTU 2.x, they could continue to use KTU 1.x versions.

mAAdhaTTah · 2021-09-09T16:56:48Z

Kefir uses KTU to test itself, so if KTU no longer supported those old node versions, we wouldn't be able to test Kefir on them either, which would effectively drop support for those versions from Kefir.

bpinto · 2021-09-09T17:48:04Z

We keep Kefir on KTU 1.x until we are ready for a major bump there? If we were to drop automated tests for node <8 on Kefir, I'm not sure if we need to do a major version bump. I mean, we wouldn't be breaking the contract it's just that we wouldn't be running automated tests on those older node versions.

mAAdhaTTah · 2021-09-09T18:00:39Z

I mean, we wouldn't be breaking the contract it's just that we wouldn't be running automated tests on those older node versions.

Generally, in other OS projects, dropping testing for a Node version means that version is no longer supported, which would be a breaking change.

bpinto · 2021-09-09T18:32:28Z

That's fair.

Ooops, so I looked into kefir build status and it's already not running tests on node 6 and 8 because of an earlier change I did. 😓

What do you think I should do?

mAAdhaTTah · 2021-09-11T14:02:58Z

I think we have two options:

Leave everything as is. The library works, the supporting libs work, so maybe changes aren't worth the effort.
Embark on a major version overhaul of the lib + ecosystem. This actually sounds worse than it is, especially because there aren't that many planned breaking changes in the core library.

Unfortunately, I don't have a ton of time right now to work on the overhaul but I can pitch in a bit, so you'd be taking on a good amount of the work for #2 if we want to go that route.

Also cc @Macil @rpominov in case either of you have thoughts around this or if either of you are interested in & available to work on this.

bpinto · 2021-09-13T10:05:35Z

As long as I have you mentoring me, I think we can do the changes you think we need for the major bump. I don't know what I'm signing up for yet, but as I said to you in another issue, since I'm using Kefir daily now, I'd like to help it in any way I can.

mAAdhaTTah · 2021-09-20T01:24:04Z

At a high level, here's what I'm thinking:

Update all of the dependencies in KTU, drop old node versions, & make any breaking changes we need to make.
Update the jest & chai packages with the new KTU package, drop old node versions, & make any breaking changes we need to make.
Update Kefir to use the new chai package, drop old node versions, & make any breaking changes.

None of the packages have major breaking changes, but just a couple of odds & ends that I think would be improvements. We can document this on GitHub projects if this is something you're interested in working on.

bpinto · 2021-09-29T18:01:43Z

Okay, so let's start with 1? I believe this is a good start.

Or maybe we could look into #321 first? I think it's ready.

bpinto · 2021-10-01T12:48:01Z

I've rebased this PR now that #314 has been merged, I think this one is ready to be merged as well.

mAAdhaTTah · 2021-10-01T12:50:56Z

src/index.js

-import lolex from 'lolex'
+import * as fakeTimers from '@sinonjs/fake-timers'


Instead of import * as, let's just import install. Maybe this is an over-optimization, but if we only use install and there are other exports we don't use, this will be more tree-shakable.

That's a good idea, I've updated it.

mAAdhaTTah · 2021-10-01T12:51:18Z

package.json

 "@types/node": "^14.0.1",
- "lolex": "^2.0.0"
+ "@types/sinonjs__fake-timers": "^6.0.3"


Does the latest version of fake-timers come w/ types? I think it might.

I'll check the release notes, I don't use TS myself.

I'll be upgrading fake timers to latest version, there have been quite a few releases since I first opened this PR.

In a recent release (8.0.0) types have been removed from the library.

mAAdhaTTah · 2021-10-01T13:04:16Z

I believe the test failure here is the result of a flaky test but do we have any idea why that test sometimes returns 0 and sometimes 1?

bpinto · 2021-10-01T13:05:58Z

I honestly haven't looked deep yet, but this is what I noticed the last time I looked into it.

I think we will need to mock new Date() in order to remove this flakiness.

mAAdhaTTah · 2021-10-01T13:35:35Z

Hmmm, I thought Date was mocked by fake-timers... that's weird.

bpinto · 2021-10-01T18:56:15Z

Okay, so I investigated this further, here is what I found: #335

Closes #38

bpinto · 2021-11-08T16:09:31Z

@mAAdhaTTah now that #335 has been merged, tests are green with this change.

bpinto force-pushed the fake-timers branch 2 times, most recently from e92cc62 to d238917 Compare October 1, 2021 12:46

mAAdhaTTah reviewed Oct 1, 2021

View reviewed changes

bpinto force-pushed the fake-timers branch 2 times, most recently from 5acdcc8 to ce80307 Compare October 1, 2021 12:58

bpinto added 2 commits November 8, 2021 16:07

Switch to @sinonjs/fake-timers

3b4e5dc

Closes #38

package-lock.json

cd0bc8e

bpinto force-pushed the fake-timers branch from ce80307 to cd0bc8e Compare November 8, 2021 16:07

mAAdhaTTah approved these changes Nov 9, 2021

View reviewed changes

mAAdhaTTah merged commit c5364d4 into kefirjs:master Nov 9, 2021

bpinto deleted the fake-timers branch November 9, 2021 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fake timers #315

Fake timers #315

bpinto commented Sep 1, 2021

mAAdhaTTah commented Sep 9, 2021 •

edited

Loading

bpinto commented Sep 9, 2021

mAAdhaTTah commented Sep 9, 2021

bpinto commented Sep 9, 2021 •

edited

Loading

mAAdhaTTah commented Sep 9, 2021

bpinto commented Sep 9, 2021

mAAdhaTTah commented Sep 9, 2021

bpinto commented Sep 9, 2021

mAAdhaTTah commented Sep 11, 2021

bpinto commented Sep 13, 2021

mAAdhaTTah commented Sep 20, 2021

bpinto commented Sep 29, 2021 •

edited

Loading

bpinto commented Oct 1, 2021

mAAdhaTTah Oct 1, 2021

bpinto Oct 1, 2021

mAAdhaTTah Oct 1, 2021

bpinto Oct 1, 2021

bpinto Oct 1, 2021

mAAdhaTTah commented Oct 1, 2021

bpinto commented Oct 1, 2021 •

edited

Loading

mAAdhaTTah commented Oct 1, 2021

bpinto commented Oct 1, 2021

bpinto commented Nov 8, 2021

		import lolex from 'lolex'
		import * as fakeTimers from '@sinonjs/fake-timers'

Fake timers #315

Fake timers #315

Conversation

bpinto commented Sep 1, 2021

mAAdhaTTah commented Sep 9, 2021 • edited Loading

bpinto commented Sep 9, 2021

mAAdhaTTah commented Sep 9, 2021

bpinto commented Sep 9, 2021 • edited Loading

mAAdhaTTah commented Sep 9, 2021

bpinto commented Sep 9, 2021

mAAdhaTTah commented Sep 9, 2021

bpinto commented Sep 9, 2021

mAAdhaTTah commented Sep 11, 2021

bpinto commented Sep 13, 2021

mAAdhaTTah commented Sep 20, 2021

bpinto commented Sep 29, 2021 • edited Loading

bpinto commented Oct 1, 2021

mAAdhaTTah Oct 1, 2021

Choose a reason for hiding this comment

bpinto Oct 1, 2021

Choose a reason for hiding this comment

mAAdhaTTah Oct 1, 2021

Choose a reason for hiding this comment

bpinto Oct 1, 2021

Choose a reason for hiding this comment

bpinto Oct 1, 2021

Choose a reason for hiding this comment

mAAdhaTTah commented Oct 1, 2021

bpinto commented Oct 1, 2021 • edited Loading

mAAdhaTTah commented Oct 1, 2021

bpinto commented Oct 1, 2021

bpinto commented Nov 8, 2021

mAAdhaTTah commented Sep 9, 2021 •

edited

Loading

bpinto commented Sep 9, 2021 •

edited

Loading

bpinto commented Sep 29, 2021 •

edited

Loading

bpinto commented Oct 1, 2021 •

edited

Loading