Skip to content

Add support for --cwd (#542) - #620

Merged
bcoe merged 1 commit into
istanbuljs:masterfrom
Speedy37:master
Sep 5, 2017
Merged

Add support for --cwd (#542)#620
bcoe merged 1 commit into
istanbuljs:masterfrom
Speedy37:master

Conversation

@Speedy37

Copy link
Copy Markdown
Contributor

I added support for --root option
I found out that the cwd option does exactly what the --root is supposed to do except:

  • the NYC_CWD environment has no documentation
  • the implicit lookup for upward package.json is not something I would expect

So I added a --root option that simply force cwd to be what I tell it to be.

@coveralls

coveralls commented Jun 30, 2017

Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 28d8df7 on Speedy37:master into ** on istanbuljs:master**.

@bcoe

bcoe commented Jul 16, 2017

Copy link
Copy Markdown
Member

@Speedy37 rather than adding a new root option, I think I'd rather than we make cwd optionally configurable; basically, just renaming root to cwd.

@Speedy37

Speedy37 commented Jul 21, 2017

Copy link
Copy Markdown
Contributor Author

NYC_CWD isn't the same as nyc working directory.
I think NYC_CWD isn't the right name for this, because it change the root path where it looks for files. The generated coverage file path are still relative to nyc working directory.

edit: I'm wrong, NYC_CWD is like the cwd, it's just I had the next piped command in the wrong working directory.

@Speedy37 Speedy37 changed the title Add support for --root (#542) Add support for --cwd (#542) Jul 21, 2017
@Speedy37

Copy link
Copy Markdown
Contributor Author

I updated this PR to use cwd naming.

But I'm not convinced this is exactly what you want, because cwd doesn't only impact file lookup, it also impact cache location.

@coveralls

coveralls commented Jul 21, 2017

Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 3b5b8a8 on Speedy37:master into ** on istanbuljs:master**.

@bcoe

bcoe commented Jul 22, 2017

Copy link
Copy Markdown
Member

@Speedy37 maybe I'm just not fully understanding the problem you're looking to solve. Do you have a repo that benefits from manually setting the root directory, is the problem not addressed by --cwd?

Just want to make sure we're solving a specific issue you're running into before we land anything.

@bcoe

bcoe commented Aug 1, 2017

Copy link
Copy Markdown
Member

@Speedy37 this did the trick for you, for your use-case? will work on landing this soon, have just been pretty swamped triaging old issues (trying to cleanup the project a bit).

@Speedy37

Speedy37 commented Aug 1, 2017

Copy link
Copy Markdown
Contributor Author

Sry for the delay, I was on vacations.

The initial problem was to be able to run nyc by including files at /mypath/node_modules/mymodule/*.js while beeing able to reject /mypath/node_modules/mymodule/node_modules/**.

I was able to do this only by using the cwd trick because the 'node_modules/' exclusion is deeply included by nyc. ('!node_modules/' doesn't help because it prevents me to reject /mypath/node_modules/mymodule/node_modules/**).

The other problem being NYC_CWD modified by nyc by looking upwards for package.json, making it unreliable (I'm building my npm packages outside the source folder).

So being able to set --cwd would make the whole process much more reliable.

NYC_CWD has two main effect:

  • setting the root directory used by nyc to lookup for files
  • being the base of the temporary directory

In my opinion, two arguments (--root and --tmpdir) would be better than --cwd and their explanations would be more specific.

@bcoe

bcoe commented Aug 22, 2017

Copy link
Copy Markdown
Member

@Speedy37 as long as the default behavior is reasonable, I'd be comfortable with splitting configuration into two parts --root and --tmpdir.

I've been swamped with life/work stuff lately, and getting a bit buried in OSS work. would love to have you join the community slack I recently stood up:

http://devtoolscommunity.herokuapp.com/

it's a great way to kick me in the butt and make sure we get work over the finish line.

@Speedy37

Speedy37 commented Sep 1, 2017

Copy link
Copy Markdown
Contributor Author

After reviewing the whole list of arguments, you already define temp-directory and because you name cwd the base path for file lookup, I think --cwd is the best name (it won't be confusing with code implementation naming).

@bcoe
bcoe merged commit 0fc6d8f into istanbuljs:master Sep 5, 2017
bcoe pushed a commit that referenced this pull request Sep 5, 2017
@bcoe

bcoe commented Sep 5, 2017

Copy link
Copy Markdown
Member

@Speedy37 this is now released in nyc@11.2.0, mind testing it out?

@Speedy37

Speedy37 commented Sep 5, 2017

Copy link
Copy Markdown
Contributor Author

Working like a charm
edit: or not...

@PowerSupply

Copy link
Copy Markdown

This does not seem to work. I can run my tests, but the coverage report is broken.

@bcoe

bcoe commented Oct 3, 2017

Copy link
Copy Markdown
Member

@Speedy37 have you had a chance to dig into this at all, I would like to make sure that if we add this feature it's working as expected.

@PowerSupply are you specifically having trouble with the --cwd feature?

@Speedy37

Speedy37 commented Oct 3, 2017

Copy link
Copy Markdown
Contributor Author

I've reverted my builds to version v11.1.0 due to #668, but from my tests the --cwd flag is working fine and with it I didn't had to clean old coverage reports (there is a check that disable cleaning .nyc_output if NYC_CWD is defined).

@PowerSupply

Copy link
Copy Markdown

@bcoe yes and no.

My problem is that I want to use the --all flag to include "files that have not been required in your tests" (as per the documentation). But if I set the --all flag then all files in my project gets included although I have specified include to only include files in a certain folder.

Running the command from a subfolder does not seem to stifle the reach of --all, it still includes all files from my project root (where node_modules is).

The previous solution for this was to use the --root flag but it is no longer an option.

Therefore I tried to use --cwd (that is still undocumented in the readme) instead but it does not work. The combination of --cwd and --all does not work like --root and --all used to.

Right now I can see no other solution to cover all files in my "source" folder than to use the --all flag and then having to manually exclude all folders/files in my project that are not under the sources directory.
This is a degradation from previous functionality.

@Speedy37

Speedy37 commented Oct 3, 2017

Copy link
Copy Markdown
Contributor Author

@PowerSupply without --cwd nyc will select cwd as the first folder in the path that is node_modules, if none then cwd is the current working directory.

--cwd and --all should work like --root and --all used to (I got it working without troubles)

What is the current behavior when you use --cwd ? If you have no instrumented file at all, see #668

@PowerSupply

Copy link
Copy Markdown

Ok, so my problem was somehow related to using babel-plugin-istanbul in a way that I dont understand. I do not have time to look into the error right now but may investigate the problem later.

Thanks for your support!

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.

4 participants