tests: add type checking to cli/tests #6874

connorjclark · 2018-12-27T23:59:51Z

This just adds type checking for lighthouse/cli/tests. I attempted to also do lighthouse-core/test, but 1) it's significantly more work, so better to be done in smaller pieces later and 2) simply removing that folder from the tsconfig.json ignore list caused an OOM error :)

connorjclark · 2018-12-28T00:00:46Z

lighthouse-cli/test/smokehouse/run-smoke.js

@@ -200,6 +198,7 @@ async function cli() {
  if (failingTests.length && (process.env.RETRY_SMOKES || process.env.CI)) {
    console.log('Retrying failed tests...');
    for (const failedResult of failingTests) {
+      /** @type {number} */


This was weird. Without the explicit type annotation, this error is emitted:

'resultIndex' implicitly has type 'any' because it does not have a type annotation and is referenced directly or indirectly in its own initializer.

that is weird... @brendankenny? :)

Some more clues for @brendankenny. I think it's likely a TypeScript bug.

Error is also avoided with this explicit cast:

/** @type {{ id: string; error?: Error | undefined; }[]} */ const smokeResults = [];

the troublesome line is this index setter:

smokeResults[resultIndex] = (await runSmokehouse([smokeDefn]))[0];

Without that line, there's no error. When I hover over smokeResults in const resultIndex = smokeResults.indexOf(failedResult);, the type is correct, but hover over it in smokeResults[resultIndex] = (await runSmokehouse([smokeDefn]))[0]; and it is any[].

The type inferencing is all out of whack here. smokeResults is inferred to be

{ id: string; error?: Error | undefined; }[]

but only after the for..of..runSmokehouse loop. resultIndex is correctly given number, but the TSC gets confused when attempting to use that derived type from the original inferred variable on that same variable whose type was inferred (that makes sense. yeah? yea).

Minimal example:

yes, this is a typescript limitation. smokeResults is being inferred as an evolving array type, adding element types as the type checker reads our code.

arr[n] = someValue is one of the evolving initializer patterns that adds typeof someValue to the array type if n is numeric. If the type checker is being semi-smart the circularity is based on the fact that when checking that resultIndex is numeric, it finds it's from Array<T>.indexOf, where the array type T is (in part) based on the assignment below to smokeResults[resultIndex]. A special case would probably need to be added to note that some array methods always return a number, regardless of the type of the array.

If the type checker is being less smart, it's probably just running into a control flow limitation, where the indexOf query and the assignment are preventing reduction of the inferred type across the loop with the assignment above.

Also, good minimal example, you should file it (though it may be another variant of microsoft/TypeScript#19955).

Not sure if it just wasn't the point of what was being demonstrated, but blah.ts in that screenshot has the same issue if you turn on noImplicitAny (playground link (have to turn on the flag in options))

oh yeah, end of all this is to work around it, the index needs an explicit type (as you've done here), the array needs an explicit type, or don't save an intermediate variable so there's no need for a circular definition (smokeResults[smokeResults.indexOf(failedResult)] = (await runSmokehouse([smokeDefn]))[0];). What you have is the shortest of the three, so seems good to me :)

Great, I've filed an issue: microsoft/TypeScript#29290

patrickhulce

thanks for takin' this up! :)

patrickhulce · 2018-12-28T16:26:15Z

lighthouse-cli/test/smokehouse/smokehouse.js

@@ -224,28 +241,30 @@ function collateResults(actual, expected) {
  });

  return {
-    finalUrl: {


what does this move do? :)

nothing, just alphabetizing stuff

patrickhulce · 2018-12-28T16:28:34Z

lighthouse-cli/test/global-mocha-hooks.js

@@ -1,3 +1,4 @@
+// @ts-nocheck


I don't think we've used this and lighthouse-core/test/global-mocha-hooks for long time.

Neat, deleted.

Neat, deleted

not pushed yet?

oops, pushed.

patrickhulce · 2018-12-28T16:29:11Z

lighthouse-cli/test/cli/run-test.js

      const results = JSON.parse(fs.readFileSync(filename, 'utf-8'));
      assert.equal(results.audits.viewport.rawValue, false);

      // passed results match saved results
      assert.strictEqual(results.fetchTime, lhr.fetchTime);
-      assert.strictEqual(results.url, lhr.url);


😆 nice find!

patrickhulce · 2018-12-28T16:30:57Z

lighthouse-cli/test/smokehouse/run-smoke.js

@@ -200,6 +198,7 @@ async function cli() {
  if (failingTests.length && (process.env.RETRY_SMOKES || process.env.CI)) {
    console.log('Retrying failed tests...');
    for (const failedResult of failingTests) {
+      /** @type {number} */


that is weird... @brendankenny? :)

patrickhulce · 2018-12-28T16:35:17Z

lighthouse-cli/test/smokehouse/smokehouse.js

@@ -316,12 +336,14 @@ const cli = yargs
    'expectations-path': 'The path to the expected audit results file',
    'debug': 'Save the artifacts along with the output',
  })
-  .require('config-path')
-  .require('expectations-path')
+  .require('config-path', true)


are our typedefs for yargs off or was this really required before and we were missing it?

yeah they are off. we use a DefinitelyTyped which doesn't define a few things. This require is apparently a deprecated API, perhaps that is why.

This require is apparently a deprecated API, perhaps that is why.

yeah, we use an ancient yargs@3.32.0 (though it might not have been valid back then, either :)

patrickhulce · 2018-12-28T16:36:22Z

lighthouse-cli/test/cli/index-test.js

@@ -3,6 +3,8 @@
 * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
 */
+// eslint-disable-next-line spaced-comment
+/// <reference types="jest" />


Interesting, I've never had to do this in my other projects with jest, but maybe that's because of some other explicit includes in our tsconfig.json?

If I change the file to .ts the explicit type include is not necessary.

maybe that's because of some other explicit includes in our tsconfig.json

it's also due to the kind of dumb way I set up typeRoots a long time ago. Basically just need to delete the typeRoots entry and change "./types/*.d.ts" to "./types/**/*.d.ts" in the tsconfig and it'll be picked up automatically.

The triple slash feels kind of old fashioned, maybe we should just make the tsconfig update instead?

Awesome, that worked.

exterkamp

I'm not the typing expert but it LGTM, love the cleanup ✨ (is there not a broom/mop emoji on gh? I'm sad about this.)

exterkamp · 2019-01-03T23:11:24Z

lighthouse-cli/test/smokehouse/run-smoke.js

  }
+  process.stdout.write(result.stdout);
+  process.stderr.write(result.stderr);


sometimes you just gotta go to big G unfortch 🧹

i'm gonna need some help here Patrick =P

lol sorry I was replying to

is there not a broom/mop emoji on gh? I'm sad about this.

big G being Google :) i.e. I got the broom emoji by googling "broom emoji" and copy/pasting

oooooh ok. I just see a square :)

