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

feat: gather process tree information #364

Merged
merged 3 commits into from
Sep 2, 2016
Merged

Conversation

addaleax
Copy link
Member

Add a --show-process-tree that shows a pretty tree of all spawned processes after nyc has run.

The data files for that are stored in processinfo are stored in (temp directory)/processinfo so that they don’t interfere with the fixed format of the coverage files.

Fixes: #158

Right now this only displays the commands as they are being run, ex:

$ nyc --show-process-tree npm run pretest

> [email protected] pretest /home/sqrt/src/nyc
> standard

----------|----------|----------|----------|----------|----------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------|----------|----------|----------|----------|----------------|
All files |  Unknown |  Unknown |  Unknown |  Unknown |                |
----------|----------|----------|----------|----------|----------------|
nyc
└─┬ /usr/local/bin/node /usr/local/bin/npm run pretest
  └── /usr/local/bin/node /home/xxxx/src/nyc/node_modules/.bin/standard

Note: Unlike described in the above issue, I’ve opted to leave environment variables out of the stored data because they may contain sensitive information and there have been cases of .nyc_output accidentally being published.

Any thoughts?

@bcoe
Copy link
Member

bcoe commented Aug 24, 2016

@addaleax I like this as a start to addressing #158! It looks like the tests are failing because Travis is outputting the hierarchy in a different ordering than your local machine, it outputs branch 4 and then branch 3 -- adding a sorting step might be a good fix for this?

A couple thoughts:

  • I think It would be worth pulling the process-tree logic into its own module lib/process.js?, since the
    logic isn't too coupled with the rest of the logic in index.js.
    • this might mean tweaking our coverage logic a bit, to instrument an additional file.
  • I think one thing @isaacs was picturing, was, could we get the coverage percentage for a given branch?
    • this could be a cool eventual addition to functionality, and is another argument for starting to isolate the branching logic into its own module.

@addaleax
Copy link
Member Author

adding a sorting step might be a good fix for this?

Done! I definitely should have thought of that… 😄

I think It would be worth pulling the process-tree logic into its own module lib/process.js?

Sounds like a good idea… I’ve started splitting things up, I guess I’ll see if I get the self-coverage handling right.

could we get the coverage percentage for a given branch?

Definitely, and I agree that that would a pretty neat thing to have! It might require touching a bit more of the reporting logic, though.

@@ -326,7 +336,7 @@ NYC.prototype.clearCache = function () {
}

NYC.prototype.createTempDirectory = function () {
mkdirp.sync(this.tempDirectory())
mkdirp.sync(this.processInfoDirectory())
Copy link
Member

Choose a reason for hiding this comment

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

could we make it so we only create this directory, if we've enabled process tracking; my thinking is, let's make the behavior 100% identical, unless you are outputting the process tree.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, I’ll try that

@bcoe
Copy link
Member

bcoe commented Aug 25, 2016

Definitely, and I agree that that would a pretty neat thing to have! It might require touching a bit more of the reporting logic, though.

Let's hold off on this until V2 💃

I made a few more comments, also we should definitely figure out a good place to document this in the README; this having been said, I'm really loving this feature.

@addaleax
Copy link
Member Author

addaleax commented Aug 25, 2016

Let's hold off on this until V2

You mean, this PR, or per-subtree coverage reporting? I’m fine with either, but if you’re talking about the latter, I’d want to try and implement per-subtree reporting to make sure everything’s wired up in order for that to work

(edit: … i took the liberty to throw away all old coveralls comments which were clobbering up everything here)

Add a `--show-process-tree` that shows a pretty tree of all spawned
processes after `nyc` has run.

The data files for that are stored in `processinfo` are stored in
`(temp directory)/processinfo` so that they don’t interfere with
the fixed format of the coverage files.

Fixes: istanbuljs#158
…lves

This is in preparation for per-subtree coverage at some point.
@coveralls
Copy link

coveralls commented Aug 27, 2016

Coverage Status

Changes Unknown when pulling 88021d0 on addaleax:process-tree into * on istanbuljs:master*.

@bcoe
Copy link
Member

bcoe commented Aug 27, 2016

You mean, this PR, or per-subtree coverage reporting?

@addaleax I think this pull request is great, and we should land it ASAP -- just want to get the behavior identical if it's not enabled, and add some documentation to the README 👍

@addaleax
Copy link
Member Author

just want to get the behavior identical if it's not enabled

I think that’s the case. :)

and add some documentation to the README

Done! Please make sure this PR is in the state you’d like it to see in :)

@coveralls
Copy link

coveralls commented Aug 27, 2016

Coverage Status

Changes Unknown when pulling 3dad767 on addaleax:process-tree into * on istanbuljs:master*.

@bcoe
Copy link
Member

bcoe commented Aug 27, 2016

@addaleax awesome, on a short vacation to NYC, but will hopefully land some pull requests tomorrow or Monday.

@isaacs
Copy link
Collaborator

isaacs commented Aug 30, 2016

on a short vacation to NYC, but will hopefully land some pull requests tomorrow or Monday.

That's right, @bcoe is so committed to Open Source, he takes vacations just to visit his node modules.

@addaleax
Copy link
Member Author

@isaacs … glad to hear your dad joke license arrived ;) (ok truth is i have been giggling at this for five minutes straight)

@bcoe
Copy link
Member

bcoe commented Aug 30, 2016

@isaacs is this what you were picturing with #158, seems like a great start.

@isaacs
Copy link
Collaborator

isaacs commented Aug 30, 2016

@bcoe @addaleax Yeah, this looks really cool. It strikes me now that maybe storing the environment variables is a bad idea, since people will occasionally publish a coverage or .nyc_output folder to npm or github, and environs often have passwords in them.

@bcoe bcoe merged commit fabe5f3 into istanbuljs:master Sep 2, 2016
@bcoe
Copy link
Member

bcoe commented Sep 3, 2016

@addaleax mind giving the next release a spin:

npm cache clear; npm i nyc@next

if QA is looking good, we'll get this promoted soon.

@addaleax addaleax deleted the process-tree branch September 3, 2016 10:00
@addaleax
Copy link
Member Author

addaleax commented Sep 4, 2016

@bcoe Everything looks good so far :)

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.

Idea: track command, args, cwd, env of each process
4 participants