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

Feature: broader uses for watch plugins #6473

Merged
merged 11 commits into from
Jul 12, 2018

Conversation

tdd
Copy link
Contributor

@tdd tdd commented Jun 15, 2018

Summary

As discussed in #6467, watch plugins could be much more useful if they could change a wider range of global config options. This implements a white-listed wider set, tests and docs it.

Two immediate use cases are toggler plugins for tests verbosity and coverage collection. They're already ready and tested on my local machine, but I have no idea how to get them inside the jest-community org, and I'll need the relevant minimum Jest version for a pertinent peerDependency.

Test plan

All tests pass on my machine. Extra tests are located in packages/jest-cli/__tests__/watch.test.js, and focus on confirming white-listing of allowed global config options is effective.

The docs in /docs were also updated (but they don't seem to be used by website/'s yarn start or yarn build, so I only checked their rendering in VS Code's Markdown preview).

This is not semver-major, as there are no breaking changes IMHO, this is more of an extra feature, so looks semver-minor to me.

Hope you like it!

Closes #6467

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

@codecov-io
Copy link

codecov-io commented Jun 15, 2018

Codecov Report

Merging #6473 into master will decrease coverage by 0.03%.
The diff coverage is 65.51%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6473      +/-   ##
==========================================
- Coverage   63.61%   63.58%   -0.04%     
==========================================
  Files         235      235              
  Lines        8971     8991      +20     
  Branches        3        4       +1     
==========================================
+ Hits         5707     5717      +10     
- Misses       3263     3273      +10     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-cli/src/watch.js 79.02% <ø> (ø) ⬆️
packages/jest-cli/src/lib/update_global_config.js 72.72% <65.51%> (-18.94%) ⬇️

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 40b0c6a...f36e644. Read the comment docs.

@tdd
Copy link
Contributor Author

tdd commented Jun 15, 2018

@rogeliog this is for your reviewing pleasure 😉


#### Authorized configuration keys

For stability and safety reasons, only part of the global configuration keys can be updated with `updateConfigAndRun`. The current white list is as follows:
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @SimenB!

  • --changedSince and --lastCommit do not have any direct equivalent in the global configuration object. They're used by getChangedFilesForRoots in jest-changed-files, and are also ignored when --onlyChanged (which is the default watch mode), so they don't seem to make sense in that mode. At any rate, the watch architecture would not let us change that on the fly right now, this is a bootstrap-time computation.
  • --onlyChanged is already togglable in watch mode: these are the a key (all tests, same as --watchAll) vs. o key (only changed tests, same as --onlyChanged).
  • --logHeapUsage does have a matching boolean in the global config, so I'm open to adding it, what do others think?

Copy link
Member

Choose a reason for hiding this comment

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

in the same way toggling onlyChanged is useful, I think changedSince would be useful. It would then overwrite onlyChanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB I agree it would be nice, but Jest's current architecture doesn't let us do it. It computes the watched list at boot time, then keeps it in a closure where specific global config options may amend it (such as the watch mode). There's no stuff in there right now for changedSince or lastCommit, and I feel the change would be significant.

This specific PR is bound by what's in the global config. Should this feature set become config-based, I'll be glad to expand the white list, but it's a different feature scope IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

@rogeliog sounds like we need another of your magical refactor runs 😀

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we would need to change a couple of things in order to make that happen 🙃

{virtual: true},
);

// updateGlobalConfig.mockReset();
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops, forgot to strip this 😒

@@ -86,6 +86,11 @@ jest.doMock(
{virtual: true},
);

const regularUpdateGlobalConfig = require('../lib/update_global_config')
Copy link
Member

Choose a reason for hiding this comment

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

You can import it to avoid the .default call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but as I'm mocking it right the next line, it seemed more interesting to keep it close by?

Copy link
Collaborator

Choose a reason for hiding this comment

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

jest.doMock should be called prior to require call

@SimenB
Copy link
Member

SimenB commented Jun 15, 2018

@tdd did you see the CLA comment?

@tdd
Copy link
Contributor Author

tdd commented Jun 15, 2018

@SimenB I did see the CLA comment and e-mailed to update and renew my CLA status. The online form fails in obscure ways for updates (always has, nobody's fixing it).

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@rogeliog rogeliog left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm excited about this!


#### Authorized configuration keys

For stability and safety reasons, only part of the global configuration keys can be updated with `updateConfigAndRun`. The current white list is as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we would need to change a couple of things in order to make that happen 🙃

);

// updateGlobalConfig.mockReset();
const whiteListedOptions = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could use it.each([objectWithWhitelistedOptionsAndOtherOptions])('')

