ui: Add Jest as test runner to DB Console #82161
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 |
This was referenced Jun 13, 2022
nathanstilwell
changed the title
maryliag
reviewed
Jun 29, 2022
There was a problem hiding this comment.
Great work, a lot of changes! :D
I added just a few comments
Reviewed 2 of 7 files at r2, 12 of 20 files at r3, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 4 files at r10, 1 of 1 files at r11, 1 of 1 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, 6 of 10 files at r15, 5 of 9 files at r17, 3 of 3 files at r18, 1 of 1 files at r19, 5 of 5 files at r20, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @nathanstilwell)
pkg/ui/workspaces/db-console/src/setupTests.js line 17 at r20 (raw file):
/** * Various thing in DB Console use global fetch. `fetch` is generally
nit: things
pkg/ui/workspaces/db-console/src/redux/analytics.ts line 19 at r20 (raw file):
import { versionsSelector } from "src/redux/nodes"; import { AdminUIState } from "src/redux/state"; import { history } from "src/redux/history";
just curious, what's the diff between the history from /history vs /state?
pkg/ui/workspaces/db-console/src/app.spec.tsx line 398 at r20 (raw file):
it("routes to <ActiveStatementsView> component with view=active", () => { navigateToPath("/sql-activity?tab=Statements&view=active"); assert.lengthOf(appWrapper.find(StatementsPage), 1);
we have a new view inside the Statements Page specific for active statements (the default view is the historical), so this test wants to check if that view was indeed selected, so I think it should be the ActiveStatementsView as it was
pkg/ui/workspaces/db-console/src/app.spec.tsx line 418 at r20 (raw file):
it("routes to <ActiveTransactionsView> component with view=active", () => { navigateToPath("/sql-activity?tab=Transactions&view=active"); assert.lengthOf(appWrapper.find(TransactionsPage), 1);
same things as above
|
Previously, maryliag (Marylia Gutierrez) wrote…
For some reason the |
maryliag
reviewed
Jun 29, 2022
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/db-console/src/app.spec.tsx line 398 at r20 (raw file):
Previously, nathanstilwell (Nathan Stilwell) wrote…
For some reason the
ActiveStatementsViewisn't found and so causes this test to fail. Does that ActiveStatementsView depend on a network request to load or some specific data to be present?
cc @xinhaoz
|
Previously, maryliag (Marylia Gutierrez) wrote…
I believe it is the same (in that they are both created from |
matthewtodd
approved these changes
Jul 5, 2022
There was a problem hiding this comment.
🎉 wow. Thanks, @nathanstilwell! Other than the open ActiveStatementsView conversation,
Reviewed 2 of 7 files at r2, 11 of 20 files at r3, 6 of 31 files at r21, 4 of 9 files at r22, 3 of 3 files at r23, 5 of 10 files at r24, 14 of 18 files at r25, 9 of 9 files at r27, all commit messages.
Reviewable status:
|
Previously, maryliag (Marylia Gutierrez) wrote…
The page should load fine if there's no data, were there any other errors encountered? |
Release note: None
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
Changing from ArrayBuffer to Uint8Array due to change from browser global fetch to node-fetch. Release note: None
it was previously missing 🤷 Release note: None
Release note: None
* replace this.timeout(…) with jest.setTimeout(…);
* replace `return api.toArrayBuffer(encodedResponse)` with
`return encodedResponse`
A few tests still have `setTimeout(() => { /* … */ }, 1000);` in them
which significantly slows those cases down, but we can handle that in
later commits.
Release note: None
It was already babel'd once during the cluster-ui build process, and node's typically ahead of browser anyway. There's no need to babel it again. Release note: None
Release note: None
Node has full support for UInt8Array (and even most modern browsers do, though IE never got support for TypedArray.prototype.slice() [1]), so there's no need to convert to the browser-specific ArrayBuffer. [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/ Global_Objects/TypedArray/slice Release note: None
`enzymeInit` is now part of `setupTests.js` in DB Console. Release note: None
Release note: None
When running `src/redux/alerts.spec.ts` in Jest, Sinon and JSDom's sessionStorage don't seem to be working as expected so session information was persisted across tests (causing failures). I added `sessionStorage.clear()` to `afterEach` to prevent this and dropped Sinon. Release note: None
Stub some components to avoid making network requests during initial renders. Release note: None
When debugging the output of large components in React Testing Library, the output can get cut off in the default Jest setting for debug lines. Increasing it to a silly amount to see even the largest of components. Release note: None
The test "renders a message when the table is empty" was failing with an `Objects are not valid as a React child` error. The error was coming from the icons passed as props to `<EmptyTable />` in the `renderEmptyState` function. These icons were being imported from `.svg` files. In the browser application these svgs are converted to base64 strings, but in the Jest environment they are replaces with the contents of `file.mock.js` (as specified in the jest.config.js moduleNameMapper section). `file.mock.js` was exporting an empty object, causing this error. The initial fix was to change the contents of `file.mocks.js` to return an empty string instead of an object. Upon recognizing that these svgs are also a part of @cockroachlabs/icons, I imported the icons from there. In @crl/icons, svg files are converted to React Components using @svgr/cli. I am leaving the change to `file.mock.js` in place to prevent future errors of this type. Removed the `expectPopperTooltipActivated` from "shows next execution time" test. Using React Testing Library it is not necessary to confirm the presence of a DOM element as long as the user interaction and result are expected. I think there was an issue with this assertion inside of a `waitFor`, as it was reaching the max time out without finding the element. This may have to do with `document` being simulated by jsdom. In any case, this was not necessary for the test. Upon rebasing there were also some prettier formatting changes that snuck in here as well (`app.spec.tsx` and `mockComponents.tsx`). I also suppressed some moment warnings by providing a time format to some `moment()` calls in `jobsTable.fixture.ts`. Release note: None
- Adding a copyright header to mockComponent.tsx - Adding a copyright header to src/redux/history.ts Release note: None
Changing from karma to `yarn test` in DB Console Release note: None
Beginning work in moving from Karma to Jest in Bazel build and tools Release note: None
Release note: None
This is a *very* messy but functional snapshot.
- Remove unnecessary `jest-silent-reporter` from dependencies - Increase Javascript heap size for Jest tests (uncomment switch in BUILD.bazel) Release note: None
Without this line in Jest's config, `cluster-ui` will attempt to load the copy of react from *its* `node_modules/` tree, resulting in two copies of `react` and errors telling us that's not a good thing. Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
|
Previously, xinhaoz (Xin Hao Zhang) wrote…
@xinhaoz The test simply fails to find the |
|
bors r+ |
|
Build succeeded: |
craig bot
pushed a commit
that referenced
this pull request
Jul 18, 2022
82860: ui: Refactor Assertions to use Jest Expect r=nathanstilwell a=nathanstilwell This is a draft pull request branched off of #82161 refactoring `assert` syntax (using [`chai`](https://www.npmjs.com/package/chai) and [`assert`](https://www.npmjs.com/package/assert)) with Jest's [`expect` syntax](https://jestjs.io/docs/expect). Since is based on top of [the Jest PR](#82161), this PR will remain in draft until this larger is merged and rebased 83712: cloud: support chaining of assumed roles r=rhu713 a=rhu713 Previously, a user could only assume a role in AWS or GCP directly, via the identity specified by implicit or specified auth. This was inadequate because there is a need to support role assumption through a chain of intermediate delegate roles. To address this, this patch allows the ASSUME_ROLE parameter in AWS and GCP storage and KMS URIs to specify a list of roles with a comma-separated string. The roles in the list can then be chain assumed in order to access the resource specified by the URI. With this patch, if a destination in S3 can only be accessed by RoleB, and the identity corresponding to implicit auth can only assume RoleB through an intermediate role RoleA, then this chain assumption can be specified in the S3 URI: s3://bucket/key?AUTH=implicit&ASSUME_ROLE=RoleA,RoleB In addition, the parameters for auth in AWS URIs via assume role have been changed so that the "assume" auth mode no longer exists, and the ASSUME_ROLE param can be specified for both "specified" and "implicit" auth. Finally, some AWS cloud unit tests, including the assume role tests, have been added to the unit test nightly. Release note (enterprise change): Allow the ASSUME_ROLE parameter in AWS and GCP storage and KMS URIs to specify a list of roles with a comma-separated string. The roles in the list can then be chain assumed in order to access the resource specified by the URI. For example, if a destination in S3 can only be accessed by RoleB, and the identity corresponding to implicit auth can only assume RoleB through an intermediate role RoleA, then this chain assumption can be specified in the S3 URI: s3://bucket/key?AUTH=implicit&ASSUME_ROLE=RoleA,RoleB In addition, remove the "assume" auth mode from AWS URIs, and instead use the ASSUME_ROLE parameter to indicate that a role should be assumed for authentication. Below are some examples: S3: s3://<bucket>/<key>?AUTH=specified&ASSUME_ROLE=<role_arn>&AWS_ACCESS_KEY_ID=<access_key>&AWS_SECRET_ACCESS_KEY=<secret_key> AWS KMS: aws:///<key_arn>?AUTH=implicit®ION=<region>&ASSUME_ROLE=<role_arn> 83868: tree: fix vectorized COALESCE and NULLIF type checking with VOID r=msirek a=msirek Fixes #83754 Previously, the COALESCE and NULLIF operators could error out in vectorized execution when comparing a VOID datum with NULL. This occurred due to the unique property of a VOID, that it can't be compared with another VOID using any of the normal operators such as `=`, `<`, `>`..., or even IS [NOT] [DISTINCT FROM], for example: ``` SELECT ''::VOID IS DISTINCT FROM ''::VOID; ERROR: unsupported comparison operator: <void> IS DISTINCT FROM <void> SQLSTATE: 22023 ``` Processing of COALESCE in the columnar execution engine for an expression like `COALESCE(void_col, NULL)` builds an IS DISTINCT FROM operation between the VOID column and NULL here: https://github.com/cockroachdb/cockroach/blob/ea559dfe0ba57259ca71d3c8ca1de6388954ea73/pkg/sql/colexec/colbuilder/execplan.go#L2122-L2129 This comparison is assumed to be OK, but fails with an internal error because comparison with `UnknownType` is only implicitly supported if the type can be compared with itself. To address this, this patch modifies vectorized COALESCE to use the IS NOT NULL operator internally instead of IS DISTINCT FROM whenever the latter is not a valid comparison. A similar problem with NULLIF, which internally uses `=`, is fixed. Type checking of NULLIF is enhanced in the parser so incompatible comparisons can be caught prior to execution time. Release note (bug fix): This patch fixes vectorized evaluation of COALESCE involving expressions of type VOID and enhances type checking of NULLIF expressions with VOID, so incompatible comparisons can be caught during query compilation instead of during query execution. 84159: colexec: fixed nullableArgs projection operators r=wenyihu6 a=wenyihu6 Previously, the `Concat` projection operators on arrays directly copy nulls into the output without performing the actual projection whenever either argument was null. This is incorrect because `Concat` operator can actually handle null arguments, and we want the actual projection result instead. Since currently the only projection operator that needs `nullableArgs == true` behaviour is `ConcatDatumDatum`, this PR fixed this by adding a boolean field `nullableArgs` to the logic for operators with `Datum`, `Datum`. If we later introduce another projection operation that has nullableArgs == true, we should update this code accordingly. Fixes: #83091 Release note (bug fix): Fixed a bug in `Concat` projection operators on arrays that gave output of nulls when the projection operator can actually handle null arguments and may result in a non-null output. Co-authored-by: Nathan Stilwell <[email protected]> Co-authored-by: Sean Barag <[email protected]> Co-authored-by: Rui Hu <[email protected]> Co-authored-by: Mark Sirek <[email protected]> Co-authored-by: wenyihu3 <[email protected]>
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.
DB Console is the last place Cockroach Labs is using a test runner other than Jest. This PR adds Jest as the test runner intended to replace Mocha. Mocha runs in a headless browser via Karma whereas Jest will run tests in NodeJS and simulate a browser environment using jsdom. Due to this change in environment, you will see not only files to set up the Jest test runner, but changes to some tests, some mocking of browser globals that are not included in jsdom by default, and some configuration adjustment to the
tsconfig.json.Since configuration changes are infrequent and are highly contextual, we decided to err on the side of verbose inline documentation in configuration files.
Details about individual changes to configs or tests are documented in commit messages.