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

setSystemTime and getRealSystemTime type and doc update #10169

Merged
merged 5 commits into from
Jun 16, 2020

Conversation

liamfd
Copy link
Contributor

@liamfd liamfd commented Jun 15, 2020

Summary

This PR does two (related) things.

Update setSystemTime now param types to allow Date

The internal types for setSystemTime are currently now?: number, but in using jest I found passing a Date also worked and was a bit more ergonomic in some cases. For example:

    // if Date is not in the union, this errors:
    jest.setSystemTime(new Date('2020-06-15T00:00:00.933Z'));
    
    // you'd have to do:
    jest.setSystemTime(new Date('2020-06-15T00:00:00.933Z').getTime());

The types for the underlying this._clock.setSystemTime call in jest-fake-timers/src/modernFakeTimers.ts support Date, so echoing that in setSystemTime seemed reasonable. I was looking into this while working on a PR to add types for setSystemTime and getRealSystemTime to DefinitelyTyped, and wanted to make sure that those types matched jest's codebase's types and the intended usage.

Update setSystemTime and getRealSystemTime docs

  1. The documentation for setSystemTime did not directly document its params, so they took me as a user a bit of time to figure out. I wasn't sure what the format for these should be. advanceTimersToNextTimer just has jest.advanceTimersToNextTimer(steps) (no types), whereas useFakeTimers has jest.useFakeTimers(implementation?: 'modern' | 'legacy'). I erred on the side of adding a type to the param, the union members are native and I think the types would be useful to both js and ts folks.
  2. The documentation for setSystemTime and getRealSystemTime both have a preceding . char, which I removed. I'm not sure if this was intentional, but the other functions/headers in that doc do not have one and I didn't see any reference to them being experimental or anything (aside from requiring the "modern" timers).

Test plan

For the new type, I added test cases for setSystemTime(now: Date) wherever I saw test cases for setSystemTime(now: number). I ran the tests locally (albeit without mercurial installed, which didn't seem to affect these particular files).

I took a look at the doc changes to make sure they looked alright. They look like:
image

Whereas on the live site, they look like:
image

@liamfd liamfd changed the title Fix/system time types docs setSystemTime and getRealSystemTime type and doc update Jun 15, 2020
@liamfd
Copy link
Contributor Author

liamfd commented Jun 15, 2020

I wasn't sure if Date not being present in the types or the preceding periods were intentional, but I figured I'd err on the side of updating them just in case.

I'm happy to do any cleanup necessary on changes in this PR that aren't desirable.

Also, thanks for these new functions!


expect(Date.now()).toBe(0);

jest.setSystemTime(1000);
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't use a Date, but a number like above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 it does indeed, thank you.


expect(Date.now()).toBe(0);

jest.setSystemTime(1000);
Copy link
Member

Choose a reason for hiding this comment

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

same here

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks! I think this only needs a tweak to the tests and it's good to go 👍

@liamfd
Copy link
Contributor Author

liamfd commented Jun 16, 2020

@SimenB thanks for you review, I've pushed an update to those tests!

@@ -647,13 +647,13 @@ This means, if any timers have been scheduled (but have not yet executed), they

Returns the number of fake timers still left to run.

### `.jest.setSystemTime()`
### `jest.setSystemTime(now?: number | Date)`
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh, seems I'm blind 😅 Thanks!

@SimenB SimenB merged commit 7192a3e into jestjs:master Jun 16, 2020
@liamfd liamfd deleted the fix/system-time-types-docs branch June 16, 2020 15:12
@liamfd liamfd restored the fix/system-time-types-docs branch June 16, 2020 15:12
sauravhiremath pushed a commit to MLH-Fellowship/jest that referenced this pull request Jul 20, 2020
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
@liamfd liamfd deleted the fix/system-time-types-docs branch May 11, 2021 19:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants