SEO - description audit #3227

kdzwinel · 2017-09-01T20:35:24Z

…tant

paulirish · 2017-09-01T20:38:38Z

lighthouse-core/config/seo.js

@@ -7,6 +7,15 @@

 module.exports = {
  extends: 'lighthouse:default',
+  passes: [{
+    passName: 'seoPass',


patrick says you can just passName: 'defaultPass' and get this gather merged into that one.

so no need for an extra pass

kdzwinel · 2017-09-01T20:38:42Z

lighthouse-core/audits/seo/meta-description.js

+   */
+  static audit(artifacts) {
+    return {
+      rawValue: (artifacts.Description !== null && artifacts.Description.length > 0)


Should we also .trim() it?

Should we include an additional text saying either "Description tag wasn't found" or "Description is empty"?

BTW I've spotted that returning a numeric value here causes test to fail but green tick to be shown next to the test:

Is this a bug?

trim sgtm

debugString for those cases would be nice

yes this seems like a bug :) there's some magic that happens if the rawValue is a number to auto-populate the score but even if that's producing something weird the UI should still be consistent.

Thanks for help! Regarding no.3 - should I create an issue for it?

Inconsistency does look like a bug. In the meantime, should be able to set scoringMode: Audit.SCORING_MODES.NUMERIC in the meta information (otherwise everything other than 100 is interpreted as failing)

@brendankenny thanks! For this audit rawValue is a boolean so we are fine, but I'll keep your advice in mind for the next ones.

If the meta description exists but is invalid, I'd also recommend including the markup in the debugString. For example:

Description text is empty: `<meta name="description" content=" ">`

It'll be helpful for developers to know where to look to fix the issue.

@rviscomi I think that's a good idea, but I wonder if it won't be confusing to show <meta name="description" content=" "> while the actual tag on page may look totally different (e.g. <META YADA="yada" CONTENT=" " name="description" />)?

patrickhulce

nice! very clean 👍

patrickhulce · 2017-09-01T20:38:32Z

lighthouse-core/config/seo.js

@@ -7,6 +7,15 @@

 module.exports = {
  extends: 'lighthouse:default',
+  passes: [{
+    passName: 'seoPass',


if you set passName to 'defaultPass' then the gatherer should just be added to the list to run, if not it's a bug and I can look into it :)

patrickhulce · 2017-09-01T20:40:45Z

lighthouse-core/audits/seo/meta-description.js

+      helpText: 'Meta descriptions may be included in search results to concisely summarize ' +
+          'page content. Read more in the ' +
+          '[Search Console Help page](https://support.google.com/webmasters/answer/35624?hl=en#1).',
+      requiredArtifacts: ['Description']


Description is rather generic, what about MetaDescription or start namespacing with SEODescription if there are going to be many more seo specific ones?

…artifact to MetaArtifact (and update gatherer file name accordingly)

kdzwinel · 2017-09-01T21:18:22Z

@paulirish @patrickhulce thank you for your comments!

defaultPass - it works! However, it looks like my gatherer is now painfully slow for some reason:

  status Retrieving: WebSQL +2ms
  status Retrieving: MetaDescription +500ms

Interestingly, the problem goes away if it's not running last:

  status Retrieving: MetaDescription +3ms
  status Retrieving: WebSQL +3ms

🤔
[EDIT] It seems to me that 'MetaDescription' taking 500ms has something to do with WebSQL having a 500ms timeout -

lighthouse/lighthouse-core/gather/gatherers/dobetterweb/websql.js

Line 10 in 3be4832

const MAX_WAIT_TIMEOUT = 500;

I've renamed Description artefact to MetaDescription and updated gatherer's name accordingly.
I'll appreciate someone helping me with these questions: SEO - description audit #3227 (comment)

patrickhulce · 2017-09-01T23:03:05Z

It seems to me that 'MetaDescription' taking 500ms has something to do with WebSQL having a 500ms timeout

Yeah the debug timestamps are confusing to read, the x ms is reporting to you how many milliseconds have passed since the previous statement, since we print the name of the gatherer before we call it, the time for a gatherer is actually printed on the next line

ali999abas · 2017-09-02T19:43:28Z

Good job

kdzwinel · 2017-09-02T22:51:26Z

x ms is reporting to you how many milliseconds have passed since the previous statement

thanks for explanation, it makes sense (although I'll have to agree that it's not very intuitive)

I've added description trimming and debugStrings. PTAL

rviscomi · 2017-09-05T16:46:33Z

lighthouse-core/audits/seo/meta-description.js

+   */
+  static get meta() {
+    return {
+      category: 'Content Best Practices',


Meta question for the LH team. category here is referring to the title of the audit group, not to be confused with the top-level audit category, SEO, defined in the config file?

https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/config/seo.js#L25-L27

@rviscomi yeah the field here has an ambiguous name and is not used anywhere anymore 😆

I'm in favor of removing entirely now that we have real categories that control quite a bit of report building (just as cleanup work by the LH team in other PRs, it's a mandatory field currently).

patrickhulce

LGTM!

patrickhulce · 2017-09-05T17:26:19Z

lighthouse-core/audits/seo/meta-description.js

+    if (artifacts.MetaDescription === null) {
+      return {
+        rawValue: false,
+        debugString: 'Description meta tag is missing.'


Actually now that you've brought it up in the other issue, I think I might have a slight preference to put these in the displayValue and keep debugString for errors like the gatherer failing (which is handled automatically)

Actually now that you've brought it up in the other issue, I think I might have a slight preference to put these in the displayValue and keep debugString for errors like the gatherer failing (which is handled automatically)

agree about confusion between user vs system errors in debugString, but don't we still want the red string for failure here? (We still use the guidance "Optional error string for helping the user figure out why they failed here." in our docs)

IMO debugString is more readable and, as @brendankenny mentioned, in this case it matches the description from the docs:

lighthouse/lighthouse-core/closure/typedefs/AuditResult.js

Lines 31 to 35 in fadaef9

/**

* Optional error string for helping the user figure out why they failed here.

* @type {(string|undefined)}

*/

AuditResult.prototype.debugString;

@patrickhulce WDYT? Can we keep the debugString here?

Yeah sgtm, I'm sort of off the reservation when it comes to my hopes for debugString anyway :)

I'll put the title and debugString in the order you'd see them in the report:

Document does not have a meta description

Description meta tag is missing

Given that the debugString duplicates so much, i think we should exclude it.

brendankenny

Gatherer unit tests are often inadequate as they tend to just be a test of how accurate the test mock is. We should set up integration tests for the incoming SEO audits so we can assert that the full gather-to-audit process is working.

It could be added to an existing test suite, but setting up a new one that uses the SEO config may be the best first step (unless anyone feels otherwise).

I think it's ok to not handle it in this PR so it stays nicely focused, but we should get one going ASAP

brendankenny · 2017-09-05T18:00:18Z

lighthouse-core/test/audits/seo/meta-description-test.js

+
+describe('SEO: description audit', () => {
+  it('fails when HTML does not contain a description meta tag', () => {
+    return assert.equal(Audit.audit({


it would be good to assert that the audit is also returning a debugString (or a displayValue, depending on how the comment above shakes out :)

will do 👍

brendankenny

just last two nits and I think I'm good

brendankenny · 2017-09-08T15:32:28Z

lighthouse-core/config/seo.js

@@ -28,7 +37,8 @@ module.exports = {
      description: 'These ensure your app is able to be understood by search engine crawlers.',
      audits: [
        {id: 'meta-viewport', weight: 1, group: 'seo-mobile'},
-        {id: 'document-title', weight: 1, group: 'seo-content'}
+        {id: 'document-title', weight: 1, group: 'seo-content'},
+        {id: 'meta-description', weight: 1, group: 'seo-content'}


nit: go for the trailing comma here so future diffs will be cleaner

I saw trailing comma being used here and there, but since there is no 100% consistency, I left it out. The cleaner diffs argument SGTM, maybe we should make eslint to check for that?

AFAIK there wasn't any effort to do trailing commas other than occasionally sneaking them in as we touched code but huge +1 to turning on the eslint rule :)

I've opened an issue for it: #3304 👍

brendankenny · 2017-09-08T15:35:17Z

lighthouse-core/test/audits/seo/meta-description-test.js

+      MetaDescription: null
+    });
+    assert.equal(auditResult.rawValue, false);
+    assert.ok(auditResult.debugString);


it would probably be (slightly) better to check which string is getting returned as the debugString since there's two different cases (could go for an exact string match or a debugString.includes() on 'empty' and 'missing'

+1 to a quick includes check

paulirish · 2017-09-08T21:54:02Z

lighthouse-core/audits/seo/meta-description.js

+    if (artifacts.MetaDescription === null) {
+      return {
+        rawValue: false,
+        debugString: 'Description meta tag is missing.'


I'll put the title and debugString in the order you'd see them in the report:

Document does not have a meta description

Description meta tag is missing

Given that the debugString duplicates so much, i think we should exclude it.

paulirish · 2017-09-08T21:54:47Z

lighthouse-core/audits/seo/meta-description.js

+    if (artifacts.MetaDescription.trim().length === 0) {
+      return {
+        rawValue: false,
+        debugString: 'Description text is empty.'


👍 Seems okay to keep this, as it's an unexpected case that'd otherwise be hard to debug.

kdzwinel · 2017-09-09T23:18:35Z

I removed the 'tag missing' debugString, added the missing trailing comma and, updated tests to check debugString with includes (there is only one debugString now, but IMO it still makes sense to have includes).

PTAL

brendankenny

woohoo! Let's land

kdzwinel added 3 commits September 1, 2017 16:50

SEO description audit - WIP

c9d0c1b

Add tests, fix audit, rename audit file

ce43b79

Use CAPITALIZED_WITH_UNDERSCORES instead of CamelCase for a test cons…

9aa4cc2

…tant

paulirish reviewed Sep 1, 2017

View reviewed changes

kdzwinel commented Sep 1, 2017

View reviewed changes

patrickhulce suggested changes Sep 1, 2017

View reviewed changes

Extend defaultPass instead of creating a new one. Rename Description …

dafe4ae

…artifact to MetaArtifact (and update gatherer file name accordingly)

kdzwinel mentioned this pull request Sep 2, 2017

[SEO Audits] Document has a meta description #3175

Closed

Trim the description before checking length. Add debugString's.

969143e

rviscomi reviewed Sep 5, 2017

View reviewed changes

patrickhulce approved these changes Sep 5, 2017

View reviewed changes

patrickhulce reviewed Sep 5, 2017

View reviewed changes

brendankenny reviewed Sep 5, 2017

View reviewed changes

brendankenny mentioned this pull request Sep 5, 2017

docs: import audit glossary from SEO doc #3207

Merged

paulirish mentioned this pull request Sep 5, 2017

Remove audit.meta.category #3252

Closed

Test if failing audits return a debugString.

e2302f0

brendankenny reviewed Sep 8, 2017

View reviewed changes

paulirish reviewed Sep 8, 2017

View reviewed changes

No debugString when tag is missing, better tests, trailing comma.

255196f

kdzwinel mentioned this pull request Sep 12, 2017

Trailing comma - enable ESLint rule #3304

Closed

brendankenny approved these changes Sep 12, 2017

View reviewed changes

brendankenny merged commit c6ed5b5 into GoogleChrome:master Sep 12, 2017

paulirish added the new_audit label Sep 27, 2017

brendankenny mentioned this pull request May 15, 2018

Audit doctype #5213

Closed

	/**
	* Optional error string for helping the user figure out why they failed here.
	* @type {(string\|undefined)}
	*/
	AuditResult.prototype.debugString;

SEO - description audit #3227

SEO - description audit #3227

Conversation

kdzwinel commented Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

kdzwinel Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdzwinel commented Sep 1, 2017 • edited Loading

patrickhulce commented Sep 1, 2017

ali999abas commented Sep 2, 2017

kdzwinel commented Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

patrickhulce Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdzwinel Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Document does not have a meta description

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Document does not have a meta description

Choose a reason for hiding this comment

kdzwinel commented Sep 9, 2017

brendankenny left a comment

Choose a reason for hiding this comment

kdzwinel commented Sep 1, 2017 •

edited

Loading

kdzwinel Sep 1, 2017 •

edited

Loading

kdzwinel commented Sep 1, 2017 •

edited

Loading

kdzwinel commented Sep 2, 2017 •

edited

Loading

patrickhulce Sep 5, 2017 •

edited

Loading

kdzwinel Sep 8, 2017 •

edited

Loading