https://facebook.github.io/jest/docs/en/api.html#testeachtable-name-fn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! Good point… Let me try…

} = {}) => {
const previousUpdateSnapshot = globalConfig.updateSnapshot;
// It is unfortunate that Flow isn't smart enough yet to let us
// implement a generic assign loop over a list of white-listed
// option names here :-(
globalConfig = updateGlobalConfig(globalConfig, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that there are a lot of options, maybe we could move a lot of this !== undefined logic into the updateGlobalConfig function, so that it is the only place that knows how to handle all of that logic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's an option. I really disliked the un-DRYness of this part of the code, but making it dynamically iterate over a list of props confused the heck out of Flow 😒 .

I'll try and move that logic inside updateGlobalConfig and see how it goes.

@tdd
Copy link
Contributor Author

tdd commented Jun 20, 2018

Hey @rogeliog!

Here's my update. It addresses the various suggestions you had, and it looks like it's going to greenlight through. Let me know what the next steps would be!

@tdd
Copy link
Contributor Author

tdd commented Jun 22, 2018

Hey @rogeliog just following up on this. Have you had time to peruse the latest changes? I'd love to know how and when we can move ahead with this 😉

@tdd tdd force-pushed the feat/broader-watch-plugins branch from ca07148 to a5f5c4b Compare June 22, 2018 12:06
@tdd
Copy link
Contributor Author

tdd commented Jun 22, 2018

Just pushed a force update to fix upstream merge conflicts over CHANGELOG.md after having rebased on top of it.

Copy link
Contributor

@rogeliog rogeliog left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks a lot!

@thymikee
Copy link
Collaborator

thymikee commented Jul 7, 2018

@tdd mind resolving conflicts? :)

@tdd
Copy link
Contributor Author

tdd commented Jul 7, 2018

@thymikee dang, there were none when this was originally approved. Also, all checks passed. Checking it now.

@tdd
Copy link
Contributor Author

tdd commented Jul 7, 2018

Grmbl, this thing stayed too long in limbo, approved but unmerged, as other PRs for watch plugins came in that broke this one. Will need to investigate. (Also, not cool, team 😐 )

@thymikee
Copy link
Collaborator

thymikee commented Jul 7, 2018

I'm pretty sure @rogeliog can help you with figuring out what's wrong now :).
Thanks for your patience though! We try our best to review and merge stuff as early as possible, but there's a lot to do 😅.

@tdd
Copy link
Contributor Author

tdd commented Jul 7, 2018

Hey @thymikee I sure would appreciate @rogeliog's help on this, the issue is the same across the board, when require'ing watch plugins' paths. jest-resolve b0rks further down the line. This is not code I touched in my PR and I have no idea just now about why the path would be missing here—and not break watch.js in master?

@rogeliog
Copy link
Contributor

rogeliog commented Jul 8, 2018

Yes, I'm more than happy to help. I'll take a look

@tdd
Copy link
Contributor Author

tdd commented Jul 15, 2018

Hey all! FWIW the initial release of the toggle-config plugin is out, now that 23.4.1 includes this.

Will move it to jest-community soon. Check it out: https://github.com/deliciousinsights/jest-watch-toggle-config#readme

tdd added a commit to deliciousinsights/jest that referenced this pull request Jul 15, 2018
- 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.
tdd added a commit to deliciousinsights/jest that referenced this pull request Jul 15, 2018
- 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.
@tdd
Copy link
Contributor Author

tdd commented Jul 15, 2018

Hey @rogeliog @SimenB I added you as admins on my toggle-config plugin so you would transfer it to jest-community. Once you've done so and added me back as contributor to it, I'll update its package.json, set up its Travis CI badge, semver-patch it and republish.

@SimenB
Copy link
Member

SimenB commented Jul 15, 2018

Done, and added you as admin

@tdd
Copy link
Contributor Author

tdd commented Jul 15, 2018

Awesome, thank you!

tdd added a commit to deliciousinsights/jest that referenced this pull request Jul 17, 2018
- 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.
tdd added a commit to deliciousinsights/jest that referenced this pull request Jul 18, 2018
- 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.
tdd added a commit to deliciousinsights/jest that referenced this pull request Jul 18, 2018
- 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.
tdd added a commit to deliciousinsights/jest that referenced this pull request Jul 22, 2018
- 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.
tdd added a commit to deliciousinsights/jest that referenced this pull request Jul 26, 2018
- 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.
thymikee pushed a commit to thymikee/jest that referenced this pull request Aug 8, 2018
- 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.
thymikee pushed a commit that referenced this pull request Aug 9, 2018
## 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.

Fixes #6693.
Refs #6473.

## 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.
@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.

Extend updateConfigAndRun to allow change of verbose and collectCoverage
6 participants