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

Support dashed args #7497

Merged
merged 5 commits into from
Dec 13, 2018
Merged

Support dashed args #7497

merged 5 commits into from
Dec 13, 2018

Conversation

hulkish
Copy link
Contributor

@hulkish hulkish commented Dec 11, 2018

Summary

Fixes #7424.

Test plan

Added tests & ran:

yarn build && yarn jest

@hulkish
Copy link
Contributor Author

hulkish commented Dec 11, 2018

@SimenB I imagine docs will need to be updated to reflect support for dashed args?

@SimenB
Copy link
Member

SimenB commented Dec 11, 2018

Can we either

I imagine docs will need to be updated to reflect support for dashed args?

Yeah, a note saying both ways of defining the options are supported would be nice 🙂


Mind updating the changelog as well?

@hulkish
Copy link
Contributor Author

hulkish commented Dec 11, 2018

@SimenB

@SimenB
Copy link
Member

SimenB commented Dec 11, 2018

Merging on our side is fine, then 🙂 I assume the values have been normalized, so it's not a lossy transformation

@hulkish
Copy link
Contributor Author

hulkish commented Dec 11, 2018

@SimenB

The problem i see with this - is that it will convert dashed to camel. Then, it will report as if you attempted the camelcased arg.

@hulkish
Copy link
Contributor Author

hulkish commented Dec 11, 2018

@SimenB ok, updated docs & changelog

@hulkish hulkish changed the title support dashed args Support dashed args Dec 11, 2018
@rickhanlonii
Copy link
Member

Is the change here that you could pass args as dashed instead of camel case? Is there a reason we shouldn't enforce the current camelCase style?

@SimenB
Copy link
Member

SimenB commented Dec 11, 2018

Standard cli is dashes, not camel. Jest is the only cli I know of that does camelCase

@hulkish
Copy link
Contributor Author

hulkish commented Dec 12, 2018

@SimenB @rickhanlonii I noticed this is failing the node@6 ci build. Should it be passing, in order to allow merge?

Btw, (unrelated to above): latest release 24.0.0-alpha.7 is fubar - missing build dirs in jest packages. Ex:

$ mkdir foo && cd foo && yarn init --yes
$ yarn add jest-cli@24.0.0-alpha.7
$ ls node_modules/jest-cli/
> bin/  node_modules/  LICENSE  README.md  package.json
$ cat node_modules/jest-cli/package.json | jq -r .main
> build/jest.js

@SimenB
Copy link
Member

SimenB commented Dec 12, 2018

Node 6 has to pass, yes 🙂 However, the failure looked transient, so I restarted the whole build. 🤞

@hulkish
Copy link
Contributor Author

hulkish commented Dec 12, 2018

@SimenB found out why it was failing: Error: You can only use --detectOpenHandles on Node 8 and newer. - which is from my test. So, easy fix by swapping it out with a diff dashed arg... this should be good to go now

@codecov-io
Copy link

codecov-io commented Dec 12, 2018

Codecov Report

Merging #7497 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7497      +/-   ##
==========================================
- Coverage   67.46%   67.45%   -0.01%     
==========================================
  Files         247      247              
  Lines        9516     9514       -2     
  Branches        5        6       +1     
==========================================
- Hits         6420     6418       -2     
  Misses       3094     3094              
  Partials        2        2
Impacted Files Coverage Δ
packages/jest-validate/src/validateCLIOptions.js 78.94% <100%> (+1.8%) ⬆️
packages/jest-config/src/getMaxWorkers.js 100% <0%> (ø) ⬆️

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 df78255...3f45e11. Read the comment docs.

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple comments on documentation

CHANGELOG.md Outdated
@@ -38,6 +38,7 @@

### Fixes

