Check watch plugins for key conflicts #6697

tdd · 2018-07-15T12:13:29Z

Summary

Watch plugins now are checked for key conflicts…

Some built-in keys remain overridable (specificically t and p).
Any key registered by a third-party watch plugin cannot be overridden by one listed later in the plugins list config.

Test plan

Additional tests are provided that check every conflict / overwritable scenario discussed.

Request for feedback / “spec” evolution

The “spec” is an ongoing discussion in #6693 — in particular, the overwritability of some built-in keys, such as a, f and o, may be subject to discussion. This PR tracks the decisions in there and may evolve a bit still.

Ping @SimenB @thymikee @rogeliog for review and discussion.

tdd · 2018-07-15T12:15:06Z

packages/jest-cli/src/watch.js

+const RESERVED_KEY_PLUGINS = new Map([
+  [
+    UpdateSnapshotsPlugin,
+    {forbiddenOverwriteMessage: 'updating snapshots ballpark', key: 'u'},


Note to reviewers: we specify the keys here because the snapshot built-in plugins only return usage info when the current running context makes them relevant, and this detection is based on internal, “private” field cooking. We don't want to interfere with / mock / hack that.

codecov-io · 2018-07-15T12:56:04Z

Codecov Report

Merging #6697 into master will increase coverage by 0.1%.
The diff coverage is 96.77%.

@@            Coverage Diff            @@
##           master    #6697     +/-   ##
=========================================
+ Coverage   63.55%   63.66%   +0.1%     
=========================================
  Files         235      235             
  Lines        9030     9059     +29     
  Branches        3        3             
=========================================
+ Hits         5739     5767     +28     
- Misses       3290     3291      +1     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-cli/src/watch.js	`81.97% <96.77%> (+2.95%)`	⬆️

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 0591f22...24cf0dc. Read the comment docs.

tdd · 2018-07-17T14:41:09Z

I just force-pushed an updated branch on top of latest master, to fix conflicts w/o butchering the graph with a reverse merge (as GitHub's UI would do). As I was force-pushing anyway, I fused both commits (original and lint:md:ci fix) as I'm not on in this for the contributors hall of fame anyway 😉

SimenB · 2018-07-18T07:30:01Z

We squash on merge anyways 🙂

@rogeliog would you be able to take a look at this?

SimenB · 2018-07-18T07:30:59Z

I think this deserves some docs as well, mind updating https://github.com/facebook/jest/edit/master/docs/WatchPlugins.md?

tdd · 2018-07-18T08:59:34Z

@SimenB sure thing, here goes. I took the opportunity to rebase on top of the latest master, too.

tdd · 2018-07-18T09:03:18Z

@SimenB unrelated: what's the Jest site release process like? I thought this would be automated, but it lags behind a bit most of the time (right now it's stuck on 23.3 for instance).

SimenB · 2018-07-18T09:10:17Z

See https://jestjs.io/docs/en/next/getting-started.html.

We cut new versions when the doc warrants it. None of the changes in 23.4 had doc changes

tdd · 2018-07-18T09:13:41Z

Ah, OK. 23.4.1 had (the broader updatable options set), but as it's very recent and a patchlevel, that probably explains why it's not there yet.

tdd · 2018-07-18T09:14:36Z

(also weird: the next version fails on watch-plugins?!)

rogeliog · 2018-07-18T15:18:19Z

packages/jest-cli/src/watch.js

+const RESERVED_KEY_PLUGINS = new Map([
+  [
+    UpdateSnapshotsPlugin,
+    {forbiddenOverwriteMessage: 'updating snapshots ballpark', key: 'u'},


Should we change this to just updating snapshots instead of update snapshots ballpark?

rogeliog · 2018-07-18T15:21:30Z

packages/jest-cli/src/watch.js

+        continue;
+      }
+      const {forbiddenOverwriteMessage} = reservedInfo;
+      watchPluginKeys.set(key, {


Given that the key is gotten from getUsageInfo(...) , a plugin can update their designated key at any time. This is not a documented(or common) use case, so I think if we don't worry about it in this PR.

Indeed. There is no formal way to obtain a list of all possible keys claimed by a plugin, so we have to trust first invocation. Context-based key changes are expected to be rather rare, though (although snapshot updates are a prime example, for instance), so let's not focus too much on that now.

rogeliog · 2018-07-18T15:22:52Z

docs/WatchPlugins.md

+- `p` (test filename pattern)
+- `t` (test name pattern)
+
+Any key not used by built-in functionality remains fair game, as you would expect. Try to avoid using keys that are difficult to obtain on various keyboards (e.g. `é`, `€`), or not visible by default (e.g. many Mac keyboards do not have visual hints for characters such as `|`, `\`, `[`, etc.)


nit: Don't worry about changing it, but....I'm not sure how remains fair game sounds for docs purposes

Good point. I wax colloquial sometimes, losing sight of how it can make reading a bit harder for non-native English speakers. Tweaked it.

tdd · 2018-07-18T22:03:32Z

I just pushed a rebased history on top of the latest master that addresses @rogeliog's good wording points.

tdd · 2018-07-18T22:04:41Z

Dammit, it looks like Netlify's deploy or some underlying API (OpenCollective?) is acting funky again 😒

rogeliog

Awesome!

tdd · 2018-07-19T20:07:38Z

Hey all, So how do we retrigger checks on this to make it pass? I'm fairly confident its breakage on Netlify was circumstancial.

On Thu, Jul 19, 2018, at 17:54, Rogelio Guzman wrote: ***@***.**** approved this pull request. Awesome! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub[1], or mute the thread[2].

-- Christophe Porteneuve tdd@tddsworld.com Links: 1. #6697 (review) 2. https://github.com/notifications/unsubscribe-auth/AAAOqPaydHsrZpUvbGVzPC1oTbjJ9Rwvks5uIKuqgaJpZM4VQOcf

rogeliog · 2018-07-19T22:46:17Z

I'm not sure how to retrigger the Netlify check. I guess you could amend your last commit and re-push 😕?

tdd · 2018-07-20T07:35:04Z

@rogeliog @SimenB Netlify check fails fast still. The message is extremely unclear:

9:34:08 AM: Failing build: Failed to prepare repo
9:34:08 AM: failed during stage 'preparing repo': exit status 1

If anyone on the build infra team could look at it…

endiliey · 2018-07-22T07:31:51Z

Hi, I've re-triggered the netlify preview. There is a build cache error causing it to fail.

tdd · 2018-07-22T09:21:54Z

@endiliey thanks!

Dang it, this time it's a Yarn registry timeout that failed test-jest-circus. As for the CHANGELOG conflict, I'll rebase and force push, we'll see how CI goes.

tdd · 2018-07-22T09:49:26Z

@endiliey @rogeliog @SimenB success! all checks pass now! Ready to merge 😉

SimenB · 2018-07-25T10:49:06Z

packages/jest-cli/src/watch.js

+    error = `Jest configuration error: watch plugins ${plugins} both attempted to register key <${key}>. Please change the key configuration for one of the conflicting plugins to avoid overlap.`;
+  }
+  console.error('\n\n' + chalk.red(error));
+  exit(64); // EX_USAGE


I wonder if it makes more sense to just throw a ValidationError like we do for other configuration errors.

E.g.

{ "jest": { "collectCoverage": "wat" } }

@SimenB this is an interesting idea. Am I correct in assuming the right place for this would be in jest-config’s normalize.js file, in its existing handling of the watchPlugins option?

(I'm not entirely sure we'd have sufficient/relevant info to populate the arguments to the plugin's getUsageInfo() there, I'll check. At the very least we can check inline key configs, though.)

Yeah, in normalize seems natural, imo. Not sure if we have enough information there, but if not we can just hard-code the reserved ones

tdd · 2018-07-26T13:28:07Z

@SimenB it turned out that moving that into jest-config's codebase wasn't practical, nor was it very clean IMHO:

It brought watch-specific concerns into the general config package
It occurred way ahead of the curve, before watch plugins can be instantiated (no global config yet, it requires normalization first)

I ended up keeping my conflict-checking code in its current location, but moved from manual console error logging and custom exit code to regular ValidationError throwing, trying to use error styling that matches existing validation errors. This works quite well in my tests.

SimenB · 2018-07-26T13:33:13Z

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

@@ -364,6 +364,139 @@ describe('Watch mode flows', () => {
    expect(run).toHaveBeenCalled();
  });

+  describe('when dealing with potential watch plugin key conflicts', () => {
+


This line causes lint failure on CI

Dang, should have checked. Just a sec.

yarn lint --fix should solve your problem 👍

It's odd, I guess my VS Code was overwhelmed with some extension, not to have auto-run Prettier on this.

- Some built-in keys remain overridable (specificically `t` and `p`). - Any key registered by a third-party watch plugin cannot be overridden by one listed later in the plugins list config. Fixes jestjs#6693. Refs jestjs#6473.

github-actions · 2021-05-12T10:07:26Z

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.

tdd commented Jul 15, 2018

View reviewed changes

facebook-github-bot added the cla signed label Jul 15, 2018

SimenB requested a review from rogeliog July 16, 2018 12:30

tdd force-pushed the feat/watch-plugin-key-conflicts branch from 20cfedc to 4efebe8 Compare July 17, 2018 14:39

tdd force-pushed the feat/watch-plugin-key-conflicts branch from 4efebe8 to 08124bb Compare July 18, 2018 08:59

rogeliog approved these changes Jul 18, 2018

View reviewed changes

tdd force-pushed the feat/watch-plugin-key-conflicts branch from 08124bb to 703fd05 Compare July 18, 2018 22:02

rogeliog approved these changes Jul 19, 2018

View reviewed changes

tdd force-pushed the feat/watch-plugin-key-conflicts branch from 9a7dc9a to cc69070 Compare July 22, 2018 09:29

SimenB reviewed Jul 25, 2018

View reviewed changes

tdd force-pushed the feat/watch-plugin-key-conflicts branch from cc69070 to 00567a8 Compare July 26, 2018 13:25

SimenB approved these changes Jul 26, 2018

View reviewed changes

SimenB reviewed Jul 26, 2018

View reviewed changes

SimenB approved these changes Jul 26, 2018

View reviewed changes

thymikee changed the base branch from master to process-exit August 8, 2018 20:04

thymikee changed the base branch from process-exit to master August 8, 2018 20:04

tdd and others added 6 commits August 8, 2018 22:06

Watch plugins now are checked for key conflicts…

e1f7be6

- Some built-in keys remain overridable (specificically `t` and `p`). - Any key registered by a third-party watch plugin cannot be overridden by one listed later in the plugins list config. Fixes jestjs#6693. Refs jestjs#6473.

copy(watch plugins): document key conflict management

4df6a73

copy: address wording issues raised by @rogeliog in PR review

3f3422c

refactor: rely on ValidationError for watch plugin key conflicts

848bfe4

style: made lint happy

34474f9

update changelog

24cf0dc

thymikee force-pushed the feat/watch-plugin-key-conflicts branch from 621e84e to 24cf0dc Compare August 8, 2018 20:09

Merge branch 'master' into feat/watch-plugin-key-conflicts

5f92d32

thymikee merged commit f8f966b into jestjs:master Aug 9, 2018

This was referenced Oct 1, 2018

Update jest in /modules/api from 22.4.3 to 23.6.0 project-accelerate/accelerate#245

Open

Update jest in /modules/frontend from 22.4.3 to 23.6.0 project-accelerate/accelerate#272

Open

dependencies bot mentioned this pull request Nov 1, 2018

Update jest in / from 22.1.4 to 23.6.0 leonardofaria/leozera-ui#250

Closed

snyk-bot mentioned this pull request Mar 17, 2020

[Snyk] Upgrade expect from 23.0.0 to 23.6.0 yowainwright/react-workshop#8

Merged

dependabot bot mentioned this pull request Mar 13, 2021

Bump babel-jest from 18.0.0 to 23.6.0 CartoonIsArt/eevee#133

Closed

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

Check watch plugins for key conflicts #6697

Check watch plugins for key conflicts #6697

Conversation

tdd commented Jul 15, 2018

Summary

Test plan

Request for feedback / “spec” evolution

Choose a reason for hiding this comment

codecov-io commented Jul 15, 2018 • edited Loading

Codecov Report

tdd commented Jul 17, 2018

SimenB commented Jul 18, 2018

SimenB commented Jul 18, 2018

tdd commented Jul 18, 2018

tdd commented Jul 18, 2018

SimenB commented Jul 18, 2018

tdd commented Jul 18, 2018

tdd commented Jul 18, 2018

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

tdd commented Jul 18, 2018

tdd commented Jul 18, 2018

rogeliog left a comment

Choose a reason for hiding this comment

tdd commented Jul 19, 2018 via email

rogeliog commented Jul 19, 2018

tdd commented Jul 20, 2018

endiliey commented Jul 22, 2018

tdd commented Jul 22, 2018

tdd commented Jul 22, 2018

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

tdd commented Jul 26, 2018

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

github-actions bot commented May 12, 2021

codecov-io commented Jul 15, 2018 •

edited

Loading