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

fix(test-runner): fix error when function metadata varies between tests #1436

Merged
merged 3 commits into from May 5, 2021

Conversation

Stuk
Copy link
Contributor

@Stuk Stuk commented May 4, 2021

Fixes #689

See also istanbuljs/v8-to-istanbul#121

What I did

  1. This PR updates addingMissingCoverageItems to also add missing functions. This can occur when a function is defined in another function, and the outer function is not called in every test run. For example methods defined on a class using arrow functions => are only created when the constructor runs. Specifically, I have moved the code inside the loops out into their own functions, so they can be called with the branch coverage data and the function coverage data.

Collect and merge function definitions from all runs in coverage mapping

Fixes modernweb-dev#689

See also istanbuljs/v8-to-istanbul#121
@changeset-bot
Copy link

changeset-bot bot commented May 4, 2021

🦋 Changeset detected

Latest commit: 5411d7e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@web/test-runner-core Patch
@web/test-runner Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Stuk
Copy link
Contributor Author

Stuk commented May 4, 2021

Let me know what changeset I should add. Maybe a minor bump?

@LarsDenBakker
Copy link
Member

@Stuk Thanks a lot! We can do a patch, you can run yarn changeset or add one by clicking the PR comment. I can also do it for you.

@benjamind @Westbrook could you check if this fixes it for you as well?

@Westbrook
Copy link
Member

Looking!

@benjamind
Copy link
Contributor

Confirmed this fixes the reproducible test case found here

@Stuk
Copy link
Contributor Author

Stuk commented May 4, 2021

@LarsDenBakker actually, I don't want to mess anything up, so would you ind adding the changeset?

@Westbrook
Copy link
Member

The results certainly look nice here!

Before:
image

After:
image

Stuk and others added 2 commits May 4, 2021 15:23
Co-authored-by: Westbrook Johnson <westbrook.johnson@gmail.com>
@Stuk
Copy link
Contributor Author

Stuk commented May 4, 2021

Added changeset with some help from Westbrook. Thanks!

Copy link
Member

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

I think this is good to go! It's already a big win for my usage in Spectrum Web Components as outlined in the screenshots above and I know a number of other have bumped into this as well. Thanks for digging into this for us @Stuk 🎉

@LarsDenBakker LarsDenBakker merged commit 6f80be6 into modernweb-dev:master May 5, 2021
@LarsDenBakker
Copy link
Member

Thanks, much appreciated!

@Stuk Stuk deleted the collect-fn-items branch May 5, 2021 16:23
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.

[test-runner] Cannot read property 'decl' of undefined in coverage UI
4 participants