Skip to content

Upgrade Jest and use TS for Jest config#97

Merged
mcmire merged 2 commits into
mainfrom
upgrade-jest-and-use-ts
Apr 6, 2022
Merged

Upgrade Jest and use TS for Jest config#97
mcmire merged 2 commits into
mainfrom
upgrade-jest-and-use-ts

Conversation

@mcmire
Copy link
Copy Markdown
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
Comment thread jest.config.ts Outdated
Comment thread jest.config.ts Outdated
Comment thread jest.config.ts Outdated
@Gudahtt
Copy link
Copy Markdown
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.

Comment thread jest.config.ts
// ],

// An array of file extensions your modules use
// moduleFileExtensions: [
Copy link
Copy Markdown
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.

Comment thread jest.config.ts
// testLocationInResults: false,

// The glob patterns Jest uses to detect test files
// testMatch: [
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread .eslintrc.js
files: ['jest.config.ts'],
rules: {
// TODO: Migrate to our shared config
'import/no-anonymous-default-export': 'off',
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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