-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tests(smokehouse): assert on expected array length #9292
Conversation
@@ -24,10 +26,10 @@ module.exports = [ | |||
artifacts: { | |||
PageLoadError: {code: 'PAGE_HUNG'}, | |||
devtoolsLogs: { | |||
'pageLoadError-defaultPass': [/* ... */], | |||
'pageLoadError-defaultPass': IS_ARRAY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % IS_ARRAY
nit
@@ -24,10 +26,10 @@ module.exports = [ | |||
artifacts: { | |||
PageLoadError: {code: 'PAGE_HUNG'}, | |||
devtoolsLogs: { | |||
'pageLoadError-defaultPass': [/* ... */], | |||
'pageLoadError-defaultPass': IS_ARRAY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
// {0: x, 1: y, 2: z, length: 5} does not match [x, y, z]. | ||
if (Array.isArray(expected) && actual.length !== expected.length) { | ||
return { | ||
path, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we do
path: `${path}.length`,
?
agreed that printing the full array value is best though
@@ -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 = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "is" prefix is usually for tests. NONEMPTY_ARRAY
seems better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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)
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
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. ℹ️ Googlers: Go here for more info. |
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. |
Extracted from #9248 (comment)
Proposal to change arrays in expectations to this behavior: