tests(smokehouse): assert on expected array length #9292

connorjclark · 2019-06-27T04:08:39Z

Proposal to change arrays in expectations to this behavior:

{
	expected: []
}

passes: []
fails: literally anything else

------------------------------

{
	expected: [1, 2, 3]
}

passes: [1, 2, 3]
fails: literally anything else

------------------------------

{
	expected: {length: 2}
}

passes: [1, 2] [1, 3] [x, y] {length: 2}
fails: [] [1] [1, 2, 3]

------------------------------

{
	expected: {0: 'a', 1: 'b'}
}

passes: ['a', 'b'] ['a', 'b', 'blah']
fails: ['b', 'a']

------------------------------

{
	expected: {0: 'a', 1: 'b', length: 3}
}

passes: ['a', 'b', *]
fails: ['a', 'b']

connorjclark · 2019-06-27T04:11:09Z

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

@@ -24,10 +26,10 @@ module.exports = [
    artifacts: {
      PageLoadError: {code: 'PAGE_HUNG'},
      devtoolsLogs: {
-        'pageLoadError-defaultPass': [/* ... */],
+        'pageLoadError-defaultPass': IS_ARRAY,


I admit this is not perfect.

But "I expect an array with these exact values, and no more" is more common than "I expect an array", so I'd rather [a, b, c] mean the former and not "expected array is a prefix of actual array"

agreed the array use case is more important

definitely weird that IS_ARRAY is an object instead of an array though :)

maybe we can comment this one-time usage that we can't use an array and at least assert {length: '>0'} or something?

patrickhulce

LGTM % IS_ARRAY nit

patrickhulce · 2019-07-08T22:23:44Z

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

@@ -24,10 +26,10 @@ module.exports = [
    artifacts: {
      PageLoadError: {code: 'PAGE_HUNG'},
      devtoolsLogs: {
-        'pageLoadError-defaultPass': [/* ... */],
+        'pageLoadError-defaultPass': IS_ARRAY,


agreed the array use case is more important

definitely weird that IS_ARRAY is an object instead of an array though :)

maybe we can comment this one-time usage that we can't use an array and at least assert {length: '>0'} or something?

patrickhulce · 2019-07-08T22:26:54Z

lighthouse-cli/test/smokehouse/smokehouse-report.js

+  // {0: x, 1: y, 2: z, length: 5} does not match [x, y, z].
+  if (Array.isArray(expected) && actual.length !== expected.length) {
+    return {
+      path,


should we do

path: `${path}.length`,

?

agreed that printing the full array value is best though

brendankenny · 2019-07-08T22:51:13Z

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

@@ -5,6 +5,12 @@
 */
 'use strict';

+// Just using `[]` actually asserts for an empty array.
+// Use this expectation object to assert an array with at least one element.
+const IS_NONEMPTY_ARRAY = {


the "is" prefix is usually for tests. NONEMPTY_ARRAY seems better

(we also use this convention all over the expectations (e.g. dbw and byte-efficiency), so I'm not sure it's really worth calling out here in particular vs just inlining, but it's also fine if you want to keep)

lighthouse-cli/test/smokehouse/smokehouse-report.js

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

googlebot · 2019-07-08T23:03:39Z

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.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-08T23:04:59Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

tests(smokehouse): assert on expected array length

3b8076e

connorjclark requested review from brendankenny and patrickhulce as code owners June 27, 2019 04:08

googlebot added the cla: yes label Jun 27, 2019

connorjclark commented Jun 27, 2019

View reviewed changes

lint

c304111

vercel bot deployed to staging June 27, 2019 05:26 View deployment

patrickhulce approved these changes Jul 8, 2019

View reviewed changes

pr

886a109

vercel bot deployed to staging July 8, 2019 22:46 View deployment

brendankenny approved these changes Jul 8, 2019

View reviewed changes

Update lighthouse-cli/test/smokehouse/smokehouse-report.js

4132e07

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

googlebot added cla: no and removed cla: yes labels Jul 8, 2019

vercel bot temporarily deployed to staging July 8, 2019 23:03 Inactive

drop the 'is'. it's cleaner.

2546943

vercel bot deployed to staging July 8, 2019 23:04 View deployment

connorjclark added cla: yes and removed cla: no labels Jul 8, 2019

connorjclark merged commit 4c658d2 into master Jul 9, 2019

brendankenny deleted the smoke-assert-array-length branch July 9, 2019 23:03

connorjclark mentioned this pull request Oct 15, 2019

tests: refactor smokehouse for extensibility #9843

Merged

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests(smokehouse): assert on expected array length #9292

tests(smokehouse): assert on expected array length #9292

tests(smokehouse): assert on expected array length #9292

tests(smokehouse): assert on expected array length #9292

Conversation

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