Created --notifyMode option for notifications on certain events #5125

psilospore · 2017-12-18T23:39:15Z

Summary

I would like notifications only on failure. It seems that someone else wanted something similar: #1839

Test plan

Unfortunately I'm not able to run yarn test. I end up with this error:

Error: packages/jest-circus/src/format_node_assert_errors.js:10
 10: import type {DiffOptions} from 'jest-diff/src/diff_strings.js';
                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ jest-diff/src/diff_strings.js. Required module not found

Also yarn install didn't work for me. I used npm install instead since I got this error:
error An unexpected error occurred: "https://registry.yarnpkg.com/@babel/code-frame/-/code-frame-7.0.0-beta.35.tgz: Request failed \"404 Not Found\"".

I followed the the contribution guide as well https://github.com/facebook/jest/blob/master/CONTRIBUTING.md

Updated Test Plan

run yarn jest --watch --notify --notifyMode=failure examples with the various notifyMode options.
success, always, failure, and change

Modify examples/snapshot/tests/link.react.test.js and check if notifications appear when appropriate.

facebook-github-bot · 2017-12-18T23:39:33Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

SimenB · 2017-12-18T23:43:52Z

You need to use yarn as we use yarn workspaces in this repo. Not sure about your error, the URL in you comment (https://registry.yarnpkg.com/@babel/code-frame/-/code-frame-7.0.0-beta.35.tgz) resolves for me. Can you try again, making sure you are on 1.3.2 of yarn?

Also, this needs some sort of test to illustrate it's working (although I understand it's hard to add a test when you're unable to install the repo)

facebook-github-bot · 2017-12-18T23:53:46Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

psilospore · 2017-12-19T00:03:33Z

@SimenB I understand I was hoping someone would happen to know how to fix this when I made the PR, and I would be able to quickly update my PR. It seems that updating yarn fixed my second issue, and running yarn add -W jest-diff as a workaround fixed my second issue.

I have a few issues now that it's running and I will try to update the PR soon. Thank you!

SimenB · 2017-12-19T07:03:32Z

docs/Configuration.md

@@ -378,9 +378,9 @@ array of absolute paths to additional locations to search when resolving
 modules. Use the `<rootDir>` string token to include the path to your project's
 root directory. Example: `["<rootDir>/app/"]`.

-### `notify` [boolean]
+### `notify` [string]


I'm not sure we want this to overload. Maybe a separate notify-mode or something? Changing its type is a breaking change, and would mean we couldn't merge this PR for months

Something like what travis gives you, where you can get a notification on failure, and when that turns green, but not green after one another. (https://docs.travis-ci.com/user/notifications/#Changing-notification-frequency)

The you could have onFailure, onSuccess, onChange. for instance.

That sounds like a better design. I like the additional events travis has as well. I'll try to spend some time tonight.

SimenB · 2017-12-19T07:04:11Z

CHANGELOG.md

@@ -74,7 +74,8 @@ None
  matcher ([#4917](https://github.com/facebook/jest/pull/4917))

 ### Features
-
+* `[jest-cli]` --notify is now configurable. `onFailure` or `onSuccess` are now valid options.


you can run yarn lint:md to autofix formatting in markdown files

psilospore · 2017-12-20T02:21:11Z

packages/jest-cli/src/reporters/notify_reporter.js

  }

  onRunComplete(contexts: Set<Context>, result: AggregatedResult): void {
    const success =
      result.numFailedTests === 0 && result.numRuntimeErrorTestSuites === 0;

-    if (success) {
+    const notifyMode = this._globalConfig.notifyMode;


Not sure what else I'm missing but when I log globalConfig I see that notify is set to true but notifyMode is undefined. However when I go into https://github.com/facebook/jest/blob/v22.0.0/packages/jest-cli/src/cli/index.js#L72
I see that notifyMode is set to the value I pass in.

I think you might be hit by https://github.com/facebook/jest/pull/4639/files#r143555141

That was it 🥇 still have to work on the change option though. It seems like a new instance of NotifyReporter is created every time jest --watch reruns. I'll look into that.

SimenB · 2017-12-20T20:31:00Z

There are flow errors, run yarn flow locally to see it

psilospore · 2017-12-26T03:04:23Z

packages/jest-cli/src/run_jest.js

+  firstRun: true,
+  previousSuccess: true,
+};
+


Let me know what you think of this. I wasn't sure what the best practice for updating the state from my previous run in NotifyReporter would be.

The NotifyReporter can have local state, can't it? Why not just have this._firstRun = false etc in there?

run_jest seems to create a new instance of TestScheduler every time a file changes with --watch and TestScheduler creates a new instance of NotifyReporter.

codecov-io · 2017-12-26T03:16:57Z

Codecov Report

Merging #5125 into master will increase coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5125      +/-   ##
==========================================
+ Coverage   61.42%   61.65%   +0.22%     
==========================================
  Files         213      213              
  Lines        7059     7067       +8     
  Branches        4        3       -1     
==========================================
+ Hits         4336     4357      +21     
+ Misses       2722     2709      -13     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-config/src/index.js	`25.92% <ø> (ø)`	⬆️
packages/jest-config/src/valid_config.js	`100% <ø> (ø)`	⬆️
packages/jest-config/src/defaults.js	`100% <ø> (ø)`	⬆️
packages/jest-config/src/normalize.js	`92.13% <ø> (ø)`	⬆️
packages/jest-cli/src/test_scheduler.js	`26.61% <100%> (-0.2%)`	⬇️
packages/jest-cli/src/reporters/notify_reporter.js	`75.86% <100%> (+67.16%)`	⬆️
packages/jest-cli/src/run_jest.js	`50.79% <100%> (+0.79%)`	⬆️

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 8fb789c...32bb3cc. Read the comment docs.

SimenB · 2017-12-26T09:59:37Z

docs/Configuration.md

+* `always`: always send a notification.
+* `failure`: send a notification when tests fail.
+* `success`: send a notification when tests pass.
+* `change`: send a notification when the status changed.


It's missing the mode I would use, which is "on every failure, and change to success". I want a notification that my tests are still red on every run until they're green, and when it turns green, but not on every run when it's green. I'm not sure how to best express it, though. Travis has onSuccess and onFailure with different modes, but it feels a bit overkill to add two new options.

What about: always, failure, success, change, failure-always, failure-change, success-always, success-change, and allow notifyMode to be passed an array instead of a single string?

I'm not sure if that's the best option, it's just what I could come up with without thinking too much about it

What would be the difference between failure-always and failure (same with success and success-always)? I personally would prefer keeping it a string because I can only imagine 2 cases from the current combination of options. e.g. ['failure', 'change'] to accomplish the case you mentioned, and I think it would be better expressed as a string with failure-change

success-change would accomplish the inverse.

What would be the difference between failure-always and failure

Good point, those would be the same. failure, success, always, change, failure-change and success-change works for me

Just added those :)

SimenB · 2017-12-26T10:00:21Z

packages/jest-cli/src/run_jest.js

@@ -75,6 +75,12 @@ const processResults = (runResults, options) => {
  return options.onComplete && options.onComplete(runResults);
 };

+// eslint-disable-next-line prefer-const


why is this needed?

eslint didn't seem to recognize that this will be mutated in NotifyReporter

You mutate the content, not the variable itself, can't this be const?

Oh looks like I just learned something https://jack.ofspades.com/es6-const-not-immutable/index.html

Yeah, there's no true native immutability in JS :(

SimenB · 2018-01-05T16:33:24Z

docs/Configuration.md

+
+Default: `always`
+
+Specifies notification mode.


Can you add that it also needs notify: true?

SimenB · 2018-01-05T16:33:52Z

CHANGELOG.md

@@ -75,6 +75,8 @@ None

 ### Features

+* `[jest-cli]` created `--notifyMode` added to specify when to be notified.


"added --notifyMode to specify when to be notified."

SimenB

Wait, I didn't notice this does not have tests 😱

Can you add some?

SimenB · 2018-01-21T10:06:03Z

@psilospore ping 🙂

psilospore · 2018-01-23T19:33:56Z

@SimenB sorry I've been busy lately but I'll try to make time this weekend!

SimenB · 2018-01-31T07:42:55Z

New test is failing CI, could you look into it?

This also needs a rebase 🙂

psilospore · 2018-01-31T21:20:48Z

@SimenB did you see my comment here: https://github.com/facebook/jest/pull/5125/files/65c929505f640531ea6b842867a002215e9bf9e7#diff-87435080064bc991e719d23f2a2ee8dd

I wanted to know if that was ok before I continued.

SimenB · 2018-01-31T22:14:13Z

@psilospore I'm unable to find the comment you're referring to...

psilospore · 2018-01-31T22:19:33Z

@SimenB This was the comment.

SimenB · 2018-01-31T22:27:40Z

Ah, you have to submit it for me to see it 🙂

But yeah, the test looks good!

psilospore · 2018-01-31T22:30:42Z

Oh I see. I'll try to work on it a bit this weekend. Add some more test cases, and everything else.

psilospore · 2018-02-05T17:13:21Z

packages/jest-cli/src/__tests__/notify_reporter.test.js

+
+const testModes = (notifyMode: string, arl: Array<AggregatedResult>) => {
+  const notify = require('node-notifier');
+  notify.notify.mock.calls = [];


mock.calls contains the calls of my previous tests. Is there a better way to do this?

Call jest.clearAllMocks() in afterEach

psilospore · 2018-02-06T00:09:21Z

Alright all ready :)

SimenB

This is awesome, thank you so much for sticking with it!

SimenB · 2018-02-06T10:05:10Z

CHANGELOG.md

@@ -286,6 +286,8 @@

 ### Features

+* `[jest-cli]` Added --notifyMode to specify when to be notified.


Can you move this up under master? (sorry 😅)

never mind, I can do it (and rebase, as it gave a conflict)

forgot about other notifyMode configs add notifyMode to normalize Created TestSchedulerContext to save previous status of test to make the change option work updated docs minor linting fix Added additional options such as success-change and failure-change Put conditions back in if else clauses Fixed documentation on notifyMode Added notify reporter test (for review) Finished NotifyReporter tests. Testing against simulated sequences of events.

psilospore · 2018-02-07T03:21:25Z

@SimenB no problem 😄 thanks so much for the help! This might be my first PR to get into a large open source project so I'm excited to see it out!

SimenB · 2018-02-07T10:45:20Z

@psilospore looking forward to more features from you 🙂

It's been released in 22.2.0.

github-actions · 2021-05-12T21:07:01Z

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.

facebook-github-bot added the CLA Signed ✔️ label Dec 18, 2017

SimenB reviewed Dec 19, 2017

View reviewed changes

psilospore force-pushed the feature/notify-on-results branch from dbb1363 to 409be3c Compare December 20, 2017 02:15

psilospore commented Dec 20, 2017

View reviewed changes

psilospore changed the title ~~Made --notify configureable. You can pass onFailure or onSuccess.~~ Created --notifyMode option for notifications on certain events Dec 20, 2017

psilospore commented Dec 26, 2017

View reviewed changes

SimenB reviewed Dec 26, 2017

View reviewed changes

SimenB reviewed Jan 5, 2018

View reviewed changes

docs/Configuration.md Outdated

Default: `always`

Specifies notification mode.

Copy link

Member

SimenB Jan 5, 2018

Choose a reason for hiding this comment

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

Can you add that it also needs notify: true?

SimenB reviewed Jan 5, 2018

View reviewed changes

SimenB approved these changes Jan 5, 2018

View reviewed changes

SimenB requested changes Jan 5, 2018

View reviewed changes

psilospore force-pushed the feature/notify-on-results branch 2 times, most recently from 5379f7f to 65c9295 Compare January 29, 2018 03:02

psilospore force-pushed the feature/notify-on-results branch from 65c9295 to 4b9ecf1 Compare February 5, 2018 03:58

psilospore force-pushed the feature/notify-on-results branch 2 times, most recently from 42cd78c to 0f3c9e1 Compare February 5, 2018 04:27

psilospore commented Feb 5, 2018

View reviewed changes

psilospore force-pushed the feature/notify-on-results branch from fb2b81e to c98a36d Compare February 5, 2018 23:52

SimenB approved these changes Feb 6, 2018

View reviewed changes

SimenB requested changes Feb 6, 2018

View reviewed changes

SimenB approved these changes Feb 6, 2018

View reviewed changes

psilospore and others added 4 commits February 6, 2018 11:07

hipsters and their emojis 😞

7bea23b

icons might not show in some envs

3f25876

Update CHANGELOG.md

fc99e6d

SimenB force-pushed the feature/notify-on-results branch from fff7535 to fc99e6d Compare February 6, 2018 10:08

SimenB and others added 2 commits February 7, 2018 09:10

Merge branch 'master' into feature/notify-on-results

32bb3cc

Merge branch 'master' into feature/notify-on-results

d36f9ab

cpojer merged commit 1947496 into jestjs:master Feb 7, 2018

renovate bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

		@@ -75,6 +75,8 @@ None

		### Features

		* `[jest-cli]` created `--notifyMode` added to specify when to be notified.

		@@ -286,6 +286,8 @@

		### Features

		* `[jest-cli]` Added --notifyMode to specify when to be notified.

Created --notifyMode option for notifications on certain events #5125

Created --notifyMode option for notifications on certain events #5125

Conversation

psilospore commented Dec 18, 2017 • edited Loading

Summary

Test plan

Updated Test Plan

facebook-github-bot commented Dec 18, 2017

SimenB commented Dec 18, 2017 • edited Loading

facebook-github-bot commented Dec 18, 2017

psilospore commented Dec 19, 2017

Choose a reason for hiding this comment

psilospore Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Dec 26, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psilospore Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Jan 21, 2018

psilospore commented Jan 23, 2018

SimenB commented Jan 31, 2018

psilospore commented Jan 31, 2018

SimenB commented Jan 31, 2018

psilospore commented Jan 31, 2018

SimenB commented Jan 31, 2018

psilospore commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psilospore commented Feb 6, 2018

SimenB left a comment

Choose a reason for hiding this comment

SimenB Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

SimenB Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

psilospore commented Feb 7, 2018

SimenB commented Feb 7, 2018 • edited Loading

github-actions bot commented May 12, 2021

psilospore commented Dec 18, 2017 •

edited

Loading

SimenB commented Dec 18, 2017 •

edited

Loading

psilospore Dec 19, 2017 •

edited

Loading

SimenB Dec 20, 2017 •

edited

Loading

codecov-io commented Dec 26, 2017 •

edited

Loading

psilospore Dec 26, 2017 •

edited

Loading

SimenB Feb 6, 2018 •

edited

Loading

SimenB Feb 6, 2018 •

edited

Loading

SimenB commented Feb 7, 2018 •

edited

Loading