Jest: Add setSystemTime and getRealSystemTime fn types #45496

liamfd · 2020-06-15T16:22:20Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change. (Run with npm test.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes:
- https://jestjs.io/docs/en/jest-object#jestsetsystemtime, https://jestjs.io/docs/en/jest-object#jestgetrealsystemtime
- These are based on Jest's internal types, but I've got a PR open to update them, this PR assumes that update has happened on their end: setSystemTime and getRealSystemTime type and doc update jestjs/jest#10169 (you can see the internal types I'm using in that PR).
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
- Not sure if I'll need to make a change here - these types are valid and work at runtime for the current version of jest. The PR I have open for essentially just updates jest's internal types and docs to reflect that, so I'm not sure if it would be better to update the version reference, given that it should be compatible with both. Regardless, the current header says 26.0 and if my jest PR is accepted I would assume it would be a patch release.
Include tests for your changes
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

typescript-bot · 2020-06-15T16:22:24Z

@liamfd Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — keep an eye on this comment as I'll be updating it with information as things progress.

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
❌ Most recent commit is approved by DT maintainers

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 45496,
  "author": "liamfd",
  "owners": [
    "NoHomey",
    "jwbay",
    "asvetliakov",
    "alexjoverm",
    "epicallan",
    "ikatyang",
    "wsmd",
    "JamieMason",
    "douglasduteil",
    "ahnpnl",
    "joshuakgoldberg",
    "UselessPickles",
    "r3nya",
    "hotell",
    "sebald",
    "andys8",
    "antoinebrault",
    "favna",
    "gstamac",
    "ExE-Boss",
    "quassnoi",
    "Belco90",
    "tonyhallett",
    "ycmjason",
    "devanshj",
    "pawfa",
    "regevbr",
    "mskelton"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "725c00d",
  "headCommitOid": "725c00d5da5651ae54f3b98679d54d28f8279751",
  "mergeIsRequested": false,
  "stalenessInDays": 7,
  "lastCommitDate": "2020-06-16T12:56:12.000Z",
  "lastCommentDate": "2020-06-16T12:56:42.000Z",
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45496/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": true,
  "popularityLevel": "Critical",
  "anyPackageIsNew": false,
  "packages": [
    "jest"
  ],
  "files": [
    {
      "filePath": "types/jest/index.d.ts",
      "kind": "definition",
      "package": "jest"
    },
    {
      "filePath": "types/jest/jest-tests.ts",
      "kind": "test",
      "package": "jest"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-06-17T15:24:58.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

typescript-bot · 2020-06-15T16:22:26Z

🔔 @NoHomey @jwbay @asvetliakov @alexjoverm @epicallan @ikatyang @wsmd @JamieMason @douglasduteil @ahnpnl @JoshuaKGoldberg @UselessPickles @r3nya @Hotell @sebald @andys8 @antoinebrault @favna @gstamac @ExE-Boss @quassnoi @Belco90 @tonyhallett @ycmjason @devanshj @pawfa @regevbr @mskelton - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

liamfd · 2020-06-15T16:24:02Z

types/jest/index.d.ts

+    /**
+     * Set the current system time used by fake timers. Simulates a user
+     * changing the system clock while your program is running. It affects the
+     * current time but it does not in itself cause e.g. timers to fire; they
+     * will fire exactly as they would have done without the call to
+     * jest.setSystemTime().
+     *
+     * > Note: This function is only available when using modern fake timers
+     * > implementation
+     */


I tried to follow the comment patterns I observed elsewhere in this file, I just copy pasted this comment and the one below straight out of jest's docs.

liamfd · 2020-06-15T16:25:57Z

types/jest/index.d.ts

+     * > Note: This function is only available when using modern fake timers
+     * > implementation
+     */
+    function setSystemTime(now?: number | Date): void;


As mentioned in the PR description, jest currently types this as:

function setSystemTime(now?: number): void;

I have a PR open to expand it to support Date, given that under the hood setSystemTime is calling a function that accepts both. If they do not want to support Date, I will remove it here.

Perhaps we can hold off on adding support for Date until the other PR is merged to keep things consistent and work as intended? Once that other PR is merged and a newer version of Jest is released with the Date support, we can then update the types here to reflect that latest changes?

Sounds good to me

wsmd

Thanks for the PR @liamfd! This looks good overall. I left one comment suggesting that we hold off on adding support for Date for now.

wsmd · 2020-06-15T18:05:42Z

types/jest/index.d.ts

+     * > Note: This function is only available when using modern fake timers
+     * > implementation
+     */
+    function setSystemTime(now?: number | Date): void;


Perhaps we can hold off on adding support for Date until the other PR is merged to keep things consistent and work as intended? Once that other PR is merged and a newer version of Jest is released with the Date support, we can then update the types here to reflect that latest changes?

typescript-bot · 2020-06-15T18:08:55Z

@liamfd One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

typescript-bot · 2020-06-15T18:26:31Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#45496	diff
Batch compilation
Memory usage (MiB)	83.4	81.8	-1.9%
Type count	14710	14712	0%
Assignability cache size	4812	4814	0%

Language service
Samples taken	2563	2578	+1%
Identifiers in tests	2563	2578	+1%
`getCompletionsAtPosition`
Mean duration (ms)	345.4	349.0	+1.0%
Mean CV	8.3%	8.2%
Worst duration (ms)	438.8	476.9	+8.7%
Worst identifier	each	toHaveProperty
`getQuickInfoAtPosition`
Mean duration (ms)	347.1	350.1	+0.9%
Mean CV	8.7%	8.6%	-1.2%
Worst duration (ms)	461.4	466.3	+1.1%
Worst identifier	fn	each

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

…upported

typescript-bot · 2020-06-16T12:59:36Z

@wsmd Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

wsmd

Thanks @liamfd!

typescript-bot · 2020-06-24T11:49:43Z

I just published @types/[email protected] to npm.

…ed#45496) * feat: add setSystemTime and getRealSystemTime to jest types * test: jest setSystemTime and getRealSystemTime * feat: remove Date from setSystemTime types while not yet officially supported

liamfd added 2 commits June 15, 2020 12:06

feat: add setSystemTime and getRealSystemTime to jest types

45f6644

test: jest setSystemTime and getRealSystemTime

72fda94

typescript-bot added Where is GH Actions? GH Actions didn't give a response to this PR Critical package labels Jun 15, 2020

typescript-bot removed the Where is GH Actions? GH Actions didn't give a response to this PR label Jun 15, 2020

liamfd commented Jun 15, 2020

View reviewed changes

wsmd suggested changes Jun 15, 2020

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jun 15, 2020

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Jun 15, 2020

feat: remove Date from setSystemTime types while not yet officially s…

725c00d

…upported

liamfd force-pushed the feat/add-new-jest-time-fns branch from 8d9e2d8 to 725c00d Compare June 16, 2020 12:56

typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Jun 16, 2020

wsmd approved these changes Jun 17, 2020

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Jun 17, 2020

elibarzilay merged commit c98e50d into DefinitelyTyped:master Jun 24, 2020

liamfd mentioned this pull request Jun 24, 2020

Jest: add Date to setSystemTime param types #45685

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jest: Add setSystemTime and getRealSystemTime fn types #45496

Jest: Add setSystemTime and getRealSystemTime fn types #45496

liamfd commented Jun 15, 2020

typescript-bot commented Jun 15, 2020 •

edited

Loading

typescript-bot commented Jun 15, 2020

liamfd Jun 15, 2020

liamfd Jun 15, 2020 •

edited

Loading

wsmd Jun 15, 2020

liamfd Jun 16, 2020

wsmd left a comment •

edited

Loading

wsmd Jun 15, 2020

typescript-bot commented Jun 15, 2020

typescript-bot commented Jun 15, 2020

typescript-bot commented Jun 16, 2020

wsmd left a comment

typescript-bot commented Jun 24, 2020

Jest: Add setSystemTime and getRealSystemTime fn types #45496

Jest: Add setSystemTime and getRealSystemTime fn types #45496

Conversation

liamfd commented Jun 15, 2020

typescript-bot commented Jun 15, 2020 • edited Loading

Code Reviews

Status

typescript-bot commented Jun 15, 2020

liamfd Jun 15, 2020

Choose a reason for hiding this comment

liamfd Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

wsmd Jun 15, 2020

Choose a reason for hiding this comment

liamfd Jun 16, 2020

Choose a reason for hiding this comment

wsmd left a comment • edited Loading

Choose a reason for hiding this comment

wsmd Jun 15, 2020

Choose a reason for hiding this comment

typescript-bot commented Jun 15, 2020

typescript-bot commented Jun 15, 2020

typescript-bot commented Jun 16, 2020

wsmd left a comment

Choose a reason for hiding this comment

typescript-bot commented Jun 24, 2020

typescript-bot commented Jun 15, 2020 •

edited

Loading

liamfd Jun 15, 2020 •

edited

Loading

wsmd left a comment •

edited

Loading