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

Use same default logic as webpack for identifying the source root #61

Merged
merged 3 commits into from
Feb 17, 2019
Merged

Use same default logic as webpack for identifying the source root #61

merged 3 commits into from
Feb 17, 2019

Conversation

peitschie
Copy link
Collaborator

Based on https://webpack.js.org/configuration/entry-context/#context, if the context is not specified, the current directory that the process is running from should be assumed instead.

Align util to mimic this behaviour to ensure paths are correctly de-absoluted during processing.

This addresses an issue reported in aurelia/cli#996 (comment)

@peitschie
Copy link
Collaborator Author

Added another commit to handle slashes being mixed on Windows. This was resulting in a report html with 2 listed entries, neither of which have coverage!

@peitschie peitschie mentioned this pull request Dec 20, 2018
Closed
djedje72
djedje72 reviewed Dec 20, 2018
View reviewed changes
src/reporter.js
coverageMap.files().forEach(path => {
if (!(path in remappedCoverageMap)) {
if (!(util.fixPathSeparators(path) in fixedFilePaths)) {
Copy link

Choose a reason for hiding this comment

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

You have to module.exports fixPathSeparators in util.js file i think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh... I missed checking that file in... oops!

@codecov-io
Copy link

codecov-io commented Dec 21, 2018 •
edited
Loading

Codecov Report

Merging #61 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #61      +/-   ##
=========================================
+ Coverage   97.61%   97.7%   +0.09%     
=========================================
  Files           2       2              
  Lines         126     131       +5     
=========================================
+ Hits          123     128       +5     
  Misses          3       3
Impacted Files Coverage Δ
src/reporter.js 97.77% <100%> (+0.07%) ⬆️
src/util.js 97.56% <100%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50f441f...66fd2ef. Read the comment docs.

@peitschie
fix: correctly strip source file prefixes when no webpack.context is …
3c48bf8 
…defined

This now uses the same default logic as webpack for identifying the source root.
Based on https://webpack.js.org/configuration/entry-context/#context, if the context is not specified, the current
directory that the process is running from should be assumed instead.

Align util to mimic this behaviour to ensure paths are correctly de-absoluted during processing.
@peitschie
fix: don't double-report files with mixed slashes in their names on w…
38087c2 
…indows

For whatever reason, the remapped coverage re-creates paths for source files with mixed slashes. This means that the reporter adds duplicate entries that differ only by the joining path separator, confusing the report writer, which fails to report the coverage on either entry.
@peitschie
Copy link
Collaborator Author

Unfortunately, I've been unable to get reproduction steps nailed down exactly for the second commit (mixed slashes coming out of coverage report).

On the Aurelia TypeScript skeleton with Karma, I get a coverage-summary.json that looks like the following:

{"total": {"lines":{"total":3,"covered":3,"skipped":0,"pct":100},"statements":{"total":3,"covered":3,"skipped":0,"pct":100},"functions":{"total":1,"covered":1,"skipped":0,"pct":100},"branches":{"total":0,"covered":0,"skipped":0,"pct":100}}
,"C:\\Work\\Source\\aurelia-ts\\src/app.ts": {"lines":{"total":3,"covered":3,"skipped":0,"pct":100},"functions":{"total":1,"covered":1,"skipped":0,"pct":100},"statements":{"total":3,"covered":3,"skipped":0,"pct":100},"branches":{"total":0,"covered":0,"skipped":0,"pct":100}}
,"C:\\Work\\Source\\aurelia-ts\\src\\app.ts": {"lines":{"total":0,"covered":0,"skipped":0,"pct":0},"functions":{"total":0,"covered":0,"skipped":0,"pct":0},"statements":{"total":0,"covered":0,"skipped":0,"pct":0},"branches":{"total":0,"covered":0,"skipped":0,"pct":0}}
}

Even aligning all the common node modules to the same versions etc hasn't allowed me to reproduce this issue standalone within this project. I'll keep poking... suffice to say that the second patch correctly handles the above issue.

@mattlewis92 mattlewis92 merged commit ec01a4c into mattlewis92:master Feb 17, 2019
@mattlewis92
Copy link
Owner

mattlewis92 commented Feb 17, 2019 •
edited
Loading

Apologies on the late reply on this one, I completely missed the PR 😢This looks great to me, merging and cutting a new release now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djedje72 djedje72 djedje72 left review comments

@mattlewis92 mattlewis92 mattlewis92 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.
4 participants
@peitschie @codecov-io @mattlewis92 @djedje72