From 6daefc68e79a09eb2848dde2b137bc43a1b20157 Mon Sep 17 00:00:00 2001 From: Dmitrii Filippov Date: Tue, 3 Nov 2020 11:05:37 +0100 Subject: [PATCH] Add Typescript test migration instruction to README.md Change-Id: Idd1a0f017ea385fd06f38abb3ecfda161831c385 --- polygerrit-ui/README.md | 246 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 239 insertions(+), 7 deletions(-) diff --git a/polygerrit-ui/README.md b/polygerrit-ui/README.md index 3e95e42ed8..2266ba0142 100644 --- a/polygerrit-ui/README.md +++ b/polygerrit-ui/README.md @@ -1,12 +1,5 @@ # Gerrit Polymer Frontend -**Warning**: DON'T ADD MORE TYPESCRIPT FILES/TYPES. Gerrit Polymer Frontend -contains several typescript files and uses typescript compiler. This is a -preparation for the upcoming migration to typescript and we actively working on -it. We want to avoid massive typescript-related changes until the preparation -work is done. Thanks for your understanding! - - Follow the [setup instructions for Gerrit backend developers](https://gerrit-review.googlesource.com/Documentation/dev-readme.html) where applicable, the most important command is: @@ -279,6 +272,245 @@ or npm run polylint ``` +## Migrating tests to Typescript + +You can use the following steps for migrating tests to Typescript: + +1. Rename the `_test.js` file to `_test.ts` +2. Remove `.js` extensions from all imports: + ``` + // Before: + import ... from 'x/y/z.js` + + // After + import .. from 'x/y/z' + ``` +3. Fix typescript and eslint errors. + +Common errors and fixes are: + +* An object in the test doesn't have all required properties. You can use +existing helpers to create an object with all required properties: +``` +// Before: +sinon.stub(element.$.restAPI, 'getPreferences').returns( + Promise.resolve({default_diff_view: 'UNIFIED'})); + +// After: +Promise.resolve({ + ...createPreferences(), + default_diff_view: DiffViewMode.UNIFIED, +}) +``` + +Some helpers receive parameters: +``` +// Before +element._change = { + change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca', + revisions: { + rev1: {_number: 1, commit: {parents: []}}, + rev2: {_number: 2, commit: {parents: []}}, + }, + current_revision: 'rev1', + status: ChangeStatus.MERGED, + labels: {}, + actions: {}, +}; + +// After +element._change = { + ...createChange(), + // The change_id is set by createChange. + // The exact change_id is not important in the test, so it was removed. + revisions: { + rev1: createRevision(1), // _number is a parameter here + rev2: createRevision(2), // _number is a parameter here + }, + current_revision: 'rev1' as CommitId, + status: ChangeStatus.MERGED, + labels: {}, + actions: {}, +}; +``` +* Typescript reports some weird messages about `window` property - sometimes an +IDE adds wrong import. Just remove it. +``` +// The wrong import added by IDE, must be removed +import window = Mocha.reporters.Base.window; +``` + +* `TS2531: Object is possibly 'null'`. To fix use either non-null assertion +operator `!` or nullish coalescing operator `?.`: +``` +// Before: +const rows = element + .shadowRoot.querySelector('table') + .querySelectorAll('tbody tr'); +... +// The _robotCommentThreads declared as _robotCommentThreads?: CommentThread +assert.equal(element._robotCommentThreads.length, 2); + +// Fix with non-null assertion operator: +const rows = element + .shadowRoot!.querySelector('table')! // '!' after shadowRoot and querySelector + .querySelectorAll('tbody tr'); + +assert.equal(element._robotCommentThreads!.length, 2); + +// Fix with nullish coalescing operator: + assert.equal(element._robotCommentThreads?.length, 2); +``` +Usually the fix with `!` is preferable, because it gives more clear error +when an intermediate property is `null/undefined`. If the _robotComments is +`undefined` in the example above, the `element._robotCommentThreads!.length` +crashes with the error `Cannot read property 'length' of undefined`. At the +same time the fix with +`?.` doesn't distinct between 2 cases: _robotCommentThreads is `undefined` +and `length` is `undefined`. + +* `TS2339: Property '...' does not exist on type 'Element'.` for elements +returned by `querySelector/querySelectorAll`. To fix it, use generic versions +of those methods: +``` +// Before: +const radios = parentTable + .querySelectorAll('input[type=radio]'); +const radio = parentRow + .querySelector('input[type=radio]'); + +// After: +const radios = parentTable + .querySelectorAll('input[type=radio]'); +const radio = parentRow + .querySelector('input[type=radio]'); +``` + +* Sinon: `TS2339: Property 'lastCall' does not exist on type '...` (the same +for other sinon properties). Store stub/spy in a variable and then use the +variable: +``` +// Before: +sinon.stub(GerritNav, 'getUrlForChange') +... +assert.equal(GerritNav.getUrlForChange.lastCall.args[4], '#message-a12345'); + +// After: +const getUrlStub = sinon.stub(GerritNav, 'getUrlForChange'); +... +assert.equal(getUrlStub.lastCall.args[4], '#message-a12345'); +``` + +If you need to define a type for such variable, you can use one of the following +options: +``` +suite('my suite', () => { + // Non static members, option 1 + let updateHeightSpy: SinonSpyMember; + // Non static members, option 2 + let updateHeightSpy_prototype: SinonSpyMember; + // Static members + let navigateToChangeStub: SinonStubbedMember; + // For interfaces + let getMergeableStub: SinonStubbedMember; +}); +``` + +* Typescript reports errors when stubbing/faking methods: +``` +// The JS code: +const reloadStub = sinon + .stub(element, '_reload') + .callsFake(() => Promise.resolve()); + +stub('gr-rest-api-interface', { + getDiffComments() { return Promise.resolve({}); }, + getDiffRobotComments() { return Promise.resolve({}); }, + getDiffDrafts() { return Promise.resolve({}); }, + _fetchSharedCacheURL() { return Promise.resolve({}); }, +}); +``` + +In such cases, validate the input and output of a stub/fake method. Quite often +tests return null instead of undefined or `[]` instead of `{}`, etc... +Fix types if they are not correct: +``` +const reloadStub = sinon + .stub(element, '_reload') + // GrChangeView._reload method returns an array + .callsFake(() => Promise.resolve([])); // return [] here + +stub('gr-rest-api-interface', { + ... + // Fix return type: + _fetchSharedCacheURL() { return Promise.resolve({} as ParsedJSON); }, +}); +``` + +If a method has multiple overloads, you can use one of 2 options: +``` +// Option 1: less accurate, but shorter: +function getCommentsStub() { + return Promise.resolve({}); +} + +stub('gr-rest-api-interface', { + ... + getDiffComments: (getCommentsStub as unknown) as RestApiService['getDiffComments'], + getDiffRobotComments: (getCommentsStub as unknown) as RestApiService['getDiffRobotComments'], + getDiffDrafts: (getCommentsStub as unknown) as RestApiService['getDiffDrafts'], + ... +}); + +// Option 2: more accurate, but longer. +// Step 1: define the same overloads for stub: +function getDiffCommentsStub( + changeNum: NumericChangeId +): Promise; +function getDiffCommentsStub( + changeNum: NumericChangeId, + basePatchNum: PatchSetNum, + patchNum: PatchSetNum, + path: string +): Promise; + +// Step 2: implement stub method for differnt input +function getDiffCommentsStub( + _: NumericChangeId, + basePatchNum?: PatchSetNum, +): + | Promise + | Promise { + if (basePatchNum) { + return Promise.resolve({ + baseComments: [], + comments: [], + }); + } + return Promise.resolve({}); +} + +// Step 3: use stubbed function: +stub('gr-rest-api-interface', { + ... + getDiffComments: getDiffCommentsStub, + ... +}); +``` + +* If a test requires a `@types/...` library, install the required library +in the `polygerrit_ui/node_modules` and update the `typeRoots` in the +`polygerrit-ui/app/tsconfig_bazel_test.json` file. + +The same update should be done if a test requires a .d.ts file from a library +that already exists in `polygerrit_ui/node_modules`. + +**Note:** Types from a library located in `polygerrit_ui/app/node_modules` are +handle automatically. + +* If a test imports a library from `polygerrit_ui/node_modules` - update +`paths` in `polygerrit-ui/app/tsconfig_bazel_test.json`. + ## Contributing Our users report bugs / feature requests related to the UI through [Monorail Issues - PolyGerrit](https://bugs.chromium.org/p/gerrit/issues/list?q=component%3APolyGerrit).