tests: support artifact assertions in smoke tests by mattzeunert · Pull Request #8044 · GoogleChrome/lighthouse

mattzeunert · 2019-04-07T17:22:48Z

Summary

We'd like to make assertions against the artifacts in the smoke tests.

Wrap existing expectations in lhr top level object
Add support for having artifacts expectations in addition to lhr

This reverts commit 7c1ed2d.

patrickhulce

nice @mattzeunert this was super quick turnaround! :)

patrickhulce · 2019-04-07T21:25:45Z

+  const artifactAssertions = [];
+  if (expected.artifacts) {
+    Object.keys(expected.artifacts).map(artifactName => {
+      const actualResult = /** @type {any} */ (actual.artifacts)[artifactName];


can we type these as partial artifacts? I guess it doesn't help much since we don't type check our assertions :)

Do artifacts have a type other than the union type of all artifact values? Is that what you were thinking of?

Oh I get it now, we can type the artifacts key instead of it being string and then the artifact value will be the union type.

Awesome yeah LGTM :)

patrickhulce · 2019-04-07T21:28:10Z

-      actual: actual.errorCode,
-      expected: expected.errorCode,
-      equal: actual.errorCode === expected.errorCode,
+      actual: actual.lhr.errorCode,


now that we are fixing things, this isn't really in the LHR, so makes sense to move out at top level

patrickhulce · 2019-04-07T21:29:10Z

-    args.push('-GA');
-  }
+  // Save artifacts
+  args.push('-GA');


we should specify a tmp path if we're always doing this otherwise every smoke run will overwrite the users saved artifacts

patrickhulce · 2019-04-08T13:43:50Z

For other reviewers ?w=1 was a lifesaver here, it's really only ~100 or so line change without the whitespace diffs :)

patrickhulce

LGTM

patrickhulce · 2019-04-08T13:45:48Z

    finalUrl: Comparison;
  }

