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.

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

core(without-javascript): remove audit #11711

Merged
merged 22 commits into from
Dec 11, 2020
Merged

core(without-javascript): remove audit #11711

merged 22 commits into from
Dec 11, 2020

Conversation

Beytoven
Copy link
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
Copy link
Member

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
Copy link
Collaborator

connorjclark commented Nov 25, 2020

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
Copy link
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 :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

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
Copy link
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
Copy link
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
Copy link
Member

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

Copy link
Member

@adamraine adamraine left a comment

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:

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

Copy link
Member

@adamraine adamraine left a comment

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove without-javascript audit
7 participants