update smokehouse to do deep result comparisons #1450

brendankenny · 2017-01-11T18:18:36Z

Allows smokehouse expectations to be objects now. Smokehouse will walk the expected value until getting to any primitive leaves, comparing to the actual result at every step, failing if there is any difference. This allows for the expected value to not be complete, so it doesn't have to include noisy details (e.g. audit meta info) better checked by unit tests.

I augmented the PWA and offline smoke test expectations files, but it turns out all those older audits don't include anything that interesting in their extended info (at least compared to newer audits). manifest-theme-color doesn't even output what the theme_color actually is. We can kick off the DBW deep expectations after this PR.

Originally I planned to allow the old expectations format (if it's just a primitive value, assume that it's the score property on the actual audit result), but it's really not much work to just explicitly write out the object literal with a score property. The literals add considerably to the length of the file, but it means you don't have to keep that special case in mind while writing expectations files. In practice we tend to write these values once and barely touch them again, so it seems fine to be explicit. If others feel strongly about the shorter syntax when you only care about the audit's score, I can add that back in.

This doesn't quite finish #964, as this doesn't include comparator support (e.g. if you want to test the actual value with a regex or a floating point comparison (TTI is ≤ 5000 or whatever)), but that's an easy step after these changes.

brendankenny · 2017-01-11T18:27:37Z

as an example, if you wanted to test the result of a manifest-background-color audit, the full audit result might be

"manifest-background-color": {
  "score": true,
  "displayValue": "",
  "rawValue": true,
  "extendedInfo": {
    "value": "#bababa",
    "formatter": "null"
  },
  "name": "manifest-background-color",
  "category": "Manifest",
  "description": "Manifest contains `background_color`"
},

