-
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(font-size): add test for attributing styles to node #9400
Changes from all commits
c911a40
01c44e3
0b2c27f
5faf245
faaa9c0
c2f8403
d61a5ff
8b216de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,10 @@ | |
const FontSizeAudit = require('../../../audits/seo/font-size.js'); | ||
const assert = require('assert'); | ||
|
||
const URL = 'https://example.com'; | ||
const URL = { | ||
requestedUrl: 'https://example.com', | ||
finalUrl: 'https://example.com', | ||
}; | ||
const validViewport = 'width=device-width'; | ||
|
||
/* eslint-env jest */ | ||
|
@@ -234,4 +237,54 @@ describe('SEO: Font size audit', () => { | |
expect(auditResult.score).toBe(1); | ||
expect(auditResult.notApplicable).toBe(true); | ||
}); | ||
|
||
describe('attributes source location', () => { | ||
async function runFontSizeAuditWithSingleFailingStyle(style, nodeProperties) { | ||
const artifacts = { | ||
URL: {finalUrl: 'http://www.example.com'}, | ||
MetaElements: makeMetaElements(validViewport), | ||
FontSize: { | ||
analyzedFailingNodesData: [ | ||
{textLength: 1, fontSize: 1, node: {nodeId: 1, ...nodeProperties}, cssRule: style}, | ||
], | ||
}, | ||
TestedAsMobileDevice: true, | ||
}; | ||
const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext()); | ||
expect(auditResult.details.items).toHaveLength(1); | ||
return auditResult; | ||
} | ||
|
||
it('to inline node stylesheet', async () => { | ||
const auditResult = await runFontSizeAuditWithSingleFailingStyle({ | ||
type: 'Inline', | ||
}, { | ||
parentNode: {attributes: ['id', 'my-parent']}, | ||
localName: 'p', | ||
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 |
||
expect(auditResult.details.items[0].selector).toMatchObject({ | ||
type: 'node', | ||
selector: '#my-parent', | ||
snippet: '<p class="my-p">', | ||
}); | ||
}); | ||
|
||
it('to attributes node stylesheet', async () => { | ||
const auditResult = await runFontSizeAuditWithSingleFailingStyle({ | ||
type: 'Attributes', | ||
}, { | ||
parentNode: {attributes: ['id', 'my-parent']}, | ||
localName: 'font', | ||
attributes: ['size', '10px'], | ||
}); | ||
|
||
expect(auditResult.details.items[0].selector).toMatchObject({ | ||
type: 'node', | ||
selector: '#my-parent', | ||
snippet: '<font size="10px">', | ||
}); | ||
}); | ||
}); | ||
}); |
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)