+  export interface Assertion{


nit: space after Assertion

Whoops, realized this is the same as Smokehouse.Comparison.

patrickhulce · 2019-04-08T13:47:49Z


  return {
-    audits: collatedAudits,
+    assertions,


a bit strange that we separate out the others but mix these into assertions IMO, I might prefer a separate artifacts property or just collapsing everything into a comparison array

but not a huge deal

Merged to just return a Comparison array.

mattzeunert · 2019-04-09T16:34:04Z

@brendankenny What do you think about this?

brendankenny · 2019-04-09T16:39:25Z

I only got to take a cursory read yesterday with the 4.3 stuff going on, but the concept and implementation were looking great to me. I'll take a closer look later today.

brendankenny

looking good, this is a definite improvement!

brendankenny · 2019-04-09T19:33:52Z

+ * @param {string} name
+ * @param {any} actualResult
+ * @param {any} expectedResult
+ * @returns {Smokehouse.Comparison}


@return to match the file

brendankenny · 2019-04-09T19:36:06Z

 }

+/**
+ * @param {string} name


can you give a doc string for name, not obvious from outside what it is.

and should it be the last param? Total nit since this is entirely internal, but usually for assertion libraries the string description is the last param, though admittedly this string is slightly different

Also, reading it now, I really regret that we called that property category in the return object :)

usually for assertion libraries the string description is the last param

though findDifference isn't like that and I'm pretty sure I wrote its signature, so what do I know :)

I've changed category to name now. We also use it for difference at TapTargets artifact.length, so it's more of an object name than a custom error message you'd have for an assertion. If we compare the whole runner result we could just use the key path here maybe.

brendankenny · 2019-04-09T19:45:29Z

-    args.push('-GA');
-  }
+  // Save artifacts
+  args.push(`-GA=${artifactsDirectory}`);


since this is always done, can just move these two lines into the args array declaration up above

brendankenny · 2019-04-09T19:51:20Z

-    audits: Comparison[];
-    errorCode: Comparison;
-    finalUrl: Comparison;
+  export type ExpectedRunResult = {


we call basically this type on the LH side RunnerResult, so should this be ExpectedRunnerResult?

brendankenny · 2019-04-09T20:06:40Z

+ * @param {any} expectedResult
+ * @returns {Smokehouse.Comparison}
+ */
+function makeAssertion(name, actualResult, expectedResult) {


maybe call this makeComparison now?

brendankenny · 2019-04-09T20:16:34Z

+}
+
 /**
 * Collate results into comparisons of actual and expected scores on each audit.


should update the "on each audit" part

brendankenny · 2019-04-09T20:17:31Z

-  });
+  /** @type {Smokehouse.Comparison[]} */
+  let auditAssertions = [];
+  if (expected.lhr.audits) {


I think we require audits to be defined, fwiw

brendankenny · 2019-04-09T20:22:00Z

I'd like to go all the way and use the real types for the LH.RunnerResult (using a recursive Partial<> except for the few things which are in the Pick<> type today), which after this PR would mostly just need testing lhr.runtimeError instead of our constructed errorCode (though we could still test that too). We could do that in another PR if others are ok with the idea.

Co-Authored-By: mattzeunert <matt@mostlystatic.com>

patrickhulce · 2019-04-10T15:18:52Z

+        throw new Error(`Config run did not generate artifact ${artifactName}`);
+      }
+
+      const expectedResult = (expected.artifacts || {})[artifactName];


should be able to nuke this || given the if on line 132

Hmm, TS fails without the || {} if the artifacts are accessed in the map callback. In general constraining the type seems to work in nested functions, but not here somehow...

ah right the TS callback problem, my preferred way around this recently has been to assign expected.artifacts to a variable right after we've asserted its definedness, then when you access it in the closure TS already knows it's safe

patrickhulce · 2019-04-10T15:19:55Z

-  [results.finalUrl, results.errorCode, ...results.audits].forEach(auditAssertion => {
-    if (auditAssertion.equal) {
+  comparisons.forEach(assertion => {
+    if (assertion.equal) {


feels like we've still got quite a bit of assertion/comparison mixing going on throughout but that was preexisting :)

It feels like it should always be comparison? Assertion seems more narrow in scope to me than what we have here.

agreed :) cleanup for another time

brendankenny

💥

brendankenny · 2019-04-11T22:42:00Z

I really don't understand the lint issue here. They aren't inconsistently quoted, and I can't get the failure in a completely fresh download and install of the lighthouse repo

brendankenny · 2019-04-11T23:23:37Z

welllll, I'm going to override travis again, assuming that when this is merged back into master everything will be happy. We'll see how this goes for the second time this week :)

Artifact assertions

7c1ed2d

mattzeunert requested review from brendankenny and patrickhulce as code owners April 7, 2019 17:22

mattzeunert added 3 commits April 7, 2019 18:35

Revert "Artifact assertions"

1c16558

This reverts commit 7c1ed2d.

Wrap expectations in LHR

97fe391

Update smoke test logic to support lhr/audit and artifact assertions

88d5a83

patrickhulce reviewed Apr 7, 2019

View reviewed changes

mattzeunert added 4 commits April 8, 2019 13:17

Move artifacts to .tmp

b499b52

Make errorCode top level

e91d5cd

Remove empty line

810b37e

Fix types and actually assert stuff

ecc7091

patrickhulce approved these changes Apr 8, 2019

View reviewed changes

mattzeunert added 2 commits April 8, 2019 14:59

Style tweaks

bc81d78

Merge comparisons/assertions

fc85de0

brendankenny reviewed Apr 9, 2019

View reviewed changes

brendankenny and others added 4 commits April 10, 2019 07:20

Read artifacts file as string instead of buffer

d668e24

Co-Authored-By: mattzeunert <matt@mostlystatic.com>

PR feedback

9970b6a

ExpectedRunNERResult

6d4f206

category -> name

6714136

patrickhulce approved these changes Apr 10, 2019

View reviewed changes

expectedArtifacts var

e5d34ea

patrickhulce mentioned this pull request Apr 11, 2019

tests: add artifact snapshot testing #8190

Closed

brendankenny approved these changes Apr 11, 2019

View reviewed changes

brendankenny merged commit 9faf2ff into master Apr 11, 2019

brendankenny deleted the artifacts-assertions branch April 11, 2019 23:25

brendankenny mentioned this pull request Apr 11, 2019

misc: fix bad auto-merge #8201

Merged

connorjclark pushed a commit that referenced this pull request Apr 13, 2019

tests(smokehouse): support artifacts assertions in smoke tests (#8044)

48a346d

Conversation

mattzeunert commented Apr 7, 2019

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Apr 8, 2019

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattzeunert commented Apr 9, 2019

Uh oh!

brendankenny commented Apr 9, 2019

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny commented Apr 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

brendankenny commented Apr 11, 2019

Uh oh!

brendankenny commented Apr 11, 2019

Uh oh!

Reviewers

brendankenny commented Apr 9, 2019 •

edited

Loading