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

test(smokehouse): add basic smoke test for SEO audits #3267

Merged
merged 6 commits into from
Sep 12, 2017
Merged

test(smokehouse): add basic smoke test for SEO audits #3267

merged 6 commits into from
Sep 12, 2017

Conversation

brendankenny
Copy link
Member

@kdzwinel FYI

this adds a starting point for SEO audit integration tests. Right now this tests only the two a11y audits that are also being used as SEO audits, but since they're a11y audits they don't have many details to test beyond the score

as new SEO audits get added:

  • add audits to the expectations file (obviously :)
  • new audits may have a lot more detail in extendedInfo/details that can be tested (example)
  • could add a second html tester file that fails some/all of these audits and asserts proper failure output in those cases

@brendankenny
Copy link
Member Author

@kdzwinel FYI

and @rviscomi et al :)

This also made me realize that our full-config will no longer be a config running all possible tests since the SEO config branches out separately, so we should probably figure that out at some point

@patrickhulce
Copy link
Collaborator

patrickhulce commented Sep 6, 2017 •
edited
Loading

I believe @rviscomi's plan was to nuke the SEO config and merge them into the full config when they were ready. Though given no one really runs the full config today, it might be fine to add them directly :)

patrickhulce
patrickhulce reviewed Sep 7, 2017
View reviewed changes
package.json
@@ -27,7 +27,7 @@
"build-viewer": "cd ./lighthouse-viewer && yarn build",
"clean": "rimraf *.report.html *.report.dom.html *.report.json *.screenshots.html *.devtoolslog.json *.trace.json || true",
"lint": "[ \"$CI\" = true ] && eslint --quiet -f codeframe . || eslint .",
"smoke": "bash lighthouse-cli/test/smokehouse/offline-local/run-tests.sh && bash lighthouse-cli/test/smokehouse/perf/run-tests.sh && bash lighthouse-cli/test/smokehouse/dobetterweb/run-tests.sh && bash lighthouse-cli/test/smokehouse/byte-efficiency/run-tests.sh && bash lighthouse-cli/test/smokehouse/tricky-ttci/run-tests.sh",
"smoke": "bash lighthouse-cli/test/smokehouse/offline-local/run-tests.sh && bash lighthouse-cli/test/smokehouse/perf/run-tests.sh && bash lighthouse-cli/test/smokehouse/dobetterweb/run-tests.sh && bash lighthouse-cli/test/smokehouse/byte-efficiency/run-tests.sh && bash lighthouse-cli/test/smokehouse/tricky-ttci/run-tests.sh && bash lighthouse-cli/test/smokehouse/seo/run-tests.sh",
Copy link
Collaborator

Choose a reason for hiding this comment

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

we're gonna need a bash script to run our bash scripts 😆

Copy link
Member

Choose a reason for hiding this comment

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

I looked at shortening this to...

for file in lighthouse-cli/test/smokehouse/*/run-tests.sh; do bash $file; done

however it doesnt' handle the exit code correctly, so... we'll just need to make run-test.js not terrible sometime. perhaps a job for lighthouse-assert..

patrickhulce
patrickhulce reviewed Sep 12, 2017
View reviewed changes
lighthouse-cli/test/fixtures/seo/seo-tester.html Outdated
@@ -0,0 +1,28 @@
<!doctype html>
<!--
* Copyright 2017 Google Inc. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

small license?

Copy link
Member Author

Choose a reason for hiding this comment

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

small license?

our HTML files don't appear to have a version of the small license and I didn't care enough to make one. Good first bug! :)

Copy link
Member

Choose a reason for hiding this comment

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

there u go: https://github.com/GoogleChrome/lighthouse/pull/3309/files

Copy link
Member Author

Choose a reason for hiding this comment

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

😶

lighthouse-cli/test/smokehouse/seo/expectations.js Outdated
url: 'http://localhost:10200/seo/seo-tester.html',
audits: {
'meta-viewport': {
score: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want the smoke test to be catching failures or passes?

Copy link
Member Author

Choose a reason for hiding this comment

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

do we want the smoke test to be catching failures or passes?

yeah, I was just dropping this in to get things going, but I can add in a failure page too

@brendankenny brendankenny changed the title test(smokehouse): add barebones smoke test for SEO audits test(smokehouse): add basic smoke test for SEO audits Sep 12, 2017
@brendankenny
Copy link
Member Author

added

  • test for just landed meta-description audit
  • a second page to check SEO audit failures

In the seo.js config, switched the meta-viewport audit (an a11y audit) with viewport audit, which checks for valid viewport content @rviscomi @kdzwinel

patrickhulce
patrickhulce approved these changes Sep 12, 2017
View reviewed changes
Copy link
Collaborator

@patrickhulce patrickhulce left a comment •
edited
Loading

Choose a reason for hiding this comment

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

nice! 🌮 lgtm % small headers copied from paul :)

@kdzwinel kdzwinel mentioned this pull request Sep 12, 2017
Merged
@brendankenny
Copy link
Member Author

failing tests are just the JVM issue facing all builds right now...

@brendankenny brendankenny merged commit 850dbb3 into master Sep 12, 2017
@brendankenny brendankenny deleted the seo-smoke branch September 12, 2017 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulirish paulirish paulirish left review comments

@patrickhulce patrickhulce patrickhulce approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brendankenny @patrickhulce @paulirish