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

tests(smoke): remove html imports from dbw_tester #12354

Merged
merged 2 commits into from
Apr 12, 2021
Merged

Conversation

brendankenny
Copy link
Member

unblocks CI along with #12353

html imports have been deprecated for a loooong time (Chrome 70), but looks like they were only finally removed in Chrome 92.

dbw_tester has been holding on to them as render blocking resources, so it's time to remove since they no longer trigger a network request. Other render-blocking stuff is pretty well covered and I didn't see anything new to add in their place, so this is just a simple removal.

@brendankenny brendankenny requested a review from a team as a code owner April 12, 2021 19:35
@brendankenny brendankenny requested review from adamraine and removed request for a team April 12, 2021 19:35
@google-cla google-cla bot added the cla: yes label Apr 12, 2021
Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

lgtm

(unrelated page crash in CI, I restarted the tests).

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.

Still a couple references to dbw_partial_*.html in the devtools logs and traces of lighthouse-core/test/results/artifacts but I don't think it's worth messing with those files.

We also might be in a deadlock with #12353. @paulirish can we force merge?

@brendankenny
Copy link
Member Author

Still a couple references to dbw_partial_*.html in the devtools logs and traces of lighthouse-core/test/results/artifacts but I don't think it's worth messing with those files.

yeah, I didn't want to mess with those just because we'd have a lot more audit changes that weren't relevant for this change (just removing two network requests), but we will have to pull the trigger on those at some point, I suppose.

@brendankenny
Copy link
Member Author

rebased on top of #12353

@brendankenny brendankenny merged commit 122072b into master Apr 12, 2021
@brendankenny brendankenny deleted the htmlimports branch April 12, 2021 20:53
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.

4 participants