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.

Sign up for GitHub

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

chore: type TestScheduler's testRunners object #9804

Merged
merged 1 commit into from
Apr 13, 2020
Merged

chore: type TestScheduler's testRunners object #9804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/jest-core/src/TestScheduler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ export default class TestScheduler {
showStatus: !runInBand,
});

const testRunners = Object.create(null);
const testRunners: {[key: string]: TestRunner} = Object.create(null);
contexts.forEach(({config}) => {
if (!testRunners[config.runner]) {
const Runner: typeof TestRunner = require(config.runner);
Expand Down
1 change: 1 addition & 0 deletions packages/jest-runner/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ namespace TestRunner {
class TestRunner {
private _globalConfig: Config.GlobalConfig;
private _context: JestTestRunnerContext;
readonly isSerial?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sounds good 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Can it be in jest-runner? And yeah, a new package is fine if it doesn't fit anywhere else, although lifting some types out (like I've done in #9747 and #9749) to break cycles might be enough.


constructor(
globalConfig: Config.GlobalConfig,
Expand Down
1 change: 0 additions & 1 deletion packages/jest-runner/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ export type WatcherState = {
};
export interface TestWatcher extends EventEmitter {
state: WatcherState;
new ({isWatchMode}: {isWatchMode: boolean}): TestWatcher;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

How would we correctly type this so TS knows we'll new the function/class they pass us with {isWatchMode: boolean}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Should we make it an abstract class? That's what I would do in Java, and maybe correct here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An interface looks right to me. The TestWatcher constructor is private. Consumers of this type only ever see TestWatcher instances.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

meh, it's using export = and already has a TODO it should be moved

setState(state: WatcherState): void;
isInterrupted(): boolean;
isWatchMode(): boolean;
Expand Down