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: mock saveLhr and assert no unit test source changes #11519

Merged
merged 5 commits into from
Oct 6, 2020

Conversation

patrickhulce
Copy link
Collaborator

Summary
We wanted to mock the saveLhr function, not just spy it. Also adds the git diff test to our github unit test suite :)

Related Issues/PRs
fixes https://github.com/GoogleChrome/lighthouse/pull/11509/files#r499844389 and our travis builds

@googlebot
Copy link

☹️ Sorry, but only Googlers may change the label cla: yes.

@@ -111,6 +111,9 @@ jobs:
- run: yarn diff:sample-json
if: matrix.os == 'windows-latest'

# Fail if any changes were written to source files (ex, from -GA).
- run: git diff --exit-code
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wanna add to the smokes step too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I'll add everywhere I just noticed it happens on update:sample-json too

@patrickhulce
Copy link
Collaborator Author

the tests should now fail before I fix the update:sample-json path, making sure we're catching this now :)

@connorjclark
Copy link
Collaborator

LGTM

@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Oct 6, 2020

soooooo @connorjclark after looking at all the places affected by the writing lhr.json change I'm not sure I like it as a thing.

This PR changes the extension to .report.json for now, but why did we need the LHR -A writing? The original PR doesn't have a description and -A creates a JSON already when you just add --output=json so I'm not super clear why we need it also written to artifacts. Writing audit output to a folder that's supposed to be the artifacts doesn't feel like the optimal solution (and rears its head here in several places according to the git clean failures in numerous tests that are difficult to mock), so maybe there is another way we can work to solve whatever the root cause is that inspired #11509?

@connorjclark
Copy link
Collaborator

soooooo @connorjclark after looking at all the places affected by the writing lhr.json change I'm not sure I like it as a thing.

I thought the mock you added would prevent this?

@patrickhulce
Copy link
Collaborator Author

I thought the mock you added would prevent this?

It prevents it in those specific tests but apparently we exercise audit mode in several other places that weren't touched in #11509.

Do you have any thoughts on the rest of my comment?

@connorjclark
Copy link
Collaborator

connorjclark commented Oct 6, 2020

What if we save in the "artifact folder" if: -A is set and --output includes json? That should avoid the test issues.

Currently it's too cumbersome to hold on to a LHR for a run, compared to how easy it is for artifacts. especially combined with your new script to save the latest-run folder.

Would also be ok with a new folder in .tmp: historic LHRs. maybe even just always do it for every run if LH_DEBUG or DEBUG is true or w/e....

@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Oct 6, 2020

What if we save in the "artifact folder" if: -A is set and --output includes json? That should avoid the test issues.

For future readers, @connorjclark and I discussed offline and settled on this solution with the addition of -G being used.

@brendankenny
Copy link
Member

For future readers, @connorjclark and I discussed offline and settled on this solution with the addition of -G being used.

amusing to look back at the original -GAR proposal thread

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.

None yet

5 participants