new_audit(canonical): document has a valid rel=canonical by kdzwinel · Pull Request #4163 · GoogleChrome/lighthouse

kdzwinel · 2018-01-03T00:54:52Z

Success:

Failure:

…th the same name).

kdzwinel · 2018-01-03T00:56:19Z

lighthouse-cli/test/fixtures/static-server.js

          if (HEADER_SAFELIST.has(headerName.toLowerCase())) {
-            headers[headerName] = headerValue;
+            headers[headerName] = headers[headerName] || [];
+            headers[headerName].push(headerValue);


Some headers can appear multiple times in the HTTP response (set-cookie, link, etc.) and, in such cases, {name:[value1, value2]} is a format expected by response.writeHead.

kdzwinel · 2018-01-03T01:01:37Z

lighthouse-core/audits/seo/canonical.js

+          canonicalURL.pathname === '/' && baseURL.pathname !== '/') {
+          return {
+            rawValue: false,
+            displayValue: 'points to a root of the same origin',


@rviscomi I made these displayValues very short and while this works for invalid URL all others require more explanation. Not sure if displayValue is the right place for this explanation though (limited formatting, not much space) - maybe separate documentation page that we discussed would help here?

Can the failureDescription be overwritten to have failure-specific explanations?

From what I tested neither failureDescription nor helpText can be overwritten ☹️ We only have displayValue and details. Unfortunately, details do not support any formatting when it comes to text:

Is the immutability of failureDescription a bug or a feature? cc @paulirish

kdzwinel · 2018-01-03T01:03:00Z

lighthouse-core/audits/seo/link-text.js

      failureDescription: 'Links do not have descriptive text',
      helpText: 'Descriptive link text helps search engines understand your content. ' +
-      '[Learn more](https://webmasters.googleblog.com/2008/10/importance-of-link-architecture.html)',
+      '[Learn more](https://webmasters.googleblog.com/2008/10/importance-of-link-architecture.html).',


unrelated change, sorry to shove it here, but it was killing me

kdzwinel · 2018-01-03T01:20:09Z

lighthouse-core/audits/seo/canonical.js

@@ -0,0 +1,166 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


@paulirish you always check those 😉 so here is a fun riddle - I created this file in 2017, but it will be merged into master in 2018. Should I put 2017 or 2018 here?

2017 sgtm. :)

rviscomi · 2018-01-03T01:11:35Z

lighthouse-core/audits/seo/canonical.js

+          canonicalURL.pathname === '/' && baseURL.pathname !== '/') {
+          return {
+            rawValue: false,
+            displayValue: 'points to a root of the same origin',


Can the failureDescription be overwritten to have failure-specific explanations?

rviscomi · 2018-01-03T01:11:53Z

lighthouse-core/audits/seo/link-text.js

      failureDescription: 'Links do not have descriptive text',
      helpText: 'Descriptive link text helps search engines understand your content. ' +
-      '[Learn more](https://webmasters.googleblog.com/2008/10/importance-of-link-architecture.html)',
+      '[Learn more](https://webmasters.googleblog.com/2008/10/importance-of-link-architecture.html).',


rviscomi · 2018-01-03T01:12:54Z

lighthouse-core/audits/seo/canonical.js

@@ -0,0 +1,166 @@
+/**
+ * @license Copyright 2017 Google Inc. All Rights Reserved.


🎊

2018?

paulirish · 2018-01-03T17:43:03Z

lighthouse-core/audits/seo/canonical.js

+
+        if (canonicals.length === 0) {
+          return {
+            rawValue: true,


when 4052 lands, we'll want to add notApplicable: true to this case.

I added it right away (it doesn't really matter if this PR will be merge after or before #4052)

kdzwinel · 2018-01-04T23:32:44Z

@patrickhulce we talked with @rviscomi about your idea to split this audit into two (errors with weight 1 and warnings with weight 0). On the one hand, it'd allow us to have separate titles and descriptions for both audits and provide more explanation, but on the other hand, it would create a confusing situation where user gets 100/100 but still sees that one of the audits is failing. So, if you don't have any strong feelings regarding the split we'd like to avoid it.

patrickhulce

this looks a-ok to me! 👌

there's probably a nicer looking way we can handle the displayValue issue in the future but doesn't make sense to block on it 👍

over to @paulirish for any closing thoughts

patrickhulce · 2018-01-09T21:02:42Z

lighthouse-core/audits/seo/canonical.js

+          };
+        }
+
+        // another common mistake is to have canonical pointing from all pages of the website to its root


still somewhat 😢 that this can fail for intentional usage. though I don't have a real life example so it's probably fine :)

patrickhulce · 2018-01-09T21:06:04Z

lighthouse-core/test/audits/seo/canonical.js

+
+  it('fails when canonical points to the root while current URL is not the root', () => {
+    const mainResource = {
+      url: 'https://example.com/articles/cats-and-you',


paulirish · 2018-01-10T18:39:38Z

lighthouse-core/audits/seo/canonical.js

+          baseURL.href !== canonicalURL.href) {
+          return {
+            rawValue: false,
+            displayValue: `points to another hreflang location (${baseURL.href})`,


Rather than overloading displayValue I think we should abuse debugString like we already do for the PWA ones.

We can even extend https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/audits/multi-check-audit.js much like the splash screen one.

This also sorts out the mutable failureDescription concern above.

(yes, debugString is supposed to be exceptions, not details of failing tests. But since the PWA audits are already violating that, we might as well violate it here, too.)

TIL about the multi-check-audit! Looks cool, but it probably makes most sense for audits returning multiple failures (Failures: ${result.failures.join(', ')}.) and this one doesn't, so I'll just switch displayValues to debugStrings.

kdzwinel · 2018-01-10T23:34:17Z

@paulirish thank you for taking a look at this! I addressed your debugString comment:

kdzwinel added 10 commits December 11, 2017 01:50

WIP

6e4f60a

tests WIP

0794a50

More rules, refactor

a07d4ea

Adjust audit code, add more tests

7a04b76

Merge remote-tracking branch 'upstream/master' into seo-canonical

1b9c542

hreflang, cleanup

59f3772

Merge remote-tracking branch 'upstream/master' into seo-canonical

71f1507

Clean up code and tests.

bce63a3

Smoke tests and fix to the static server (support multiple headers wi…

2edb56e

…th the same name).

debugString -> displayValue

2213936

devtools-bot added the waiting4reviewer label Jan 3, 2018

kdzwinel commented Jan 3, 2018

View reviewed changes

rviscomi reviewed Jan 3, 2018

View reviewed changes

paulirish reviewed Jan 3, 2018

View reviewed changes

kdzwinel added 2 commits January 4, 2018 22:06

Merge remote-tracking branch 'upstream/master' into seo-canonical

697d508

Add notApplicable flag when there are no canonicals.

48b623d

patrickhulce approved these changes Jan 9, 2018

View reviewed changes

patrickhulce assigned paulirish Jan 9, 2018

paulirish requested changes Jan 10, 2018

View reviewed changes

kdzwinel added 2 commits January 10, 2018 23:46

Merge remote-tracking branch 'upstream/master' into seo-canonical

94f0f95

Replace displayValue's with debugString's

23dd6ca

paulirish approved these changes Jan 10, 2018

View reviewed changes

paulirish merged commit 2ba8d1f into GoogleChrome:master Jan 10, 2018

paulirish removed the waiting4reviewer label Mar 6, 2018

		@@ -0,0 +1,166 @@
		/**
		* @license Copyright 2017 Google Inc. All Rights Reserved.

Conversation

kdzwinel commented Jan 3, 2018

Uh oh!

kdzwinel Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdzwinel commented Jan 4, 2018

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdzwinel commented Jan 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kdzwinel Jan 3, 2018 •

edited

Loading

paulirish Jan 10, 2018 •

edited

Loading