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 package name to NotifyReporter notification #5898

Merged
merged 3 commits into from
Aug 9, 2018
Merged

Add package name to NotifyReporter notification #5898

merged 3 commits into from
Aug 9, 2018

Conversation

GeorgeSapkin
Copy link
Contributor

@GeorgeSapkin GeorgeSapkin commented Mar 29, 2018

Summary

This change adds package name to notification in NotifyReporter when using --notify flag or notify configuration option. This clarifies which package is being tested which is especially useful when watching tests in mono-repos and with multiple IDEs opened.

First I'm trying to get hasteFS from the first context to get the module name (must be a better way). Then I'm falling back to rootDir and otherwise to original behavior without an additional title.

Before:

Before

After:

After

SimenB
SimenB previously requested changes Mar 29, 2018
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

I think this might be useful!

CI is unhappy though, mind taking a look?

@@ -22,6 +23,8 @@ const isDarwin = process.platform === 'darwin';

const icon = path.resolve(__dirname, '../assets/jest_logo.png');

const readFile = util.promisify(fs.readFile);
Copy link
Member

Choose a reason for hiding this comment

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

Not available in node 6, please use pify

Copy link
Member

Choose a reason for hiding this comment

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

Disregard this, shouldn't need fs here at all?

'package.json',
);
const {name: packageName = this._globalConfig.rootDir} = JSON.parse(
await readFile(packagePath),
Copy link
Member

Choose a reason for hiding this comment

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

This file should be available in hastefs already, should not be necessary to hit the file system

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check it out.

@rickhanlonii
Copy link
Member

This sounds great! Do you have a screenshot of the new notification?

@GeorgeSapkin
Copy link
Contributor Author

@SimenB I've rewritten the code a bit using hastefs. I don't think this change has anything to do with test-node-9 failing in CI.

@rickhanlonii I've updated the PR description with screenshots.

@rickhanlonii
Copy link
Member

What do you think about leading with the percentage:

100% Passed (pubsub-store)

@rickhanlonii
Copy link
Member

And yeah, the node 9 failure is unrelated 👌

@SimenB
Copy link
Member

SimenB commented Apr 15, 2018

Sorry about the silence, forgot about this one!

If you rebase on master, the test failure should go away.

This needs a test as well :)

@rickhanlonii
Copy link
Member

@GeorgeSapkin bump, this is great, would love to get this in for Jest 23

@codecov-io
Copy link

codecov-io commented Apr 29, 2018

Codecov Report

Merging #5898 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5898      +/-   ##
==========================================
+ Coverage   63.55%   63.58%   +0.02%     
==========================================
  Files         235      235              
  Lines        9030     9037       +7     
  Branches        3        4       +1     
==========================================
+ Hits         5739     5746       +7     
  Misses       3290     3290              
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-cli/src/reporters/notify_reporter.js 80% <100%> (+4.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db28956...4fac224. Read the comment docs.

@GeorgeSapkin
Copy link
Contributor Author

@SimenB I've rebased and existing checks are passing. What kind of additional tests do I need to write?

@SimenB
Copy link
Member

SimenB commented May 1, 2018

There are no tests added in this PR, some unit tests asserting that the newly added code does its job would be awesome.
You can mock out node-notifier to verify it's called with the correct arguments

@cpojer
Copy link
Member

cpojer commented May 4, 2018

@GeorgeSapkin do you have any time to take this PR over the finish line?

@GeorgeSapkin
Copy link
Contributor Author

I'll try to add the missing tests this weekend.

@thymikee thymikee dismissed SimenB’s stale review August 8, 2018 20:45

feedback addressed

@thymikee
Copy link
Collaborator

thymikee commented Aug 8, 2018

@GeorgeSapkin mind rebasing this? After that we should be good to merge, sorry for this taking so long!

@GeorgeSapkin
Copy link
Contributor Author

deploy/netlify is failing. How can I address that?

@thymikee
Copy link
Collaborator

thymikee commented Aug 9, 2018

It's failing allover the repo, I don't know what's the issue :(.
cc @endiliey

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants