[RW-5959][risk=no] Upgrade jest to latest version #4733

aweng98 · 2021-03-23T22:17:38Z

Retry reverted PR commit 4287.

aweng98 · 2021-03-24T00:32:10Z

ui/src/app/utils/leo-runtime-initializer.spec.tsx

@@ -7,7 +7,7 @@ import {Runtime} from 'generated/fetch';
 import {RuntimeStatus} from 'generated/fetch';
 import {RuntimeApi} from 'generated/fetch/api';
 import SpyInstance = jest.SpyInstance;
-import expect = jest.Expect;
+import { expect } from '@jest/globals';


Fix ts error from jest.assertions usage in this file.

aweng98 · 2021-03-24T00:53:01Z

ui/package.json

@@ -25,7 +25,7 @@
      "./test-shim.js",
      "./test-setup.js"
    ],
-    "setupTestFrameworkScriptFile": "./test-setup-script.js",
+    "setupFilesAfterEnv": ["./test-setup-script.js"],


Jest deprecated setupTestFrameworkScriptFile in favor of new setupFilesAfterEnv (#7119)

aweng98 · 2021-03-25T20:51:44Z

ui/src/app/cohort-search/cohort-search/cohort-search.component.tsx

@@ -248,7 +248,7 @@ export const CohortSearch = withCurrentCohortSearchContext()(class extends React
      clearTimeout(this.growlTimer);
    }
    // This is to set style display: 'none' on the growl so it doesn't block the nav icons in the sidebar
-    this.growlTimer = setTimeout(() => this.setState({growlVisible: false}), 2500);
+    this.growlTimer = global.setTimeout(() => this.setState({growlVisible: false}), 2500);


Resolving typescript issue

aweng98 · 2021-03-26T15:34:46Z

ui/src/tsconfig.jest.json

@@ -7,12 +7,10 @@
    // No outDir needs to be specified here, as test runs will not produce output files anyway.
    "baseUrl": "./",
    "module": "commonjs",
-    "target": "es5",


target: "es6" inherited from ../tsconfig.json.

aweng98 · 2021-03-26T15:35:10Z

ui/src/tsconfig.jest.json

    "types": [
      "jest",
      "node"
-    ],
-    "jsx": "react"


inherited from ../tsconfig.json

aweng98 · 2021-03-26T15:36:15Z

ui/tsconfig.json

-    "typeRoots": [
-      "node_modules/@types"
-    ],


defining typeRoots is unnecessary. It is part of the default value.
https://www.typescriptlang.org/tsconfig#typeRoots

aweng98 · 2021-03-26T16:59:56Z

ui/src/app/pages/profile/profile-page.tsx

+        if (valueKey && !Array.isArray(valueKey)) {
+          valueKey = [valueKey];
+        }
+        if (valueKey && valueKey.length > 1) {


The profile-page.spec.tsx was failing consistently after versions upgrade. @ch helped me and figured out it was caused by a bug here.

calbach

Looks good - but would like @petesantos' review on the line I mentioned before merging in case I'm missing something.

calbach · 2021-03-26T17:07:21Z

ui/src/app/pages/profile/profile-page.tsx

@@ -433,7 +433,10 @@ export const ProfilePage = fp.flow(

      const makeProfileInput = ({title, valueKey, isLong = false, ...props}) => {
        let errorText = profile && errors && errors[valueKey];
-        if (valueKey && Array.isArray(valueKey) && valueKey.length > 1) {
+        if (valueKey && !Array.isArray(valueKey)) {
+          valueKey = [valueKey];


@petesantos can you review this specific change? Based on my reading of the previous code, L444 when the valueKey is 'givenName', the array expansion should equate to:

this.setState(fp.set(['currentProfile', 'g', 'i', 'v', 'e', 'n', 'N', 'a', 'm', 'e'], v))

With Alex's change, the test started failing from this somehow. Do you have ideas on what's going on here? I'm still puzzled as to how the current code is functional.

This is interesting, it looks like JS and TS handle spread differently. When the TS compiles down to javascript it must do something to prevent strings from getting split apart on a spread. The new code looks more correct and in-line with what I would expect. I will put together a code example demonstrating this issue. What was the specific error?

UI unit profile-page test failure:

FAIL src/app/pages/profile/profile-page.spec.tsx ● ProfilePageComponent › should invalidate inputs correctly expect(received).toBeTruthy() Received: false 88 | const wrapper = component(); 89 | wrapper.find(TextInput).first().simulate('change', {target: {value: ''}}); > 90 | expect(wrapper.find(TextInput).first().prop('invalid')).toBeTruthy(); | ^ 91 | }); 92 | 93 | it('should display/update address', async() => { at Object.<anonymous> (src/app/pages/profile/profile-page.spec.tsx:90:61)

Cool, thanks - it should be possible to set up a more isolated reproducer here (the above failure is highly detached from the cause, and required some trial and error to trace down), I can try and help set this up as well if you'd like.

For the purposes of this PR I think it should be fine to merge now. Pete's comment of "The new code looks more correct and in-line with what I would expect" answers what I was looking for, which is to make sure I wasn't missing something obvious and that the change makes sense.

So this is the issue:

This Typescript code (in version 3.2.z)

const checkValueKey = (valueKey) => { console.log(['test', ...valueKey]) } checkValueKey('test')

compiles to this in javascript:

var checkValueKey = function (valueKey) { console.log(['test'].concat(valueKey)); }; checkValueKey('test');

When using concat as a spread operator you will see the behavior we are seeing in the code. The string item in the concat will not be split apart.

However, the same code when compiled in TS 4.2.3 yields:

var __spreadArray = (this && this.__spreadArray) || function (to, from) { for (var i = 0, il = from.length, j = to.length; i < il; i++, j++) to[j] = from[i]; return to; }; var checkValueKey = function (valueKey) { console.log(__spreadArray(['test'], valueKey)); }; checkValueKey('test');

Where the __spreadArray function more closely follows what JS does with the spread operator, splitting apart the string.

Nice find! Thanks. That seems like the likely culprit.

Something that's still not obvious to me though is why the typescript version changed with this PR, that seems like an unexpected side effect. My best guess is that changing ts-jest changed the Typescript version behind the scenes for tests, but if so this interaction is not obvious from yarn.lock or from the ts-jest docs.

It looks like "ts-jest": "^26.5.4" uses TypeScript 4.x. This is probably why this error cropped up in the test.

This is a caveat we should be aware of - the jest tests will be using a different version of typescript from our code.

One more update - I was wondering what our ES target version was, and whether a change to that would prevent substituting the spread operator at all, I don't think there is any browser we are targeting that doesn't support this.
Looking at the tsconfig.json L13 we are changing our ES target to es6 in this change.
Prior to this change, the code in question would compile to this (note the concat) :

return _this.setState(lodash_fp__WEBPACK_IMPORTED_MODULE_1__["set"](['currentProfile'].concat(valueKey), v));

and it now compiles to:

onChange: v => this.setState(lodash_fp__WEBPACK_IMPORTED_MODULE_1__["set"](['currentProfile', ...valueKey], v)),

I am curious as to whether the error popped up as a result of the target change, or the jest upgrade. In other words - is jest using a different typescript version from our build?

Alright... Final update... just for a full understanding of what is happening.
It looks like ts-jest is using our version of TS, the real issue is with the target change in the config and how it effects the resulting Javascript that comes out.

This is what I tested to confirm the behavior of the tests:

ts-jest version | target | result ---------------------------------------------- 25.3.0 | es5 | pass (because it uses concat ) 25.3.0 | es6 | fail (uses spread operator) 26.5.4 | es5 | pass (uses concat) --> If this were using 4.x of TS it would fail by using `__spreadArray` 26.5.4 | es6 | fail (uses spread operator)

I'm not 100% confident but it looks to me that there was one usage of valueKey that this missed:
valueKey: 'verifiedInstitutionalAffiliation.institutionDisplayName' -> #4822

calbach · 2021-03-26T20:46:02Z

ui/src/app/utils/leo-runtime-initializer.spec.tsx

@@ -7,7 +7,7 @@ import {Runtime} from 'generated/fetch';
 import {RuntimeStatus} from 'generated/fetch';
 import {RuntimeApi} from 'generated/fetch/api';
 import SpyInstance = jest.SpyInstance;
-import expect = jest.Expect;
+import { expect } from '@jest/globals';


nit: general convention in the codebase (and this file) is {expect} instead of { expect }

petesantos · 2021-04-01T14:57:18Z

ui/src/app/pages/profile/profile-page.tsx

@@ -433,7 +433,10 @@ export const ProfilePage = fp.flow(

      const makeProfileInput = ({title, valueKey, isLong = false, ...props}) => {
        let errorText = profile && errors && errors[valueKey];
-        if (valueKey && Array.isArray(valueKey) && valueKey.length > 1) {
+        if (valueKey && !Array.isArray(valueKey)) {
+          valueKey = [valueKey];


Alright... Final update... just for a full understanding of what is happening.
It looks like ts-jest is using our version of TS, the real issue is with the target change in the config and how it effects the resulting Javascript that comes out.

This is what I tested to confirm the behavior of the tests:

ts-jest version | target | result ---------------------------------------------- 25.3.0 | es5 | pass (because it uses concat ) 25.3.0 | es6 | fail (uses spread operator) 26.5.4 | es5 | pass (uses concat) --> If this were using 4.x of TS it would fail by using `__spreadArray` 26.5.4 | es6 | fail (uses spread operator)

petesantos · 2021-04-01T14:59:28Z

ui/package.json

@@ -117,7 +121,7 @@
    "serialize-javascript": "^3.1.0",
    "stackdriver-errors-js": "^0.4.0",
    "stacktrace-js": "^2.0.0",
-    "ts-jest": "^25.3.0",
+    "ts-jest": "^26.5.4",


Should this be moved to devDependencies?

Good catch. Done.

aweng98

Additional changes to address PR feedback and tests failures:

Updated notebook download menu functions because Puppeteer notebook-download test. menu clicking is not working 100% of time from PR changes.
Reverted target back to es5.
Edited polyfills.ts to resolve a new error.

aweng98 · 2021-04-01T17:54:21Z

ui/src/polyfills.ts

+ * Resolving ERROR: ReferenceError: global is not defined.
+ *
+ */
+(window as any).global = window;


Puppeteer notebook tests were failing. Failures were caused by ReferenceError: global is not defined.

aweng98 · 2021-04-01T19:07:10Z

I made more changes and fixed broken tests. CircleCI testing passed 2 of 2. Could you do a final review? Just to verify new changes made today are okay.

petesantos

Adding the global variable and changing the code to use the global to accommodate an upgrade in the testing lib "feels" a bit off... But I will accept the tradeoff to keep the lib up to date. Perhaps we can revisit those changes once Angular is removed.

petesantos · 2021-04-01T19:59:29Z

ui/src/app/utils/leo-runtime-initializer.spec.tsx

@@ -7,7 +7,7 @@ import {Runtime} from 'generated/fetch';
 import {RuntimeStatus} from 'generated/fetch';
 import {RuntimeApi} from 'generated/fetch/api';
 import SpyInstance = jest.SpyInstance;


Could this be combined with the import statement below it?

No, I wasn't able to combine two imports.
However, I can do this so that we don't need import SpyInstance = jest.SpyInstance;

let mockGetRuntime: jest.SpyInstance; let mockCreateRuntime: jest.SpyInstance; let mockDeleteRuntime: jest.SpyInstance; let mockStartRuntime: jest.SpyInstance;

Let me know if you like me to make this change.

aweng98

I'm going to merge the PR now. If something is off, please let me know so I will make a follow up PR to address them.

aweng98 · 2021-04-01T22:34:23Z

ui/src/app/utils/leo-runtime-initializer.spec.tsx

@@ -7,7 +7,7 @@ import {Runtime} from 'generated/fetch';
 import {RuntimeStatus} from 'generated/fetch';
 import {RuntimeApi} from 'generated/fetch/api';
 import SpyInstance = jest.SpyInstance;


No, I wasn't able to combine two imports.
However, I can do this so that we don't need import SpyInstance = jest.SpyInstance;

let mockGetRuntime: jest.SpyInstance; let mockCreateRuntime: jest.SpyInstance; let mockDeleteRuntime: jest.SpyInstance; let mockStartRuntime: jest.SpyInstance;

Let me know if you like me to make this change.

yonghaoy · 2021-04-02T12:55:04Z

Hello, yesterday's nightly test fails with error:

    console.error
      Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
          in DemographicSurveyComponent (created by ProfileErrorWrapper)
          in ProfileErrorWrapper (created by ErrorModalWrapper)
          in ErrorModalWrapper (created by AccountCreationSurvey)
          in AccountCreationSurvey (created by SignInImpl)

      206 |       const msgType = message.type() === 'warning' ? 'warn' : message.type();
      207 |       console[msgType](`Page console: "${title}"`);
    > 208 |       console[msgType](...args);
          |                       ^
      209 |       console.log('');
      210 |     } catch (err) {
      211 |       console.error(`❗ "${title}"\nException occurred when getting page console message.\n${err}\n${message.text()}`);

      at jest.test-setup.ts:208:23
          at runMicrotasks (<anonymous>)

    console.log

Is that reverent to this PR?

aweng98 · 2021-04-02T14:58:53Z

No. It failed here. the runtime status spinner were not excluded in waitWhileLoading function but it should have. I am making a PR to address this.

aweng98 force-pushed the aw/RW-5959 branch from ab5a95e to aff44f8 Compare March 23, 2021 22:22

aweng98 commented Mar 24, 2021

View reviewed changes

aweng98 changed the title ~~[RW-5959][risk=no] Upgrade jest and related packages to latest versions~~ [RW-5959][risk=no] Upgrade jest to latest version Mar 24, 2021

aweng98 commented Mar 25, 2021

View reviewed changes

aweng98 commented Mar 26, 2021

View reviewed changes

aweng98 requested review from yonghaoy and calbach March 26, 2021 16:57

aweng98 commented Mar 26, 2021

View reviewed changes

yonghaoy approved these changes Mar 26, 2021

View reviewed changes

calbach approved these changes Mar 26, 2021

View reviewed changes

aweng98 force-pushed the aw/RW-5959 branch from 89c0e9f to 0f75e1e Compare March 31, 2021 22:51

aweng98 added 15 commits March 31, 2021 20:27

upgrade jest to latest version

bad4be0

remove node from types in tsconfig.jest.json

acdcddf

fix setupFilesAfterEnv

3d932db

update more

b2f4287

fix error TS2304: Cannot find name 'setImmediate'

602f1a0

fix test error

253ac18

wip

2277643

revert profile-page.spec

089ac6c

update package.json

89d2471

remove .spec.tsx in package.json

6572f1f

fix profile-page bug

ad69520

remove whitespace in import

44c1cd5

lint error

014cab7

rebase

60a76af

fix test script in package.json

487c9a3

aweng98 force-pushed the aw/RW-5959 branch from 899e365 to 487c9a3 Compare April 1, 2021 00:28

petesantos reviewed Apr 1, 2021

View reviewed changes

aweng98 added 2 commits April 1, 2021 13:47

fix tests

2f142af

revert lib es2016

234a000

aweng98 commented Apr 1, 2021

View reviewed changes

move ts-jest to devDependencies

7df5586

petesantos approved these changes Apr 1, 2021

View reviewed changes

aweng98 commented Apr 1, 2021

View reviewed changes

aweng98 merged commit e605ad1 into master Apr 1, 2021

aweng98 deleted the aw/RW-5959 branch April 1, 2021 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RW-5959][risk=no] Upgrade jest to latest version #4733

[RW-5959][risk=no] Upgrade jest to latest version #4733

aweng98 commented Mar 23, 2021 •

edited

Loading

aweng98 Mar 24, 2021

aweng98 Mar 24, 2021

aweng98 Mar 25, 2021 •

edited

Loading

aweng98 Mar 26, 2021

aweng98 Mar 26, 2021

aweng98 Mar 26, 2021

aweng98 Mar 26, 2021

calbach left a comment

calbach Mar 26, 2021

petesantos Mar 31, 2021

aweng98 Mar 31, 2021

calbach Mar 31, 2021 •

edited

Loading

petesantos Apr 1, 2021 •

edited

Loading

calbach Apr 1, 2021

petesantos Apr 1, 2021

petesantos Apr 1, 2021 •

edited

Loading

petesantos Apr 1, 2021

jmthibault79 Apr 13, 2021

calbach Mar 26, 2021

aweng98 Mar 26, 2021

petesantos Apr 1, 2021

petesantos Apr 1, 2021

aweng98 Apr 1, 2021

aweng98 left a comment •

edited

Loading

aweng98 Apr 1, 2021

aweng98 commented Apr 1, 2021

petesantos left a comment

petesantos Apr 1, 2021

aweng98 Apr 1, 2021

aweng98 left a comment

aweng98 Apr 1, 2021

yonghaoy commented Apr 2, 2021

aweng98 commented Apr 2, 2021

[RW-5959][risk=no] Upgrade jest to latest version #4733

[RW-5959][risk=no] Upgrade jest to latest version #4733

Conversation

aweng98 commented Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweng98 Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calbach Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

petesantos Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petesantos Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweng98 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweng98 commented Apr 1, 2021

petesantos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweng98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonghaoy commented Apr 2, 2021

aweng98 commented Apr 2, 2021

aweng98 commented Mar 23, 2021 •

edited

Loading

aweng98 Mar 25, 2021 •

edited

Loading

calbach Mar 31, 2021 •

edited

Loading

petesantos Apr 1, 2021 •

edited

Loading

petesantos Apr 1, 2021 •

edited

Loading

aweng98 left a comment •

edited

Loading