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

Upgrade Jest and use TS for Jest config #97

Merged
merged 2 commits into from
Apr 6, 2022
Merged

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Apr 4, 2022

v27 is the latest version of Jest and introduces these major changes:

  • node is now the default environment instead of jsdom. This
    results in faster runtimes.
  • The runner has been completely rewritten as jest-circus which is
    also the default, replacing jest-jasmine2.
    • This should have no impact on us as we don't rely on any
      Jasmine-specific APIs, but is nice to know.
  • The "modern" version of Fake Timers (enabled via
    jest.useFakeTimers("modern")) is now the default.

More info here: https://jestjs.io/blog/2021/05/25/jest-27

Additionally, to ensure that we make use of all of the latest and
greatest, this PR regenerates the Jest config file (as
jest.config.ts). This has the added improvement that each option is
now commented so we know exactly what each one does.

v27 is the latest version of Jest and introduces these major changes:

* `node` is now the default environment instead of `jsdom`. This
  results in faster runtimes.
* The runner has been completely rewritten as `jest-circus` which is
  also the default, replacing `jest-jasmine2`.
  * This should have no impact on us as we don't rely on any
    Jasmine-specific APIs, but is nice to know.
* The "modern" version of Fake Timers (enabled via
  `jest.useFakeTimers("modern")`) is now the default.

More info here: <https://jestjs.io/blog/2021/05/25/jest-27>

Additionally, to ensure that we make use of all of the latest and
greatest, this PR regenerates the Jest config file (as
`jest.config.ts`). This has the added improvement that each option is
now commented so we know exactly what each one does.
@mcmire mcmire requested a review from a team as a code owner April 4, 2022 19:56
jest.config.ts Outdated Show resolved Hide resolved
jest.config.ts Outdated Show resolved Hide resolved
jest.config.ts Outdated Show resolved Hide resolved
@Gudahtt
Copy link
Member

Gudahtt commented Apr 6, 2022

I started going through the new config, but it looks like most of our old config was discarded. Perhaps by mistake? Also lots of lint errors.

// ],

// An array of file extensions your modules use
// moduleFileExtensions: [
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 default value matches our previously provided value, so I opted to leave the default.

// testLocationInResults: false,

// The glob patterns Jest uses to detect test files
// testMatch: [
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 default value covers more than the pattern we provided, but it also includes that pattern. Is there a reason why we want to override this? Was it just to communicate that test files ought to 1) be named *.test.ts and 2) be colocated with implementation files?

Copy link
Member

@Gudahtt Gudahtt Apr 6, 2022

Choose a reason for hiding this comment

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

I'm guessing that it's because it used to diverge from the standard. The default seems fine to me.

@mcmire
Copy link
Contributor Author

mcmire commented Apr 6, 2022

@Gudahtt Apologies for the rush job on this, just wanted to make a PR based on the changes I'd made to eth-json-rpc-infura before I forgot. Should be better now.

files: ['jest.config.ts'],
rules: {
// TODO: Migrate to our shared config
'import/no-anonymous-default-export': 'off',
Copy link
Member

@Gudahtt Gudahtt Apr 6, 2022

Choose a reason for hiding this comment

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

👀

I was curious what this rule was for so I just looked up the original PR where it was added. Apparently the advantage is that it "improves the greppability of the codebase". That is, when searching for an identifier, sometimes it can be hard to find what you're looking for if that identifier is only in the filename and not represented in the file itself.

Copy link
Member

Choose a reason for hiding this comment

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

I have definitely had that problem myself, so in general this rule seems beneficial to me. But it does seem out-of-place when applied to a top-level config file.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@mcmire mcmire merged commit df1c004 into main Apr 6, 2022
@mcmire mcmire deleted the upgrade-jest-and-use-ts branch April 6, 2022 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants