test(smokehouse): fix flakiness of smokehouse dom-size expectation #1881
Conversation
brendankenny
changed the title
| @@ -111,6 +111,10 @@ function findDifference(path, actual, expected) { | |||
| return null; | |||
| } | |||
|
|
|||
| if (expected instanceof RegExp && expected.test(actual)) { | |||
There was a problem hiding this comment.
expected.test() will do actual.toString() internally, so this check should be good for any value of actual, but if it fails this check, typeof expected will still be 'object', so it could pass the conditional below and then try to recurse on the regexp object instead of returning the difference. This could lead to a false positive result since Object.keys(/ul.versionlist/) is [], so any value of actual that has typeof 'object' will pass. e.g. findDifference('path', {}, /whatever/) will find no difference.
Simplest thing would be to also add || expected instanceof RegExp to the conditional below.
There was a problem hiding this comment.
Although there is also the minuscule chance that actual.toString() could yield a string that somehow passes expected.test(), so you may also want to add a typeof actual === 'string' above and below (or rearrange somehow) if that's not a feature we want to support :)
There was a problem hiding this comment.
surprised I don't see a RegExp.isRegExp() proposed anywhere
There was a problem hiding this comment.
Really? :P
well expected instanceof RegExp won't work if through some ridiculously circuitous process you're writing expectations values with a RegExp object from another realm, so the "real" way to check is like the old isArray check of Object.prototype.toString.call(expected) === '[object RegExp]'.
I would have thought someone, somewhere would have proposed RegExp.isRegExp() like Array.isArray() to TC39 at some point, but I couldn't find any record of such a thing :)
| @@ -111,6 +111,10 @@ function findDifference(path, actual, expected) { | |||
| return null; | |||
| } | |||
|
|
|||
| if (expected instanceof RegExp && expected.test(actual)) { | |||
There was a problem hiding this comment.
surprised I don't see a RegExp.isRegExp() proposed anywhere
|
how was this flaking, incidentally? What string was being generated instead? |
from a recent run:
|
|
Never did I expect so many comments for this... haha |
adds support for `RegExp` tests in smokehouse expected values
adds support for `RegExp` tests in smokehouse expected values
adds RegExp smokehouse to boot :)