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(coverage): ignore files from npm registry #18457

Merged
merged 18 commits into from
Mar 30, 2023
Merged

fix(coverage): ignore files from npm registry #18457

merged 18 commits into from
Mar 30, 2023

Conversation

GJZwiers
Copy link
Contributor

@GJZwiers GJZwiers commented Mar 27, 2023

Fixes #17664 and part of #18454 by excluding files belonging to npm modules by default in the coverage output.

@loynoir
Copy link

loynoir commented Mar 27, 2023

@GJZwiers

https://deno.com/blog/v1.29#custom-registry-support-via-environment-variable

registry.npmjs.org seems hardcoded.

https://github.com/denoland/deno/search?q=NPM_CONFIG_REGISTRY

I guess something like

e.url.startsWith(
  url.pathToFileURL(
    path.join(deno_cache_dir(), 'npm', partial_parsed_NPM_CONFIG_REGISTRY()) + path.sep
  )
)

@loynoir
Copy link

loynoir commented Mar 27, 2023

@GJZwiers

I still think should do some quoting.

I guess .base_url().domain() is something like js parsedURL.host or parsedURL.hostname.

Not my case, but what about below cases?

> NPM_CONFIG_REGISTRY=https://mirror deno ...
> NPM_CONFIG_REGISTRY=https://mirror:1234/sub/path deno ...
> NPM_CONFIG_REGISTRY=https://mirror:1234/sub/path/ deno ...

If user code path includes mirror, current PR implement will be too wide.

That's why I think need to be more specific, because npm registry is mutable, or said not-hardcoded.

e.url.startsWith(
  url.pathToFileURL(
    path.join(deno_cache_dir(), 'npm', partial_parsed_NPM_CONFIG_REGISTRY()) + path.sep
  )
)

@GJZwiers
Copy link
Contributor Author

Thanks for the good points @loynoir! I have updated the npm folder path to be more specific.

@GJZwiers GJZwiers marked this pull request as ready for review March 28, 2023 13:06
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

See comment. There is an easier way we can do this.

let script_coverages = collect_coverages(coverage_flags.files)?;
let script_coverages = filter_coverages(
npm_folder_filepath.as_str(),
Copy link
Member

Choose a reason for hiding this comment

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

Instead of all the above, the directory of where the npm files are found can be found on ps.npm_resolver.fs_resolver.root_dir_url(). We need to expose it on npm_resolver as something like ps.npm_resolver.root_dir_url(). Then we can provide the url to this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @dsherret, that is a lot cleaner and fixes a failing test I was getting.

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for this!

@dsherret dsherret enabled auto-merge (squash) March 30, 2023 17:08
@dsherret dsherret merged commit 206c593 into denoland:main Mar 30, 2023
mmastrac pushed a commit that referenced this pull request Mar 31, 2023
Fixes #17664 and part of
#18454 by excluding files
belonging to npm modules by default in the coverage output.
@GJZwiers GJZwiers deleted the coverage_ignore_npm_files branch April 3, 2023 17:54
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.

npm modules are included in coverage report by default
4 participants