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

Serve only files when running lhci collect #445

Merged
merged 3 commits into from Sep 17, 2020
Merged

Serve only files when running lhci collect #445

merged 3 commits into from Sep 17, 2020

Conversation

ojizero
Copy link
Contributor

@ojizero ojizero commented Sep 16, 2020

Fixes the behaviour where the FallbackServer attempts to serve folders that
happen to end with .html causing errors. Filters scanned directory
for only files in it before executing any of the logic there.

Fixes: #444

@ojizero ojizero mentioned this pull request Sep 16, 2020
Closed
patrickhulce
patrickhulce reviewed Sep 16, 2020
View reviewed changes
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.

awesome thank you @ojizero! would you be able to add this to one of our autorun test cases?

I'm thinking it's fine if you just add a file folder.html/invisible.html to https://github.com/GoogleChrome/lighthouse-ci/tree/master/packages/cli/test/fixtures/autorun-static-dir/build and make sure we don't pick it up in the existing test, no need to write a new one

@ojizero
Copy link
Contributor Author

ojizero commented Sep 16, 2020

@patrickhulce hmmmm 🤔 just thinking out loud here but isn't the condition if (folder.includes('.')) continue; kind of an overkill? maybe the URL does have . in it and has other things living under it? aren't those valid pages to check on?

shouldn't the check be only for hidden files? i.e. something.other/somepage.html should be a valid page that gets audited? maybe this can be treated in another PR but I'm just thinking out loud here since I think the suggested folder.html/invisible.html should in fact be visible given that the depth being checked is 2 🤔

@patrickhulce
Copy link
Collaborator

isn't the condition if (folder.includes('.')) continue; kind of an overkill? maybe the URL does have . in it and has other things living under it? aren't those valid pages to check on?

Maybe, but I don't think we should change it here for two reasons.

  1. This is just for automatic defaults, it's not restrictive of what you can do. A folder with a dot is at the very least unusual and I'm OK with those URLs needing explicit opt-in.
  2. Any change that automatically starts including more files here would need to be in a major breaking version per version policy.

@ojizero
Copy link
Contributor Author

ojizero commented Sep 17, 2020

very true yea changing this would be a breaking change and may introduce unexpected behaviours ! I'll add the new fixture for the test as it is now 👍

@ojizero
fix(cli): serve only files when running lhci collect
284e3e6 
Fixes the behaviour where the FallbackServer attempts to serve folders that
happen to end with `.html` causing errors. Filters scanned directory
for only files in it before executing any of the logic there.

Fixes: GoogleChrome#444
@ojizero
tests(cli): added fixture for folders ending with .html suffix
87fef85 
Existing autorun tests should cover it normally and should remain unchanged
patrickhulce
patrickhulce approved these changes Sep 17, 2020
View reviewed changes
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.

implementation looks great to me, thanks @ojizero! 🎉

packages/cli/test/fixtures/autorun-static-dir/build/folder.html/invesible.html Outdated
@@ -0,0 +1,10 @@
<!DOCTYPE html>
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you rename this file invisible.html? :) just spelling typo

Copy link
Contributor Author

@ojizero ojizero Sep 17, 2020 •
edited

Choose a reason for hiding this comment

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

if I got a dollar for every typo I make 💃 fixed

@patrickhulce patrickhulce merged commit 3d22230 into GoogleChrome:master Sep 17, 2020
@ojizero ojizero deleted the serve-only-files branch September 17, 2020 16:09
denkrasnov pushed a commit to Travix-International/lighthouse-ci that referenced this pull request Oct 29, 2020
@ojizero @denkrasnov
fix(cli): don't use folders for default HTML files in collect (Goog…
1c86ad9 
…leChrome#445)
This was referenced Dec 19, 2020
Open
Open
This was referenced Jan 28, 2021
Open
Open
@snyk-bot snyk-bot mentioned this pull request Oct 11, 2021
Open
@snyk-bot snyk-bot mentioned this pull request Mar 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.

lhci incorrectly tries to serve (and audit) folder with .html extension
2 participants
@ojizero @patrickhulce