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

Program never ends when actions given on MacOS #283

Closed
jackkinsella opened this issue Aug 13, 2019 · 6 comments
Closed

Program never ends when actions given on MacOS #283

jackkinsella opened this issue Aug 13, 2019 · 6 comments

Comments

@jackkinsella
Copy link

jackkinsella commented Aug 13, 2019

I noticed that when running the jest unit testing library with --watch --notify (which calls node-notifier), my CPU usage soon went up to 90%, due to multiple instances of node-notifier hanging around. There are a couple of other people with the same issue over on in the jest repo.

Here's an example process that was spinning:

./myapp/node_modules/node-notifier/vendor/mac.noindex/terminal-notifier.app/Contents/MacOS/terminal-notifier -actions Run again,Exit tests -closeLabel "Close" -message "-&M-T️ 1 of 29 tests failed" -title "4% Failed" -ap

I experimented around with the node-notifier library's API and noticed that if I passed the same configuration options to node-notifier as jest does, the program never ends. To see what I mean, run the following in a stand-alone program. You'll notice it displays the notification but the program keeps running indefinitely.

// reproduceIssue.js
// Usage: node reproduceIssue.js

const notifier = require('node-notifier')
notifier.notify(
  {
    actions: [ 'Run again', 'Exit tests' ],
    closeLabel: 'Close',
    message: '⛔️ 1 of 9 tests failed',
    title: '12% Failed'
  }, () => {}
)

However, when I removed the actions attribute, the program ends after the notification is shown. I could also get the program to end, even with actions, by adding an explicit timeout attribute:

const notifier = require('node-notifier')
notifier.notify(
  {
    actions: [ 'Run again', 'Exit tests' ],
    closeLabel: 'Close',
    // new code
    timeout: 5,
    // end new code
    message: '⛔️ 1 of 9 tests failed',
    title: '12% Failed'
  }, () => {}
)

So my question is — is this indefinite running while awaiting an action expected behavior? If so, I'll make a PR in jest. If not, then I'll wait for an update to this library.

Attached is a log of a single iteration of the repeated system calls. system-calls.log

@jnielson94
Copy link
Collaborator

@jackkinsella - Can you provide any more information like what versions of the libraries you're using are at? It sounds like it might be related to #218 (which we'll try again to introduce a default in a future 6.0 release but rolled it back due to issues).

@jackkinsella
Copy link
Author

jackkinsella commented Aug 13, 2019

Sure thing:

Anything more info I can provide?

@jnielson94
Copy link
Collaborator

That looks good to me. It definitely sounds related to #218 - we had to roll back adding a default timeout since it caused the notifier to wait to close until after Jest was expecting. It does sound like we might need to work with the jest team to add a timeout if they're going to use actions though.

It looks like they already have a switch based on watch mode to add the actions - https://github.com/facebook/jest/blob/master/packages/jest-reporters/src/notify_reporter.ts#L108-L122 so we should see if they'd add a timeout when using actions (we actually rolled back #218 because of that first invocation where they expect it to close immediately). Does that make sense? If it does, we'll close this one and open an issue (or PR) to jest.

@jackkinsella
Copy link
Author

Yep adding a timeout to the watch mode in jest makes good sense to me!

The puzzling thing, which seems unrelated to either code-base, is why the terminal-notifier in macOS is such a CPU hog.

@jnielson94
Copy link
Collaborator

I haven't any ideas on the terminal-notifier issues, you might be able to dig into it in their repository,

If you can throw up an issue on the jest repository to see if they'll add a timeout to the watch mode notification (since it uses actions and therefore doesn't close automatically), that'd be awesome. Otherwise we'll eventually get around to trying again with a default timeout (which would require adding timeout: false or whatever we end up with to the non-watch notification anyway).

@jackkinsella
Copy link
Author

jackkinsella commented Aug 15, 2019

OK have done: jestjs/jest#8831

Nice working with you :)

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

No branches or pull requests

2 participants