Skip to content
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.

Sign up for GitHub

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

Jump to bottom

report: add Trust & Safety group under Best Practices #10623

Merged
merged 13 commits into from
Apr 30, 2020
Merged

report: add Trust & Safety group under Best Practices #10623

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ec2be2d
Add T&S subcategory under Best Practices
Beytoven Apr 23, 2020
2b3d5f0
Updating sample_v2.json
Beytoven Apr 23, 2020
7494909
Missing comma :/
Beytoven Apr 23, 2020
e304ec8
Adding more sub categories
Beytoven Apr 23, 2020
ffedbc0
Add fix for bug in config test where deepcopy of Config was broken fo…
Beytoven Apr 24, 2020
084467f
Update snapshot
Beytoven Apr 24, 2020
723352a
Update offline smoke test to reflect audits being migrated out
Beytoven Apr 24, 2020
593fd59
Apply wording suggestions from Patrick and Brendan
Beytoven Apr 29, 2020
9d61065
More renaming and improve config unit test
Beytoven Apr 29, 2020
acbb6e4
Reword failure title for no-document-write audit
Beytoven Apr 29, 2020
2237f99
Update snapshots
Beytoven Apr 29, 2020
722ff75
Apply Patrick's suggestions from code review
Beytoven Apr 29, 2020
7a30125
Merge branch 'master' into trust_safety
Beytoven Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 65 additions & 24 deletions lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,73 +680,75 @@ Object {
"best-practices": Object {
"auditRefs": Array [
Object {
"id": "appcache-manifest",
"group": "best-practices-safety",
"id": "is-on-https",
"weight": 1,
},
Object {
"id": "is-on-https",
"group": "best-practices-safety",
"id": "external-anchors-use-rel-noopener",
"weight": 1,
},
Object {
"id": "uses-http2",
"group": "best-practices-safety",
"id": "geolocation-on-start",
"weight": 1,
},
Object {
"id": "uses-passive-event-listeners",
"group": "best-practices-safety",
"id": "notification-on-start",
"weight": 1,
},
Object {
"id": "no-document-write",
"group": "best-practices-safety",
"id": "no-vulnerable-libraries",
"weight": 1,
},
Object {
"id": "external-anchors-use-rel-noopener",
"group": "best-practices-ux",
"id": "password-inputs-can-be-pasted-into",
"weight": 1,
},
Object {
"id": "geolocation-on-start",
"group": "best-practices-ux",
"id": "image-aspect-ratio",
"weight": 1,
},
Object {
"group": "best-practices-ux",
"id": "image-size-responsive",
"weight": 1,
},
Object {
"group": "best-practices-browser",
"id": "doctype",
"weight": 1,
},
Object {
"group": "best-practices-browser",
"id": "charset",
"weight": 1,
},
Object {
"id": "no-vulnerable-libraries",
"group": "best-practices-general",
"id": "appcache-manifest",
"weight": 1,
},
Object {
"group": "best-practices-general",
"id": "js-libraries",
"weight": 0,
},
Object {
"id": "notification-on-start",
"weight": 1,
},
Object {
"group": "best-practices-general",
"id": "deprecations",
"weight": 1,
},
Object {
"id": "password-inputs-can-be-pasted-into",
"weight": 1,
},
Object {
"group": "best-practices-general",
"id": "errors-in-console",
"weight": 1,
},
Object {
"id": "image-aspect-ratio",
"weight": 1,
},
Object {
"id": "image-size-responsive",
"weight": 1,
},
],
"title": "Best Practices",
},
Expand Down Expand Up @@ -933,6 +935,21 @@ Object {
"id": "third-party-summary",
"weight": 0,
},
Object {
"group": "diagnostics",
"id": "uses-http2",
"weight": 0,
},
Object {
"group": "diagnostics",
"id": "uses-passive-event-listeners",
"weight": 0,
},
Object {
"group": "diagnostics",
"id": "no-document-write",
"weight": 0,
},
Object {
"id": "network-requests",
"weight": 0,
Expand Down Expand Up @@ -1167,6 +1184,18 @@ Object {
"description": "These are opportunities to to improve the experience of reading tabular or list data using assistive technology, like a screen reader.",
"title": "Tables and lists",
},
"best-practices-browser": Object {
"title": "Browser Compatibility",
},
"best-practices-general": Object {
"title": "General",
},
"best-practices-safety": Object {
"title": "Trust & Safety",
},
"best-practices-ux": Object {
"title": "User Experience",
},
"budgets": Object {
"description": "Performance budgets set standards for the performance of your site.",
"title": "Budgets",
Expand Down Expand Up @@ -1430,6 +1459,18 @@ Object {
"description": "These are opportunities to to improve the experience of reading tabular or list data using assistive technology, like a screen reader.",
"title": "Tables and lists",
},
"best-practices-browser": Object {
"title": "Browser Compatibility",
},
"best-practices-general": Object {
"title": "General",
},
"best-practices-safety": Object {
"title": "Trust & Safety",
},
"best-practices-ux": Object {
"title": "User Experience",
},
"budgets": Object {
"description": "Performance budgets set standards for the performance of your site.",
"title": "Budgets",
Expand Down
9 changes: 0 additions & 9 deletions lighthouse-cli/test/smokehouse/test-definitions/offline-local/offline-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ module.exports = [
'is-on-https': {
score: 1,
},
'uses-http2': {
score: 0,
},
'external-anchors-use-rel-noopener': {
score: 1,
},
Expand All @@ -35,12 +32,6 @@ module.exports = [
'render-blocking-resources': {
score: 1,
},
'no-document-write': {
score: 1,
},
'uses-passive-event-listeners': {
score: 1,
},
'password-inputs-can-be-pasted-into': {
score: 1,
},
Expand Down
58 changes: 41 additions & 17 deletions lighthouse-core/config/default-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,14 @@ const UIStrings = {
'not automatically checked by Lighthouse. They do not affect your score but it\'s important that you verify them manually.',
/** Title of the Best Practices category of audits. This is displayed at the top of a list of audits focused on topics related to following web development best practices and accepted guidelines. Also used as a label of a score gauge; try to limit to 20 characters. */
bestPracticesCategoryTitle: 'Best Practices',
/** Title of the Trust & Safety group of the Best Practices category. Within this section are the audits related to trust and safety. */
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
bestPracticesSafetyGroupTitle: 'Trust & Safety',
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
/** Title of the User Experience group of the Best Practices category. Within this section are the audits related to trust and safety. */
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
bestPracticesUXGroupTitle: 'User Experience',
/** Title of the Browser Compatibility group of the Best Practices category. Within this section are the audits related to trust and safety. */
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
bestPracticesBrowserGroupTitle: 'Browser Compatibility',
/** Title of the General group of the Best Practices category. Within this section are the audits that don't belong to a specific group. */
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
bestPracticesGeneralGroupTitle: 'General',
/** Title of the Fast and Reliable section of the web app category. Within this section are audits that check if the web site loaded quickly and can reliably load even if the internet connection is very slow or goes offline. */
pwaFastReliableGroupTitle: 'Fast and reliable',
/** Title of the Installable section of the web app category. Within this section are audits that check if Chrome supports installing the web site as an app on their device. */
Expand Down Expand Up @@ -384,6 +392,18 @@ const defaultConfig = {
title: str_(UIStrings.seoCrawlingGroupTitle),
description: str_(UIStrings.seoCrawlingGroupDescription),
},
'best-practices-safety': {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe best-practices-trust-safety?

If I come in looking for the Trust and Safety group, my ctrl-f would definitely start with "trust" :)

title: str_(UIStrings.bestPracticesSafetyGroupTitle),
},
'best-practices-ux': {
title: str_(UIStrings.bestPracticesUXGroupTitle),
},
'best-practices-browser': {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

best-practices-compat if we're abbreviating? could also just be more verbose for this one best-practices-browser-compat

browser feels a bit too vague since all of this in a browser :)

title: str_(UIStrings.bestPracticesBrowserGroupTitle),
},
'best-practices-general': {
title: str_(UIStrings.bestPracticesGeneralGroupTitle),
},
},
categories: {
'performance': {
Expand Down Expand Up @@ -428,6 +448,9 @@ const defaultConfig = {
{id: 'timing-budget', weight: 0, group: 'budgets'},
{id: 'resource-summary', weight: 0, group: 'diagnostics'},
{id: 'third-party-summary', weight: 0, group: 'diagnostics'},
{id: 'uses-http2', weight: 0, group: 'diagnostics'},
{id: 'uses-passive-event-listeners', weight: 0, group: 'diagnostics'},
{id: 'no-document-write', weight: 0, group: 'diagnostics'},
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
// Audits past this point don't belong to a group and will not be shown automatically
{id: 'network-requests', weight: 0},
{id: 'network-rtt', weight: 0},
Expand Down Expand Up @@ -505,23 +528,24 @@ const defaultConfig = {
'best-practices': {
title: str_(UIStrings.bestPracticesCategoryTitle),
auditRefs: [
{id: 'appcache-manifest', weight: 1},
{id: 'is-on-https', weight: 1},
{id: 'uses-http2', weight: 1},
{id: 'uses-passive-event-listeners', weight: 1},
{id: 'no-document-write', weight: 1},
{id: 'external-anchors-use-rel-noopener', weight: 1},
{id: 'geolocation-on-start', weight: 1},
{id: 'doctype', weight: 1},
{id: 'charset', weight: 1},
{id: 'no-vulnerable-libraries', weight: 1},
{id: 'js-libraries', weight: 0},
{id: 'notification-on-start', weight: 1},
{id: 'deprecations', weight: 1},
{id: 'password-inputs-can-be-pasted-into', weight: 1},
{id: 'errors-in-console', weight: 1},
{id: 'image-aspect-ratio', weight: 1},
{id: 'image-size-responsive', weight: 1},
// Trust & Safety
{id: 'is-on-https', weight: 1, group: 'best-practices-safety'},
{id: 'external-anchors-use-rel-noopener', weight: 1, group: 'best-practices-safety'},
{id: 'geolocation-on-start', weight: 1, group: 'best-practices-safety'},
{id: 'notification-on-start', weight: 1, group: 'best-practices-safety'},
{id: 'no-vulnerable-libraries', weight: 1, group: 'best-practices-safety'},
// User Experience
{id: 'password-inputs-can-be-pasted-into', weight: 1, group: 'best-practices-ux'},
{id: 'image-aspect-ratio', weight: 1, group: 'best-practices-ux'},
{id: 'image-size-responsive', weight: 1, group: 'best-practices-ux'},
// Browser Compatibility
{id: 'doctype', weight: 1, group: 'best-practices-browser'},
{id: 'charset', weight: 1, group: 'best-practices-browser'},
// General Group
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
{id: 'appcache-manifest', weight: 1, group: 'best-practices-general'},
{id: 'js-libraries', weight: 0, group: 'best-practices-general'},
{id: 'deprecations', weight: 1, group: 'best-practices-general'},
{id: 'errors-in-console', weight: 1, group: 'best-practices-general'},
],
},
'seo': {
Expand Down
12 changes: 12 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-US.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,9 +1331,21 @@
"lighthouse-core/config/default-config.js | a11yTablesListsVideoGroupTitle": {
"message": "Tables and lists"
},
"lighthouse-core/config/default-config.js | bestPracticesBrowserGroupTitle": {
"message": "Browser Compatibility"
},
"lighthouse-core/config/default-config.js | bestPracticesCategoryTitle": {
"message": "Best Practices"
},
"lighthouse-core/config/default-config.js | bestPracticesGeneralGroupTitle": {
"message": "General"
},
"lighthouse-core/config/default-config.js | bestPracticesSafetyGroupTitle": {
"message": "Trust & Safety"
},
"lighthouse-core/config/default-config.js | bestPracticesUXGroupTitle": {
"message": "User Experience"
},
"lighthouse-core/config/default-config.js | budgetsGroupDescription": {
"message": "Performance budgets set standards for the performance of your site."
},
Expand Down
12 changes: 12 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-XL.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1331,9 +1331,21 @@
"lighthouse-core/config/default-config.js | a11yTablesListsVideoGroupTitle": {
"message": "T̂áb̂ĺêś âńd̂ ĺîśt̂ś"
},
"lighthouse-core/config/default-config.js | bestPracticesBrowserGroupTitle": {
"message": "B̂ŕôẃŝér̂ Ćôḿp̂át̂íb̂íl̂ít̂ý"
},
"lighthouse-core/config/default-config.js | bestPracticesCategoryTitle": {
"message": "B̂éŝt́ P̂ŕâćt̂íĉéŝ"
},
"lighthouse-core/config/default-config.js | bestPracticesGeneralGroupTitle": {
"message": "Ĝén̂ér̂ál̂"
},
"lighthouse-core/config/default-config.js | bestPracticesSafetyGroupTitle": {
"message": "T̂ŕûśt̂ & Śâf́êt́ŷ"
},
"lighthouse-core/config/default-config.js | bestPracticesUXGroupTitle": {
"message": "Ûśêŕ Êx́p̂ér̂íêńĉé"
},
"lighthouse-core/config/default-config.js | budgetsGroupDescription": {
"message": "P̂ér̂f́ôŕm̂án̂ćê b́ûd́ĝét̂ś ŝét̂ śt̂án̂d́âŕd̂ś f̂ór̂ t́ĥé p̂ér̂f́ôŕm̂án̂ćê óf̂ ýôúr̂ śît́ê."
},
Expand Down
15 changes: 14 additions & 1 deletion lighthouse-core/test/config/config-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1161,8 +1161,21 @@ describe('Config', () => {
};
const extendedConfig = new Config(extended);

// When gatherers have instance properties that are bind()'d, they'll not match.
// We'll still continue to diff the constructor via .implementation
Beytoven marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/GoogleChrome/lighthouse/pull/10090#discussion_r382864319
function deleteInstancesForTest(config) {
for (const pass of config.passes) {
for (const gatherer of pass.gatherers) {
delete gatherer.instance;
}
}
}
deleteInstancesForTest(extendedConfig);
deleteInstancesForTest(config);

assert.equal(config.passes.length, 1, 'did not filter config');
assert.deepStrictEqual(config, extendedConfig, 'had mutations');
expect(config).toMatchObject(extendedConfig, 'had mutations');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toMatchObject here only checks that everything in extendedConfig is in config, which means config can be extendedConfig plus extra non-matching properties and this assertion would still pass.

e.g. if one of the gatherer.implementation in extendedConfig accidentally got switched to {}, it would match with almost anything in the same gatherer.implementation slot in config.

This could be checked both ways (each is a subset of the other) with

expect(config).toMatchObject(extendedConfig, 'had mutations');
expect(extendedConfig).toMatchObject(config, 'had mutations');

to check both directions but I think we really want

expect(config).toEqual(extendedConfig, 'had mutations');

to make sure we're being strict on equality.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh good catch, while we're on the subject I don't think the message part works with that particular expect API, so we can nix had mutations or convert it to a comment and stick to the actual object diff

});

it('should filter out other passes if passed Performance', () => {
Expand Down
Loading