but much of this is boilerplate and better testable at the audit unit test level (we also don't want to update a bunch of test expectations every time we switch helpText strings).

So for your expected result you might write

'manifest-background-color': {
  score: true,
  extendedInfo: {
    value: '#bababa'
  }
}

This tests only the score and extendedInfo.value values of the audit result, ignoring everything else.

(In this example the value is really testing the manifest parser (which is tested elsewhere), so a better use of this is in the DBW mega test, where we can check things like violation counts, to make sure all corner cases are correctly found and handled)

patrickhulce

What are your thoughts on allowing the expectations to be functions and passing an assert or some similar chai expect object (with maybe a non-function type defaulting to .score equality? Just don't want to find ourselves basically writing a new test comparison assertion library for a lighthouse-specific expectations when there are some pretty good ones out there. It's not super clear to me how this extends to arbitrary comparisons we might want to make (existence of something in a list for example). Yay for improved smokehouse testing! :)

patrickhulce · 2017-01-11T22:27:10Z

lighthouse-cli/test/smokehouse/smokehouse.js

 */
 function reportAssertion(assertion) {
  if (assertion.equal) {
    console.log(`  ${log.greenify(log.tick)} ${assertion.category}: ` +
        log.greenify(assertion.actual));
  } else {
-    console.log(`  ${log.redify(log.cross)} ${assertion.category}: ` +
-        log.redify(`found ${assertion.actual}, expected ${assertion.expected}`));
+    if (!assertion.diff) {


can you flip this if/else? had to pause for what this was meaning !equal && !diff === 'diff was just missing' :)

patrickhulce · 2017-01-11T22:28:53Z

lighthouse-cli/test/smokehouse/smokehouse.js

+
+  // We only care that all expected's own properties are on actual (and not the other way around).
+  for (const key of Object.keys(expected)) {
+    // Bracket numbers, but property names requiring quotes will still be unquoted.


why? do we use this for anything other than informing the user?

yeah, this is just for nice formatting. myArray.1.value looked weird :)

haha gotcha

lol myArray.1.value is what polymer uses for it's API.

brendankenny · 2017-01-11T23:07:23Z

What are your thoughts on allowing the expectations to be functions and passing an assert or some similar chai expect object (with maybe a non-function type defaulting to .score equality? Just don't want to find ourselves basically writing a new test comparison assertion library for a lighthouse-specific expectations when there are some pretty good ones out there.

The main issue I had was I couldn't find anyone doing one-sided deep equality checking, so that if you don't include something on expected it didn't care if it was on actual. That leaves you writing a comparison that checks the subset of values you're interested in yourself, checking actual.score, then actual.extendedInfo, then actual.extendedInfo.value. To me the declarative form of that, which directly mirrors the actual output LH gives for a run, was simpler and more readable.

This isn't so bad (if this is what you mean):

// results defined up here somewhere
it('correctly does whatever with manifest-background-color', () => {
  const manifestBgColorResult = results.audits['manifest-background-color'];
  assert.strictEqual(manifestBgColorResult.score, true);
  assert(manifestBgColorResult.extendedInfo);
  assert.strictEqual(manifestBgColorResult.extendedInfo.value, '#bababa');
});

but

'manifest-background-color': {
  score: true,
  extendedInfo: {
    value: '#bababa'
  }
}

scans a lot easier to me.

It's not super clear to me how this extends to arbitrary comparisons we might want to make (existence of something in a list for example)

basically if when walking the expected result the expected value is a function, we call the function here with the actual result, and it can do what it likes. That could return a boolean or, if we wanted, throw an assertion error (which would make deciding what to show in expected vs found a lot easier). This could also be extended to support regexp objects (test against actual) and the numeric string conditionals some people like (">=5000" or whatever, now used by pwmetrics)

patrickhulce · 2017-01-12T00:20:10Z

The main issue I had was I couldn't find anyone doing one-sided deep equality checking

Yeah none that I know of either, but assert is pretty limited here and chai's nested deep property checking doesn't make it that bad IMO, but given

basically if when walking the expected result the expected value is a function, we call the function here with the actual result, and it can do what it likes.

Basically what I was looking for, SGTM!

ebidel · 2017-01-12T00:24:50Z

I'll give this a look on the way home.

ebidel · 2017-01-11T18:21:34Z

lighthouse-cli/test/smokehouse/pwa-expectations.js

-      'manifest-start-url': true,
-      // 'cache-start-url': true
+      'is-on-https': {
+        score: true,


trailing ",". Fine by me but you don't do this elsewhere.

ebidel · 2017-01-12T01:31:33Z

lighthouse-cli/test/smokehouse/pwa-expectations.js

+        }
+      },
+      'manifest-theme-color': {
+        score: true


does this one have a color val we can check?

no, since it was written before extendedInfo it doesn't include it

ebidel · 2017-01-12T01:32:06Z

lighthouse-cli/test/smokehouse/pwa-expectations.js

+      'manifest-icons-min-144': {
+        score: true
+      },
+      'manifest-name': {


I assume the name and urls don't have a val we can check?

I assume the name and urls don't have a val we can check?

nope, none of these older ones do

ebidel · 2017-01-12T01:37:38Z

lighthouse-cli/test/smokehouse/smokehouse.js

+
+  // We only care that all expected's own properties are on actual (and not the other way around).
+  for (const key of Object.keys(expected)) {
+    // Bracket numbers, but property names requiring quotes will still be unquoted.


lol myArray.1.value is what polymer uses for it's API.

ebidel · 2017-01-12T01:39:58Z

lighthouse-cli/test/smokehouse/smokehouse.js

+    return null;
+  }
+
+  // if they aren't both an object, reject.


not sure what "reject" means.

Maybe something other than "reject". ...Can't create diff if actual and expected are both not objects.

Good point. Is this more understandable?

brendankenny · 2017-01-12T02:17:42Z

updated

ebidel · 2017-01-12T03:45:34Z

⬇️ 🕳 🚬 🔢 💯

* update smokehouse to do deep result comparisons * add deeper offline and PWA smoketests expectations

brendankenny force-pushed the deepsmoke branch from 2f7df18 to d5e9f79 Compare January 11, 2017 19:50

patrickhulce suggested changes Jan 11, 2017

View reviewed changes

brendankenny force-pushed the deepsmoke branch from d5e9f79 to b5012e7 Compare January 11, 2017 23:16

patrickhulce approved these changes Jan 12, 2017

View reviewed changes

ebidel approved these changes Jan 12, 2017

View reviewed changes

brendankenny added 2 commits January 11, 2017 18:16

update smokehouse to do deep result comparisons

541a064

add deeper offline and PWA smoketests expectations

8b1b4cb

brendankenny force-pushed the deepsmoke branch from b5012e7 to 8b1b4cb Compare January 12, 2017 02:17

ebidel merged commit df9e8f1 into master Jan 12, 2017

ebidel deleted the deepsmoke branch January 12, 2017 03:45

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017

Update smokehouse to do deep result comparisons (GoogleChrome#1450)

ba97c53

* update smokehouse to do deep result comparisons * add deeper offline and PWA smoketests expectations

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update smokehouse to do deep result comparisons #1450

update smokehouse to do deep result comparisons #1450

brendankenny commented Jan 11, 2017

brendankenny commented Jan 11, 2017 •

edited

Loading

patrickhulce left a comment

patrickhulce Jan 11, 2017

brendankenny Jan 11, 2017

patrickhulce Jan 11, 2017

brendankenny Jan 11, 2017

patrickhulce Jan 12, 2017

ebidel Jan 12, 2017

brendankenny commented Jan 11, 2017

patrickhulce commented Jan 12, 2017 •

edited

Loading

ebidel commented Jan 12, 2017

ebidel Jan 11, 2017

brendankenny Jan 12, 2017

ebidel Jan 12, 2017

brendankenny Jan 12, 2017

ebidel Jan 12, 2017

brendankenny Jan 12, 2017

ebidel Jan 12, 2017

ebidel Jan 12, 2017

ebidel Jan 12, 2017

brendankenny Jan 12, 2017

brendankenny commented Jan 12, 2017

ebidel commented Jan 12, 2017

update smokehouse to do deep result comparisons #1450

update smokehouse to do deep result comparisons #1450

Conversation

brendankenny commented Jan 11, 2017

brendankenny commented Jan 11, 2017 • edited Loading

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

brendankenny commented Jan 11, 2017

patrickhulce commented Jan 12, 2017 • edited Loading

ebidel commented Jan 12, 2017

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

brendankenny commented Jan 12, 2017

ebidel commented Jan 12, 2017

brendankenny commented Jan 11, 2017 •

edited

Loading

patrickhulce commented Jan 12, 2017 •

edited

Loading