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

Add --projectRoot option #415

Closed
wants to merge 2 commits into from
Closed

Add --projectRoot option #415

wants to merge 2 commits into from

Conversation

rasimx
Copy link

@rasimx rasimx commented Aug 19, 2022

Checklist
  • npm test, tests passing
  • npm run test:snap (to update the snapshot)

Copy link
Contributor

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

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

Should this be passed to test-exclude as cwd as well?

c8/lib/report.js

Lines 39 to 45 in 7f1069d

this.exclude = new Exclude({
exclude: exclude,
include: include,
extension: extension,
relativePath: !allowExternal,
excludeNodeModules: excludeNodeModules
})

@rasimx
Copy link
Author

rasimx commented Aug 31, 2022

Should this be passed to test-exclude as cwd as well?

c8/lib/report.js

Lines 39 to 45 in 7f1069d

this.exclude = new Exclude({
exclude: exclude,
include: include,
extension: extension,
relativePath: !allowExternal,
excludeNodeModules: excludeNodeModules
})

I do not think so. This option affects only the paths in the lcov file.

@rasimx rasimx requested a review from AriPerkkio August 31, 2022 06:57
.options('project-root', {
default: undefined,
type: 'string',
describe: 'specifies which points point to the root of the mono repository'
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this option overlaps with the undocumented resolve option. Have you tried using it as the projectRoot in istanbul-reporter options?

projectRoot: this.resolve || process.cwd(),

With the current changes it feels like you'd need to keep projectRoot and resolve in sync manually.

c8/lib/parse-args.js

Lines 136 to 139 in 7f1069d

.option('resolve', {
default: '',
describe: 'resolve paths to alternate base directory'
})

const path = resolve(this.resolve, v8ScriptCov.url)

Copy link
Author

Choose a reason for hiding this comment

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

To be honest, I don't understand why the 'resolve' option is needed. Whatever I specify there, it does not affect the final files.

Copy link
Author

Choose a reason for hiding this comment

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

I tried using this, but it didn't help me.

Copy link
Author

Choose a reason for hiding this comment

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

projectRoot only affects paths in lcov files. We are currently using a fork in a working project and it works fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did some testing with the this.resolve option now, and it indeed seems to have no effect at all. The v8ScriptCov.url paths are absolute, not relative. No idea what that API is supposed to do.

@AriPerkkio
Copy link
Contributor

I do not think so. This option affects only the paths in the lcov file.

I still think this same option should be used in test-exclude as cwd. This is what nyc does, and this project seems to mimic its API quite well. Let's wait for project maintainers to make the final call though.

nyc has cwd option, which is used as projectRoot for istanbul-lib-report and as cwd for test-exclude:

You can change the project root directory with the --cwd option.

This feature is also something that is needed in vitest in order to change project root: vitest-dev/vitest#1902.

@bcoe
Copy link
Owner

bcoe commented Nov 3, 2022

Hello @rasimx could you provide a description in your PR of the problem you're trying to solve?

I believe the resolve functionality rewrites the output from v8 to resolve under an alternate path, allowing you to remove leading directories, etc. Whereas passing projectRoot to the Reporter is only used by a couple reporters (clover and cobertura, see search).

@@ -74,7 +76,8 @@ class Report {
reports.create(_reporter, {
skipEmpty: false,
skipFull: this.skipFull,
maxCols: process.stdout.columns || 100
maxCols: process.stdout.columns || 100,
projectRoot: this.projectRoot
Copy link
Contributor

Choose a reason for hiding this comment

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

@rasimx looks like an alternative way to pass this option was implemented in #423.

It's included in c8@7.13.0.

@bcoe
Copy link
Owner

bcoe commented Feb 14, 2023

@rasimx looks like an alternative way to pass this option was implemented in #423.

I would prefer that we expose this reporterOptions configuration so that it can be configured in package configuration, or with --reporter-options.projectRoot -- reasoning being that the options available for the reporter sometimes have names that are a bit confusing because they sound like top level configuration for c8 itself (this is why I was initially confused by this PR).

@rasimx closing this for now, since I did't hear back from you with my last update. But, I'd be happy to land this work in the future if you're willing to update based on this suggestion.

@bcoe bcoe closed this Feb 14, 2023
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.

None yet

3 participants