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 support for --cwd (#542) #620

Merged
merged 1 commit into from
Sep 5, 2017
Merged

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

merged 1 commit into from
Sep 5, 2017

Conversation

Speedy37
Copy link
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
Copy link

coveralls commented Jun 30, 2017

Coverage Status

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

@bcoe
Copy link
Member

bcoe commented Jul 16, 2017

@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
Copy link
Contributor Author

Speedy37 commented Jul 21, 2017

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
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
Copy link

coveralls commented Jul 21, 2017

Coverage Status

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

@bcoe
Copy link
Member

bcoe commented Jul 22, 2017

@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
Copy link
Member

bcoe commented Aug 1, 2017

@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
Copy link
Contributor Author

Speedy37 commented Aug 1, 2017

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
Copy link
Member

bcoe commented Aug 22, 2017

@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
Copy link
Contributor Author

Speedy37 commented Sep 1, 2017

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
Copy link
Member

bcoe commented Sep 5, 2017

@Speedy37 this is now released in [email protected], mind testing it out?

@Speedy37
Copy link
Contributor Author

Speedy37 commented Sep 5, 2017

Working like a charm
edit: or not...

@PowerSupply
Copy link

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

@bcoe
Copy link
Member

bcoe commented Oct 3, 2017

@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
Copy link
Contributor Author

Speedy37 commented Oct 3, 2017

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

@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
Copy link
Contributor Author

Speedy37 commented Oct 3, 2017

@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

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