- `[jest-cli]` Support dashed args ([#7497](https://github.com/facebook/jest/pull/7497))
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this below the breaking changes, preferably at the bottom of this section?

docs/CLI.md Outdated
@@ -81,6 +81,21 @@ you can use:
npm test -- -u -t="ColorPicker"
```

## Camelcase & dashed args support

Jest supports both camelcase and dashed arg formats. Which means the following examples will have equal result:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Jest supports both camelcase and dashed arg formats. Which means the following examples will have equal result:
Jest supports both camelcase and dashed arg formats. The following examples will have equal result:

docs/CLI.md Outdated
jest --collectCoverage
```

They can also be mixed:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
They can also be mixed:
Arguments can also be mixed:

@hulkish
Copy link
Contributor Author

hulkish commented Dec 12, 2018

@rickhanlonii ok all set, I think.

@SimenB
Copy link
Member

SimenB commented Dec 13, 2018

CI still failing

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.

Just to verify, the object passed around as globalObject after jest-config has done its thing has just the camelCase keys, right?

arg => !allowedOptions.has(arg),
);
const unrecognizedOptions = Object.keys(argv).filter(arg => {
const camelCased = arg.replace(/-([^-])/g, (a, b) => b.toUpperCase());
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add camelize dep?

@hulkish
Copy link
Contributor Author

hulkish commented Dec 13, 2018

@SimenB

Just to verify, the object passed around as globalObject after jest-config has done its thing has just the camelCase keys, right?

Hmm so good question - because, this is already an issue in master + this pr. I'll include that fix here:


const buildArgv = (maybeArgv: ?Argv, project: ?Path) => {
  const rawArgv: Argv | string[] = maybeArgv || process.argv.slice(2);
  const argv: Argv = yargs(rawArgv)
    .usage(args.usage)
    .alias('help', 'h')
    .options(args.options)
    .epilogue(args.docs)
    .check(args.check).argv;

  validateCLIOptions(
    argv,
    Object.assign({}, args.options, {deprecationEntries}),
    Array.isArray(rawArgv)
      ? rawArgv.map(rawArgv => rawArgv.replace(/^--?/, ''))
      : Object.keys(rawArgv),
  );

  return argv;
};

aka...remove the dashed entries of argv on the return.

@hulkish
Copy link
Contributor Author

hulkish commented Dec 13, 2018

@SimenB k, using camelcase, filtering returned argv result & added test for it

@hulkish hulkish force-pushed the support-dashed-args branch 2 times, most recently from 0d1a477 to 90ce0fd Compare December 13, 2018 13:22
@hulkish
Copy link
Contributor Author

hulkish commented Dec 13, 2018

@SimenB hmm can u restart these failed ci runs? looks network related

nvm, fixed

@hulkish
Copy link
Contributor Author

hulkish commented Dec 13, 2018

@SimenB k, ci passing now

@rickhanlonii rickhanlonii merged commit 700e0da into jestjs:master Dec 13, 2018
@rickhanlonii
Copy link
Member

Thanks @hulkish and congrats on your first Jest PR!

thymikee added a commit to spion/jest that referenced this pull request Dec 18, 2018
* master: (24 commits)
  Add `jest.isolateModules` for scoped module initialization (jestjs#6701)
  Migrate to Babel 7 (jestjs#7016)
  docs: changed "Great Scott!" link (jestjs#7524)
  Use reduce instead of filter+map in dependency_resolver (jestjs#7522)
  Update Configuration.md (jestjs#7455)
  Support dashed args (jestjs#7497)
  Allow % based configuration of max workers (jestjs#7494)
  chore: Standardize filenames: jest-runner pkg (jestjs#7464)
  allow `bail` setting to control when to bail out of a failing test run (jestjs#7335)
  Add issue template labels (jestjs#7470)
  chore: standardize filenames in e2e/babel-plugin-jest-hoist (jestjs#7467)
  Add node worker-thread support to jest-worker (jestjs#7408)
  Add `testPathIgnorePatterns` to CLI documentation (jestjs#7440)
  pretty-format: Omit non-enumerable symbol properties (jestjs#7448)
  Add Jest Architecture overview to docs. (jestjs#7449)
  chore: run appveyor tests on node 10
  chore: fix failures e2e test for node 8 (jestjs#7446)
  chore: update docusaurus to v1.6.0 (jestjs#7445)
  Enhancement/expect-to-be-close-to-with-infinity (jestjs#7444)
  Update CHANGELOG formatting (jestjs#7429)
  ...
willsmythe pushed a commit to willsmythe/jest that referenced this pull request Dec 20, 2018
* added dashed arg support to fix jestjs#7424

* update docs & changelog

* fix dashed arg support tests

* move changelog item to correct location, small edit to docs

* use camelcase, filter dashed args on returns config
aleclarson pushed a commit to aleclarson/jest that referenced this pull request Dec 25, 2018
* added dashed arg support to fix jestjs#7424

* update docs & changelog

* fix dashed arg support tests

* move changelog item to correct location, small edit to docs

* use camelcase, filter dashed args on returns config
aleclarson pushed a commit to aleclarson/jest that referenced this pull request Dec 25, 2018
* added dashed arg support to fix jestjs#7424

* update docs & changelog

* fix dashed arg support tests

* move changelog item to correct location, small edit to docs

* use camelcase, filter dashed args on returns config
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
* added dashed arg support to fix jestjs#7424

* update docs & changelog

* fix dashed arg support tests

* move changelog item to correct location, small edit to docs

* use camelcase, filter dashed args on returns config
@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.

Unrecognized cli patterns for jest@beta
5 participants