-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Conversation
https://deno.com/blog/v1.29#custom-registry-support-via-environment-variable
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
)
) |
I still think should do some quoting. I guess 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 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
)
) |
Thanks for the good points @loynoir! I have updated the npm folder path to be more specific. |
There was a problem hiding this 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.
cli/tools/coverage/mod.rs
Outdated
let script_coverages = collect_coverages(coverage_flags.files)?; | ||
let script_coverages = filter_coverages( | ||
npm_folder_filepath.as_str(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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!
Fixes #17664 and part of #18454 by excluding files belonging to npm modules by default in the coverage output.