Skip to content

core(without-javascript): remove audit - #11711

Merged
devtools-bot merged 22 commits into
masterfrom
del-without-js
Dec 11, 2020
Merged

core(without-javascript): remove audit#11711
devtools-bot merged 22 commits into
masterfrom
del-without-js

Conversation

@Beytoven

@Beytoven Beytoven commented Nov 24, 2020

Copy link
Copy Markdown
Contributor

Fixes #11459.

@Beytoven Beytoven added the 7.0 label Nov 24, 2020
@Beytoven
Beytoven requested a review from a team as a code owner November 24, 2020 20:56
@Beytoven
Beytoven requested review from adamraine and removed request for a team November 24, 2020 20:56
@google-cla google-cla Bot added the cla: yes label Nov 24, 2020
@adamraine

adamraine commented Nov 25, 2020

Copy link
Copy Markdown
Contributor

In addition to the CI failures, I found some files which still reference this audit:

  • lighthouse-cli/test/smokehouse/test-definitions/offline-local/offline-config.js Related to smoke failures?
  • lighthouse-treemap/app/debug.json @connorjclark should we update in this PR?

Additionally, the HTMLWithoutJavascript gatherer is only used for this audit, should we remove this too?

@connorjclark

connorjclark commented Nov 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

lighthouse-treemap/app/debug.json @connorjclark should we update in this PR?

Nah we don't have too. I suppose if we wanted to it'd be ok. could just remove every audit in that LHR except for script-treemap-data | large-javascript-libraries | unused-javascript | source-maps | valid-source-maps

@patrickhulce

patrickhulce commented Nov 28, 2020

Copy link
Copy Markdown
Collaborator

New idea for a replacement: we could take a screenshot of the page without javascript and run the speed index algorithm on it, if it's more than ~20% different it fails? If there's interest I can file an issue with more details :)

patrickhulce
patrickhulce reviewed Nov 28, 2020
View reviewed changes

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

one other note on the changes necessary here, we'll probably want to simplify the flow and remove the disableJavaScript handling in driver as well :)

@Beytoven

Beytoven commented Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

New idea for a replacement: we could take a screenshot of the page without javascript and run the speed index algorithm on it, if it's more than ~20% different it fails? If there's interest I can file an issue with more details :)

Just to be clear, you're proposing this as an alternative to removing the 'without-js' audit completely, right?

@patrickhulce

patrickhulce commented Nov 30, 2020

Copy link
Copy Markdown
Collaborator

Just to be clear, you're proposing this as an alternative to removing the 'without-js' audit completely, right?

A long-term alternative yes, but my proposal shouldn't block the landing of this PR. I commented here to keep discussion about why it's OK to remove this in one logical place :)

brendankenny
brendankenny reviewed Dec 1, 2020
View reviewed changes
Comment thread lighthouse-core/gather/driver.js
@brendankenny

brendankenny commented Dec 1, 2020

Copy link
Copy Markdown
Contributor

smoke test failure for now-possible js redirects is an interesting new case :)

adamraine
adamraine reviewed Dec 8, 2020
View reviewed changes

@adamraine adamraine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One last remnant I noticed:

lighthouse/lighthouse-core/test/results/artifacts/artifacts.json

Lines 3342 to 3344 in 80b9db0

"HTMLWithoutJavaScript": {
"bodyText": "Do better web tester page\nHi there!\ntouchmove section\n "
},

Comment thread ...ium-webtests/webtests/http/tests/devtools/lighthouse/lighthouse-export-run-expected.txt.orig Outdated
Comment thread ...webtests/webtests/http/tests/devtools/lighthouse/lighthouse-successful-run-expected.txt.orig Outdated
Comment thread ...mium-webtests/webtests/http/tests/devtools/lighthouse/lighthouse-view-trace-run-expected.txt Outdated
adamraine
adamraine approved these changes Dec 9, 2020
View reviewed changes

@adamraine adamraine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@connorjclark connorjclark changed the title core(without-javascript): remove the without-javascript audit core(without-javascript): remove audit Dec 9, 2020
@devtools-bot
devtools-bot merged commit 2aa9845 into master Dec 11, 2020
@devtools-bot
devtools-bot deleted the del-without-js branch December 11, 2020 00:21
paulirish added a commit that referenced this pull request Dec 11, 2020
@paulirish
Revert "core(without-javascript): remove audit (#11711)"
1f80c94 
This reverts commit 2aa9845.
@paulirish paulirish mentioned this pull request Dec 11, 2020
Closed
@brendankenny brendankenny mentioned this pull request Dec 11, 2020
Merged

This branch was successfully deployed

1 active deployment
Preview — 8d758544 Deployed Dec 10, 2020 by vercel[bot]
View deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers
@brendankenny brendankenny brendankenny left review comments
@patrickhulce patrickhulce patrickhulce left review comments
@adamraine adamraine adamraine approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@adamraine adamraine

Labels

breaking cla: yes waiting4reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

remove without-javascript audit

7 participants

@Beytoven @adamraine @connorjclark @patrickhulce @brendankenny @devtools-bot @paulirish