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

Jump to bottom

Use property initializer syntax in Jest codebase #7881

Closed
wants to merge 8 commits into from
Closed

Use property initializer syntax in Jest codebase #7881

wants to merge 8 commits into from

Conversation

vitkarpov
Copy link
Contributor

Summary

This PR addresses #7846

@vitkarpov
Copy link
Contributor Author

vitkarpov commented Feb 13, 2019 •
edited
Loading

I think something's wrong with my yarn since I got such a long yarn.lock diff 🤔

UPD Updated to the latest yarn version which doesn't strip integrity fields.

@vitkarpov
Copy link
Contributor Author

vitkarpov commented Feb 13, 2019 •
edited
Loading

I don't have toMatchSnapshot errors locally which I see in CI 🤔

Instead, I see 1 test fails locally in the branch but passes in master, it's about error message in CustomConsole; one comes like Node.js is running a file and another like the code runs in Node.js REPL, that's weird 😃

SimenB
SimenB approved these changes Feb 13, 2019
View reviewed changes
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!

regarding test failures, you've done yarn build after your changes?

packages/jest-watcher/src/lib/Prompt.ts Outdated

enter(
onChange: (pattern: string, options: ScrollOptions) => void,
onSuccess: () => void,
onCancel: () => void,
) {
this._entering = true;
Copy link
Member

Choose a reason for hiding this comment

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

these should be kept, since we need to reset the values in enter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to _resetState function → 68ce8b4
and added a test for that.

I changed this code and nothing didn't tell me that I can't, seems like "a lack of coverage", doesn't it? WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, probably need to expand an e2e test to activate a plugin, type something, go out of the plugin, go back in, then assert that the prompt is empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, do you think it should be an e2e test? (I've added a unit one, probably doesn't make sense then)

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 guess it's better to address separately.

@vitkarpov
Copy link
Contributor Author

regarding test failures, you've done yarn build after your changes?

Nice catch :-)

I run it this way now:

$ yarn build
$ yarn test

Everything's okay except the one test:

packages/jest-util/src/__tests__/console.test.ts
  ● CustomConsole › assert › log the assertion error when the assertion is falsy

    expect(received).toMatch(expected)

    Expected value to match:
      "false == true"
    Received:
      "AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
    
      error.toString()
    
    "

      at Object.toMatch (packages/jest-util/src/__tests__/console.test.ts:41:23)

Don't know why it's happening so far but need to figure out, if I check out on master everything's okay.

@vitkarpov
Copy link
Contributor Author

Everything's okay except the one test:

And it's not even e2e test, so "building" doesn't make any difference.

@vitkarpov
Copy link
Contributor Author

Everything's okay except the one test:

Oh, man, you're right. I just didn't wait until all the tests finish 😱 I'm getting the same results as in CI now. My bad.

thymikee
thymikee reviewed Feb 13, 2019
View reviewed changes
packages/jest-cli/src/plugins/quit.js
super(options);
this.isInternal = true;
}
isInternal: true = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be boolean instead of true, here and in other places

Copy link
Member

Choose a reason for hiding this comment

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

In here it's fine, I'd say. It can never be false, thus true is just stricter. The parent class (BaseWatchPlugin) needs to be boolean, though

vitkarpov added 8 commits February 13, 2019 18:21
chore: Add class props babel plugin
d2c3c1e
refactor: Use class props in FakeTimers
f735175
refactor: Get rid of magic numbers
9bb28b8
refactor: Use class props in jest-cli
398f730
refactor: Use class props in jest-watchers
a076e85
refactor: Remove redundant constructors
1cb54b7
chore: Update snapshots
6b2817a
fix: inconsistent error messages with class props
2d72c60 
After adding `@babel/plugin-proposal-class-properties' the `error` to string coercions start to give a different message:

```
AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:

      error.toString()
```

It's weird since we can add `console.log` inside the custom console `assert` method and the correct message gets back.
In case of autogenerated message, let's specify the exact one since we have all the data within error object for that.
SimenB
SimenB reviewed Feb 13, 2019
View reviewed changes
packages/jest-cli/src/plugins/update_snapshots_interactive.js
@@ -25,7 +25,6 @@ class UpdateSnapshotInteractivePlugin extends BaseWatchPlugin {
super(options);
this._failedSnapshotTestAssertions = [];
Copy link
Member

Choose a reason for hiding this comment

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

this as well?

Copy link

Choose a reason for hiding this comment

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

You can move all of these to properties, even new SnapshotInteractiveMode(this._stdout);.

@vitkarpov
Copy link
Contributor Author

This 2d72c60 concerns me the most. TBH, I don't get the root cause.

@vitkarpov
Copy link
Contributor Author

Gosh, there's no 2 days in a row without getting conflicting files 😃

@vitkarpov
Copy link
Contributor Author

I think it's better to split this PR marking more incremental changes, I believe the codebase's changing due to migration to TS and that's why there's a lot of conflicts.

@vitkarpov vitkarpov closed this Feb 17, 2019
@vitkarpov vitkarpov deleted the issue-7846 branch February 17, 2019 09:13
@SimenB SimenB mentioned this pull request Mar 7, 2019
Closed
@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 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@milesj milesj milesj left review comments

@SimenB SimenB SimenB approved these changes

@thymikee thymikee thymikee left review comments

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vitkarpov @milesj @SimenB @thymikee @facebook-github-bot