tests(smokehouse): fix unintentional 404. remove max-len rule for expectations #9665
Conversation
connorjclark
requested review from
brendankenny and
patrickhulce
as code owners
|
yaaaaa we shouldn't have line limits in these files. |
connorjclark
changed the title
| @@ -30,7 +30,7 @@ | |||
| <!-- Note: these will only fail when using the static-server.js, which supports the ?delay=true param. | |||
| If you're using your own server, the resource will load instantly and the | |||
| stylesheets will be ignored for being below the threshold. --> | |||
| <link rel="stylesheet" href="./dobetterweb/dbw_tester.css?delay=100"> <!-- FAIL, when run under smokehouse --> | |||
| <link rel="stylesheet" href="./dbw_tester.css?delay=100"> <!-- FAIL, when run under smokehouse --> | |||
There was a problem hiding this comment.
anyone know what FAIL, when run under smokehouse is alluding to? when would this pass? Should I just replace with FAIL?
There was a problem hiding this comment.
also, I wish we put the audit id that these failed for.
There was a problem hiding this comment.
it's referring to the Note: above, this will fail but only when run under smokehouse and not a generic static file server
There was a problem hiding this comment.
personally I think all this caveat is nonsense at this point because like half of smokehouse would be different if not run in our custom server, so 🤷♂
There was a problem hiding this comment.
I agree. if mentioned at all, it should be noted in a README describing how these smoke tests work. i'll leave as-is for a follow up.
There was a problem hiding this comment.
also, I wish we put the audit id that these failed for.
is it possible to check this and add a note since it's being changed anyways? Presumably by the comment something currently failing should pass if there isn't a delay
There was a problem hiding this comment.
it actually didn't really fail anything (only failures were b/c the name changes) ...
There was a problem hiding this comment.
it actually didn't really fail anything (only failures were b/c the name changes) ...
make sense to remove the <!-- FAIL, when run under smokehouse --> then?
connorjclark
changed the title
connorjclark
changed the title
| @@ -73,6 +73,14 @@ module.exports = { | |||
| 'valid-jsdoc': 0, | |||
| 'arrow-parens': 0, | |||
| }, | |||
| overrides: [ | |||
| { | |||
There was a problem hiding this comment.
can you add a comment to this block so it's clearer what it's for?
There was a problem hiding this comment.
narrowing seems to make it self documenting
| @@ -30,7 +30,7 @@ | |||
| <!-- Note: these will only fail when using the static-server.js, which supports the ?delay=true param. | |||
| If you're using your own server, the resource will load instantly and the | |||
| stylesheets will be ignored for being below the threshold. --> | |||
| <link rel="stylesheet" href="./dobetterweb/dbw_tester.css?delay=100"> <!-- FAIL, when run under smokehouse --> | |||
| <link rel="stylesheet" href="./dbw_tester.css?delay=100"> <!-- FAIL, when run under smokehouse --> | |||
There was a problem hiding this comment.
also, I wish we put the audit id that these failed for.
is it possible to check this and add a note since it's being changed anyways? Presumably by the comment something currently failing should pass if there isn't a delay
bug from this change: #5006
the path was mistranslated
i also expanded the console expectations to be an explicit array, because
{length: x}is smoky garbage for debugging