chore: type TestScheduler's testRunners object #9804
Conversation
|
Hi @JoelEinbinder! Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
| @@ -44,6 +44,7 @@ namespace TestRunner { | |||
| class TestRunner { | |||
| private _globalConfig: Config.GlobalConfig; | |||
| private _context: JestTestRunnerContext; | |||
| readonly isSerial?: boolean; | |||
There was a problem hiding this comment.
The jest-runner class right now is the source of truth for all test runners. This means it needs to have an isSerial property for the test scheduler to use it.
In the future, I think TestRunner should become an interface implemented by JestTestRunner.
There was a problem hiding this comment.
I tried and failed to do the interface patch. The runner interface depends on jest-haste-map, which depends on jest/@types. So I can't put it in types. And it needs to be used by jest-runner, which is depended on by @jest/core, so I can't put it in @jest/core. I'm not sure where it should go without creating a cycle. Can I create a new package? How does that work?
| @@ -63,7 +63,6 @@ export type WatcherState = { | |||
| }; | |||
| export interface TestWatcher extends EventEmitter { | |||
| state: WatcherState; | |||
| new ({isWatchMode}: {isWatchMode: boolean}): TestWatcher; | |||
There was a problem hiding this comment.
I believe this type was actually wrong. It implies you can do new testWatcher({isWatchMode:true}), rather than the intent of new TestWatcher({isWatchMode:true}). Interfaces don't have constructors.
There was a problem hiding this comment.
Some of the examples here have constructors? https://www.typescriptlang.org/docs/handbook/interfaces.html#difference-between-the-static-and-instance-sides-of-classes But it seems like we have to separate them out, or something - I might need to read up on it some more
There was a problem hiding this comment.
How would we correctly type this so TS knows we'll new the function/class they pass us with {isWatchMode: boolean}?
There was a problem hiding this comment.
Something might have changed? This was my script to try it out
interface Hey {
new(foo: number): Hey;
woo: number;
}
function testFunction(hey: Hey) {
new hey(123);
}There was a problem hiding this comment.
Ah, yes nothing changed. This is consistent with the docs link you sent. Notice they are defining the interface for the constructor itself, not the Clock.
interface ClockConstructor {
new (hour: number, minute: number): ClockInterface;
}
interface ClockInterface {
tick(): void;
}We could add a TestWatcherConstructor interface, but that doesn't seem to represent anything public.
There was a problem hiding this comment.
Yeah, it's definitely wrong now, and just deleting is an improvement. The docs say something about the static part (like the constructor) needing to be typed separately from the "instance" functions.
There was a problem hiding this comment.
Should we make it an abstract class? That's what I would do in Java, and maybe correct here as well?
There was a problem hiding this comment.
An interface looks right to me. The TestWatcher constructor is private. Consumers of this type only ever see TestWatcher instances.
There was a problem hiding this comment.
The implementation lives here: https://github.com/facebook/jest/blob/c83051789cc60789f2c575374270236e3a428879/packages/jest-core/src/TestWatcher.ts
Maybe we should just move that to the jest-runner package rather than duplicating its interface in an interface? We even import it from core in a test, so seems reasonable: https://github.com/facebook/jest/blob/4a5bac1459ed14b53262935b0bc0d6d6613b2000/packages/jest-runner/src/__tests__/testRunner.test.ts#L9
There was a problem hiding this comment.
meh, it's using export = and already has a TODO it should be moved
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
SimenB
changed the title
|
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. |
Summary
There was an
anyobject containing all of the TestRunners. I gave it a type.Test plan
Typescript build succeeded.