brendankenny

nice!

Love this change to the unit tests, too. It would be lovely to get them all type checked as well (though obviously balancing mocking with refactoring/searching will be difficult in core/)

brendankenny · 2019-01-03T23:29:20Z

lighthouse-cli/test/cli/index-test.js

@@ -3,6 +3,8 @@
 * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
 */
+// eslint-disable-next-line spaced-comment
+/// <reference types="jest" />


maybe that's because of some other explicit includes in our tsconfig.json

it's also due to the kind of dumb way I set up typeRoots a long time ago. Basically just need to delete the typeRoots entry and change "./types/*.d.ts" to "./types/**/*.d.ts" in the tsconfig and it'll be picked up automatically.

The triple slash feels kind of old fashioned, maybe we should just make the tsconfig update instead?

brendankenny · 2019-01-03T23:37:31Z

lighthouse-cli/test/cli/run-test.js

@@ -57,6 +64,7 @@ describe('flag coercing', () => {

 describe('saveResults', () => {
  it('will quit early if we\'re in gather mode', async () => {
+    // @ts-ignore - testing a thin part of the interface


can we switch to casting undefined to LH.RunnerResult (or casting {} if it needs to be shorter) so future refactors of run or saveResults will still flag this test as needing to be updated?

Argument of type '{}' is not assignable to parameter of type 'RunnerResult'. Type '{}' is missing the following properties from type 'RunnerResult': lhr, report, artifacts Argument of type '{ gatherMode: true; }' is not assignable to parameter of type 'CliFlags'. Type '{ gatherMode: true; }' is missing the following properties from type 'CliFlags': _, chromeFlags, outputPath, saveAssets, and 9 more.

Because of these errors, closest I could get is

const result = await run.saveResults( /** @type {any} */ ({}), /** @type {any} */ ({gatherMode: true}) );

Which isn't much better than the current code. wdyt?

Which isn't much better than the current code. wdyt?

Mocking types is going to come up as an issue like this a whole lot when/if we convert core/test/ over to type checking. I think we do want to default to as little // @ts-ignore as possible because the compiler ends up blind there. Casting/assertions have the same problem, of course, but at least it's limited to the single variable, and tsc will catch the small case of if the types become mutually incompatible.

The ideal solution is the mocks become good enough to properly match the types, but failing that for this case, we can also do:

const result = await run.saveResults( /** @type {LH.RunnerResult} */ ({}), /** @type {LH.CliFlags} */ ({gatherMode: true}));

(I think the errors you saw were if you don't include the @type?)

It looks a little sloppy, but it's also an expression of how narrowly the test is targeting the internals of the current saveResults implementation, so it seems somewhat appropriate :) WDYT?

Oh yes, that code works. I must have mistyped the jsdoc comment.

brendankenny · 2019-01-03T23:41:14Z

lighthouse-cli/test/fixtures/byte-efficiency/script.js

@@ -3,6 +3,7 @@
 * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
 */
+// @ts-nocheck


rather than @ts-nochecking each of these, what do you think about just excluding lighthouse-cli/test/fixtures/**/*.js in the tsconfig.json? I don't foresee us ever incrementally adding types to those files in the future as they rely on implicit dependencies between each other (e.g. in html files, dynamically added script nodes, etc), so might as well make the exclusion explicit up top.

brendankenny · 2019-01-03T23:41:28Z

lighthouse-cli/test/global-mocha-hooks.js

@@ -1,3 +1,4 @@
+// @ts-nocheck


Neat, deleted

not pushed yet?

brendankenny · 2019-01-04T00:20:32Z

lighthouse-cli/test/smokehouse/smokehouse.js

@@ -316,12 +336,14 @@ const cli = yargs
    'expectations-path': 'The path to the expected audit results file',
    'debug': 'Save the artifacts along with the output',
  })
-  .require('config-path')
-  .require('expectations-path')
+  .require('config-path', true)


This require is apparently a deprecated API, perhaps that is why.

yeah, we use an ancient yargs@3.32.0 (though it might not have been valid back then, either :)

brendankenny · 2019-01-04T00:29:25Z

lighthouse-cli/test/smokehouse/run-smoke.js

@@ -12,7 +12,8 @@ const execAsync = promisify(require('child_process').exec);
 const {server, serverForOffline} = require('../fixtures/static-server');
 const log = require('lighthouse-logger');

-const purpleify = str => `${log.purple}${str}${log.reset}`;
+// TODO(hoten): add to logger. but wait until it's not a separate package.
+const purpleify = ( /** @type {string} */ str) => `${log.purple}${str}${log.reset}`;


we don't have an ironclad rule for this, but it's nice to have params called out as params and a single line /** @param {string} str */ above this seems more readable than the inline version.

brendankenny · 2019-01-04T00:30:00Z

lighthouse-cli/test/smokehouse/run-smoke.js

@@ -88,18 +89,15 @@ const SMOKETESTS = [{

 /**
 * Display smokehouse output from child process
- * @param {{id: string, process: NodeJS.Process} || {id: string, error: Error & {stdout : NodeJS.WriteStream, stderr: NodeJS.WriteStream}}} result
+ * @param {{id: string, stdout : string, stderr: string, error?: Error}} result


extra space after stdout

brendankenny · 2019-01-04T00:32:48Z

lighthouse-cli/test/smokehouse/run-smoke.js

-      .then(cp => ({id: id, process: cp}))
-      .catch(err => ({id: id, error: err}))
+      .then(cp => ({id, ...cp}))
+      .catch(err => ({id, stdout: err.stdout, stderr: err.stderr, error: err}))


brendankenny · 2019-01-04T02:07:04Z

lighthouse-cli/test/smokehouse/run-smoke.js

@@ -200,6 +198,7 @@ async function cli() {
  if (failingTests.length && (process.env.RETRY_SMOKES || process.env.CI)) {
    console.log('Retrying failed tests...');
    for (const failedResult of failingTests) {
+      /** @type {number} */


yes, this is a typescript limitation. smokeResults is being inferred as an evolving array type, adding element types as the type checker reads our code.

arr[n] = someValue is one of the evolving initializer patterns that adds typeof someValue to the array type if n is numeric. If the type checker is being semi-smart the circularity is based on the fact that when checking that resultIndex is numeric, it finds it's from Array<T>.indexOf, where the array type T is (in part) based on the assignment below to smokeResults[resultIndex]. A special case would probably need to be added to note that some array methods always return a number, regardless of the type of the array.

If the type checker is being less smart, it's probably just running into a control flow limitation, where the indexOf query and the assignment are preventing reduction of the inferred type across the loop with the assignment above.

brendankenny · 2019-01-04T02:09:54Z

lighthouse-cli/test/smokehouse/run-smoke.js

@@ -200,6 +198,7 @@ async function cli() {
  if (failingTests.length && (process.env.RETRY_SMOKES || process.env.CI)) {
    console.log('Retrying failed tests...');
    for (const failedResult of failingTests) {
+      /** @type {number} */


Also, good minimal example, you should file it (though it may be another variant of microsoft/TypeScript#19955).

Not sure if it just wasn't the point of what was being demonstrated, but blah.ts in that screenshot has the same issue if you turn on noImplicitAny (playground link (have to turn on the flag in options))

googlebot · 2019-01-07T20:10:52Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

patrickhulce

LGTM!

brendankenny

LGTM2!

brendankenny · 2019-01-07T20:36:47Z

lighthouse-cli/test/cli/index-test.js

@@ -3,6 +3,7 @@
 * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
 */
+// eslint-disable-next-line spaced-comment


don't need this anymore

cla bot is already broken from earlier merge anyways :)

tests: add type checking (#6852)

306c541

connorjclark requested review from brendankenny, patrickhulce and paulirish as code owners December 27, 2018 23:59

connorjclark commented Dec 28, 2018

View reviewed changes

more types

71e6d19

connorjclark changed the title ~~tests: add type checking (#6852)~~ tests: add type checking Dec 28, 2018

change redundant name

e84c5a6

patrickhulce reviewed Dec 28, 2018

View reviewed changes

delete unused mocha hook files

f12c9e9

exterkamp reviewed Jan 3, 2019

View reviewed changes

brendankenny changed the title ~~tests: add type checking~~ tests: add type checking to cli/tests Jan 3, 2019

brendankenny reviewed Jan 4, 2019

View reviewed changes

brendankenny mentioned this pull request Jan 4, 2019

tests(smokehouse): use native node URLSearchParams #6929

Merged

connorjclark mentioned this pull request Jan 7, 2019

Unexpected implicit any for return value of Array<T>.indexOf microsoft/TypeScript#29290

Open

connorjclark and others added 7 commits January 7, 2019 11:07

remove triple slash type include for jest. update tsconfig types

77649f9

cr changes

9243ff7

ts exclude fixtures

4380698

deps(snyk): update snyk snapshot (#6888)

79b8b28

docs: Update reference of tti (#6865)

9a153ec

Merge branch 'master' into issue-6852-type-smokehouse

859a00c

Merge branch 'master' into issue-6852-type-smokehouse

0ebfccd

better types in smoke run test

e8c4a31

patrickhulce approved these changes Jan 8, 2019

View reviewed changes

brendankenny approved these changes Jan 8, 2019

View reviewed changes

remove triple slash comment

db39a91

cla bot is already broken from earlier merge anyways :)

brendankenny merged commit bb3fcdd into master Jan 8, 2019

brendankenny deleted the issue-6852-type-smokehouse branch January 8, 2019 20:18

connorjclark mentioned this pull request Dec 17, 2019

tests(driver): type check #10123

Merged

tests: add type checking to cli/tests #6874

tests: add type checking to cli/tests #6874

Conversation

connorjclark commented Dec 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Dec 28, 2018 • 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

patrickhulce 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

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

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

exterkamp 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jan 7, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Dec 28, 2018 •

edited

Loading