-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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(font-size): add test for attributing styles to node #9400
Changes from 1 commit
c911a40
01c44e3
0b2c27f
5faf245
faaa9c0
c2f8403
d61a5ff
8b216de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -239,7 +239,7 @@ describe('SEO: Font size audit', () => { | |
}); | ||
|
||
describe('attributes source location', () => { | ||
function runFontSizeAuditWithSingleFailingStyle(style, nodeProperties) { | ||
async function runFontSizeAuditWithSingleFailingStyle(style, nodeProperties) { | ||
const artifacts = { | ||
URL: {finalUrl: 'http://www.example.com'}, | ||
MetaElements: makeMetaElements(validViewport), | ||
|
@@ -250,7 +250,9 @@ describe('SEO: Font size audit', () => { | |
}, | ||
TestedAsMobileDevice: true, | ||
}; | ||
return FontSizeAudit.audit(artifacts, getFakeContext()); | ||
const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext()); | ||
expect(auditResult.details.items).toHaveLength(1); | ||
return auditResult; | ||
} | ||
|
||
it('to inline node stylesheet', async () => { | ||
|
@@ -262,7 +264,6 @@ describe('SEO: Font size audit', () => { | |
attributes: ['class', 'my-p'], | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a separate review ongoing with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you even tried to warn me over chat and it didn't work! 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope, this was it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
there was one in #9354, though? #9400 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes... and I moved it here and asked patrick to review over chat :) a flawless system as you see |
||
assert.equal(auditResult.details.items.length, 1); | ||
expect(auditResult.details.items[0].selector).toMatchObject({ | ||
type: 'node', | ||
selector: '#my-parent', | ||
|
@@ -279,7 +280,6 @@ describe('SEO: Font size audit', () => { | |
attributes: ['size', '10px'], | ||
}); | ||
|
||
assert.equal(auditResult.details.items.length, 1); | ||
expect(auditResult.details.items[0].selector).toMatchObject({ | ||
type: 'node', | ||
selector: '#my-parent', | ||
|
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.
this is also not a valid
URL
artifact :)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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not necessarily, I just found it funny :)
(
URL
is declared at the top level, though, so could be reused likevalidViewport
is)