ui: Refactor Assertions to use Jest Expect #82860
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
nathanstilwell
changed the title
nathanstilwell
changed the title
matthewtodd
approved these changes
Jul 13, 2022
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 2 of 2 files at r5, 59 of 59 files at r6, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nathanstilwell)
Adjusted Jest config for DB Console
-----------------------------------
To account for the many types of assets in the DB Console application,
added a `moduleNameMapper` block with entries for image files and css.
These entries were taken and modified from the Cluster UI jest config to
build on an already working solution. To account for the very
specialized way that DB Console overlays paths for OSS and CCL builds
(see "CCL Build" section of /pkg/ui/ README), I included the `paths`
section of the `tsconfig.json` via a `pathsToModuleNameMapper` utility
from `ts-jest`. To further accomodate the CCL source paths, this path
was included in the `roots` property.
Since `enzyme` is currently used for DOM testing, I set the
`testEnvironment` to "enzyme" as well as included some enzyme related
files to `setupFilesAfterEnv`.
Jest does not use Webpack to build the app before running tests and so
files need to be "transformed" before running on Node.js. The `transform`
section of the config specifies which transformer should be used for
which files. All .ts and .tsx files are transformed with `ts-jest` (this
is the bulk of the app), and .js files are transformed with
`babel-jest`. This is specifically for proto files in `src/js/protos`
which are generated, but use `import` statements which are not supported
by Node.js[1] in the same way we using them via babel.
Adjusted TS Config for DB Console
---------------------------------
I changed the `jsx` property from "preserve" to "react". Perhaps
something else in our webpack config is transforming JSX besides the
Typescript compiler, but for Jest test Node.js can't execute jsx. This
property will have the Typescript compiler convert jsx syntax to
`React.createElement` calls.
Beyond some adjustments to the `paths` entry, the other addition here is
setting `esModuleInterop` to "true". This allows namespace imports
(example: `import * as React from "react"`) to be correctly transformed
into CommonJS (import react = require("react"))
Renaming .babelrc to babel.config.js
------------------------------------
I thought at first that .babelrc had been deprecated in Babel version 7,
but it turns out that Babel supports both formats, but treats them
different in terms of priority when merging configurations[2]. It seems
the convention is to use `babel.config.js` for project wide
configuration and `.babelrc` for specific overrides for individual
modules. I again took the babel.config.js from Cluster UI as a starting
place. Besides needing some of the included plugins, Jest specifically
needs the `@babel/plugin-transform-modules-commonjs`
[1]: Node.js supports ECMAScript modules since version 14, but you must
specify `"type": "module"` in the local package.json or use `*.mjs`
extensions. Neither of these are practical when transpiling. By default
Node.js still uses CommonJS modules.
[2]: see https://babeljs.io/docs/en/configuration#print-effective-configs
Release note: None
== general ==
Added a mock for `window.fetch` (which does not seem to exist in JSDom)
to prevent compile error for code that references global `fetch` which
doesn't exist in NodeJs. This is fine for compilation errors, but for
tests that actually need to fetch data, `fetch-mock` should be used.
== emailSubscriptionForm.spec ==
emailSubscriptionForm.styl uses a `~`
prefix which is a webpack-only convention that jest / ts-jest doesn't
support. For this particular case using classnames wasn't necessary, so
I removed the import and changed the selector to select the only button
that appears in EmailSubscriptionForm
== lingegraph.spec ==
Linegraph depends on µPlot which depends on both window.matchMedia and
Canvas apis in the browser. Added a jest mock for matchMedia and added a
dependency (jest-canvas-mock) to mock Canvas. Added an import to
linegraph.spec for matchMedia, but jest-canvas-mock suggests adding the
initialization to `setupFile` in the jest config so I added it there.
== locations.spec ==
After a couple of attempts at mocking document to resolve a TypeError
when importing d3, it turned out the issue was that Babel was running d3
in "strict mode" causing `this` to be undefined rather than equivelant
to global window. So the error was coming from an undefined `this` in
`this.document` rather than document not being defined.
To resolve this I added `sourceType` to the Babel config with a value of
"unambiguous", which will run any file containing `import` or `export`
in strict mode and any file without in non-strict mode. This resolved
the error.
In the process I consolidated the environmental setup into
src/setupTests.js removing the need for src/enzyme.setup.js and
src/testUtils/matchMedia.mock.ts. I also dropped
src/test-utils/document.mock.ts as everything we need can be
provided by JSDom. I also dropped src/enzymeInit.ts as it is now
duplicated by setupTests.js
== app.spec.ts ==
app.spec.ts loads jobDetails.tsx through the import dependency chain.
jobDetails.tsx had imported summaryCard/styles.styl with an alias import
path ("src/views/shared/components/summaryCard/styles.styl") rather than
a relative import path (like all other `.styl` imports). This alias path
was being picked up by the the `compilerOptions.paths` portion of the
`moduleNameMapper` option in the Jest config. Since these paths are
passed through the `transform` section of the config there was no
matching pattern and Jest was failing to process this `.styl` file as
JavaScript. To resolve this I changed the path to a relative one.
While messing with the `moduleNameMapper` section, I realized we were
loading a long list of image files with `identity-obj-proxy`, which is
used to mock CSS modules. I added a file.mock.js file and pointed all
image and font files to it.
Since the `<App>` component is being mounted outside of a browser
`Element.scrollTo` seems to not be implemented on the <div> in
`layout/index.tsx` so I added a guard around it.
Also mounting App seemed to throw `Invalid hook call` errors for every
React hook being used. I added an entry into `moduleNameMapper` to
ensure that only one instance of React is being loaded and that seemed
to clear them up.
After changing `after()` to `afterAll()`, the tests passed.
Unfortunately now there are around 16 errors about "an interaction not
being wrapped in act() inside a test". Despite wrapping several things
in act(), I could not get rid of these errors.
== api.spec.ts ==
Removed `this.timeout` calls, as this refers to a Mocha test level
timeout (and Mocha doesn't exist in Jest).
Release note: None
Having `enzyme` as the `testEnvironment` was causing the error, ``` Test environment found at [path to jest-enzyme-enzyme] does not export a "getVmContext" method, which is mandatory from Jest 27. This method is a replacement for "runScript". ``` This seems to be coming from runScript.js[1] Changing the testEnvironment to `jsdom` removes all the errors. What I don't understand is why this setting works in Cluster-ui or has worked intermittently during this Jest migration. [1]: https://github.com/facebook/jest/blob/main/packages/ jest-runner/src/runTest.ts#L173 Release note: None
`enzymeInit` is now part of `setupTests.js` in DB Console. Release note: None
Stub some components to avoid making network requests during initial renders. Release note: None
This commit is the initial run of `npx jest-codemods` to replace Chai assertions in `*.spec.tsx?` files. Release note: None
Some of the tests weren't refactored or were refactored incorrectly by jest-codemods. These few tests were refactored by hand in this change. Release note: None
When I ran jest-codemods before I only included `*.spec.ts` files. This change includes `*.spec.tsx` files with a single hand correction. Release note: None
Besides using assertions from `chai`, there is an additional assertion dependency (`assert`) used in some of the tests. This change removes the use of this assertion library and refactors the tests using `expect`. Release note: None
Changing occurances of `.toEqual` on a primitive value to `.toBe` as well as changing occurances of `.not.toBeDefined` to `.toBeUndefined`. I also spotted some lint errors, so I ran `npm run lint:fix` to clean up some linting errors Release note: None
Release note: None
This test probably snuck back in on a rebase, but was deleted earlier based on changes to TimeSeries Release note: None
|
bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a draft pull request branched off of #82161 refactoring
assertsyntax (usingchaiandassert) with Jest'sexpectsyntax.Since is based on top of the Jest PR, this PR will remain in draft until this larger is merged and rebased