Fix now() sharing global state between tests
#316
Conversation
Signed-off-by: Sebastian Malton <sebastian@malton.name>
Signed-off-by: Sebastian Malton <sebastian@malton.name>
|
@NaridaL or @mweststrate Sorry to ping you but this PR has been here for a couple months, would I be able to get a review please? |
|
I can take a look this week. |
src/now.ts
Outdated
| const timer = tickers[interval] | ||
| ? tickers[interval] | ||
| : typeof interval === "number" | ||
| ? createIntervalTicker(interval) | ||
| : createAnimationFrameTicker() |
There was a problem hiding this comment.
Correct me if I'm wrong, but tickets[interval] now never gets assigned, ie no caching.
There was a problem hiding this comment.
yes because during testing we don't want caching
There was a problem hiding this comment.
Yes but it's not caching when not testing either.
There was a problem hiding this comment.
Yes, I have updated so it can
src/now.ts
Outdated
| if (!synchronized) { | ||
| if (typeof interval === "number") { | ||
| return createIntervalTicker(interval).current() | ||
| } | ||
|
|
||
| return createAnimationFrameTicker().current() |
There was a problem hiding this comment.
If you look at the docs for fromResource https://github.com/mobxjs/mobx-utils/blob/master/src/from-resource.ts , which this uses, the usual usage is that you define your observable outside a reactive context, and then use it inside the reactive context. utils.now gets around this by caching based on the interval. Without this it kinda breaks, and we unsubscribe/resubscrive to the observable on every autorun execution:
There was a problem hiding this comment.
This may not be a problem as this is designed only for tests.
test/now.js
Outdated
| jest.useFakeTimers("modern") | ||
| jest.setSystemTime(new Date("2015-10-21T07:28:00Z")) | ||
|
|
||
| utils.desynchronizeNowForTests() |
There was a problem hiding this comment.
commenting out this line did not cause the test to fail, even fixing the above bug.
See also comment on original bug.
There was a problem hiding this comment.
Yes I see that now. So I am somehow modifying the behaviour otherwise. The test does fail if I revert all the changes to src/now.ts so the bug does exist.
I guess another solution for us would be for there to be an easy way create IComputedValue<number> for the date
Signed-off-by: Sebastian Malton <sebastian@malton.name>
|
@NaridaL Would you be able to take another look? I decided to switch to a different implementation so that the tests still cache the but only within a single test case. I guess another option would be for the project that I am working on to switch to managing state ourselves and just copy over (or expose here) the |
|
This approach seems much simpler. I've made a couple of minor improvements: renamed it to resetNowInternalState (because the relation to now is currently not visible, and we don't use the "timers" terminology anywhere; and added documentation comments. I can't push to this branch though, (this is why you "allow edits by maintainers" ;) ) |
|
published in 6.0.8 |
|
Thanks |
Signed-off-by: Sebastian Malton sebastian@malton.name
fix #306