tests(font-size): add test for attributing styles to node #9400
Conversation
patrickhulce
left a comment
patrickhulce
left a comment
There was a problem hiding this comment.
ain't gonna complain about more tests! :)
LGTM
|
|
||
| const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext()); | ||
| assert.equal(auditResult.details.items.length, 2); | ||
| assert.equal(auditResult.details.items[0].source, URL.finalUrl); |
There was a problem hiding this comment.
expect(auditResult.details.items).toMatchObject
for the new stuff? :D
There was a problem hiding this comment.
done.
Also, I grabbed the describe fn I made in #9354 and moved it here. 1) will make the other review smaller 2) doing the same stuff here.
brendankenny
left a comment
brendankenny
left a comment
There was a problem hiding this comment.
Fixed it - but there were also no tests that used it. So I made one that covers attributing styles to nodes.
neither of these are asserting source, though? Or at least one back to a URL
Also, maybe it's just the nature of the artifact, but these are pretty inscrutable test cases :) What on earth is type 'Attributes'?
| describe('attributes source location', () => { | ||
| async function runFontSizeAuditWithSingleFailingStyle(style, nodeProperties) { | ||
| const artifacts = { | ||
| URL: {finalUrl: 'http://www.example.com'}, |
There was a problem hiding this comment.
this is also not a valid URL artifact :)
There was a problem hiding this comment.
do we want to make artifacts in tests fully complete, or just the parts we need for the test?
There was a problem hiding this comment.
do we want to make artifacts in tests fully complete, or just the parts we need for the test?
not necessarily, I just found it funny :)
(URL is declared at the top level, though, so could be reused like validViewport is)
| localName: 'p', | ||
| attributes: ['class', 'my-p'], | ||
| }); | ||
|
|
There was a problem hiding this comment.
Oh my gosh wait yeah this file is completely different!
I'm sorry @connorjclark I got confused between two different font-size-test changes and approved this immediately after looking at the other one, too many reviews today my b 😢
There was a problem hiding this comment.
is there a separate review ongoing with runFontSizeAuditWithSingleFailingStyle...
There was a problem hiding this comment.
you even tried to warn me over chat and it didn't work! 😆
There was a problem hiding this comment.
nope, this was it.
There was a problem hiding this comment.
nope, this was it.
there was one in #9354, though? #9400 (comment)
There was a problem hiding this comment.
yes... and I moved it here and asked patrick to review over chat :) a flawless system as you see
It's a css rule that was applied via a dom attribute |
4 participants
I noticed that the
URLartifact forfont-size-testwas invalid. Fixed it - but there were also no tests that used it. So I made one that covers attributing styles to nodes.