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(coverage): minimize impact of timeout due to istanbul's instrumentation #4396

Merged
merged 3 commits into from
Jan 31, 2018

Conversation

paulirish
Copy link
Member

Issue #4395 has the backstory. This PR has a few things going on:

As a result of this PR, yarn coverage should be 36s faster, and no timeout error in the travis log. However the code coverage stats for the CLI are still incorrect and the roundtrip run is still not truly successful.

This leads to a followup question: Should we have other success criteria for the run-test.js roundtrip test, like calculating TTI, etc?

Ref #4395 but doesn't close it.

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.

seems fine to me!

really annoying that the only way to eliminate instrumentation is separate files, then again it might be nice to collect all our client-side run functions in a single place so it becomes clear what gets run where :)

@@ -25,7 +25,8 @@ describe('CLI run', function() {
it('runLighthouse completes a LH round trip', () => {
const url = 'chrome://version';
const filename = path.join(process.cwd(), 'run.ts.results.json');
const flags = getFlags(`--output=json --output-path=${filename} ${url}`);
const timeoutFlag = `--max-wait-for-load=${9000}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious, why put 9000 in template? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

for the funsies. 🕺

@paulirish
Copy link
Member Author

really annoying that the only way to eliminate instrumentation is separate files, then again it might be nice to collect all our client-side run functions in a single place so it becomes clear what gets run where :)

Yup totally agree.

@paulirish paulirish changed the title test(coverage): minimize impact of timeout due to istanbul's instrumentation tests(coverage): minimize impact of timeout due to istanbul's instrumentation Jan 31, 2018
@paulirish paulirish merged commit d6b027e into master Jan 31, 2018
@paulirish paulirish deleted the istanbultimeout branch January 31, 2018 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants