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

Print deprecation warnings on CLI flags #5536

Merged
merged 3 commits into from
Feb 16, 2018

Conversation

Aftabnack
Copy link
Contributor

@Aftabnack Aftabnack commented Feb 12, 2018

Summary

Test plan

  • Integration test to look for deprecation warning printed

@codecov-io
Copy link

codecov-io commented Feb 12, 2018

Codecov Report

Merging #5536 into master will decrease coverage by 0.08%.
The diff coverage is 30.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5536      +/-   ##
==========================================
- Coverage   60.63%   60.54%   -0.09%     
==========================================
  Files         213      213              
  Lines        7311     7328      +17     
  Branches        3        3              
==========================================
+ Hits         4433     4437       +4     
- Misses       2877     2890      +13     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-util/src/index.js 89.47% <ø> (-10.53%) ⬇️
packages/jest-config/src/index.js 28.57% <100%> (+2.64%) ⬆️
packages/jest-validate/src/index.js 100% <100%> (ø) ⬆️
packages/jest-validate/src/validate_cli_options.js 44.44% <18.18%> (ø)
packages/jest-validate/src/utils.js 80% <0%> (-20%) ⬇️

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 966aab6...86ca4ae. Read the comment docs.

@Aftabnack
Copy link
Contributor Author

Couple things to note here:

  • I have moved validate_cli_opts to jest-validate, as a result I had to add jest-validate as a dependancy in some places. This might increase final size for jest-repl and jest-runtime.
  • Also, I have exposed jest-config/src/deprecated.js as deprecatedEntries. And used it directly in validate_cli_opts.
  • I know that will make code more imperative than declarative, but since this won't be necessary when we merge CLI flags with config in future. (If needed I can pass those deprecationEntries from wherever it's called [jest-repl, jest-runtime], which would in turn add to the bundle size of those)

Good things here:

  • All validations are now in jest-validate package
  • No extra attributes in jest-cli/src/cli/args.js
  • Works off of the single source of truth for all deprecations. jest-config/src/deprecated.js

Pointers for future:

  • Merge CLI flags with whatever's supplied in config file. CLI flags takes precedence
  • Maintain single source of truth for attributes that are common in CLI flags and Configuration.

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.

Needs test and a changelog update 🙂

},
{},
);
const deprecations = new Set(Object.keys(CLIDeprecations));
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some tests for this behavior?

@SimenB SimenB requested a review from thymikee February 12, 2018 22:56
@thymikee
Copy link
Collaborator

Can we now remove jest-validate from jest-util deps? :) Also tests and maybe some screenshots

@Aftabnack
Copy link
Contributor Author

Screenshot after copy pasting mapCoverage deprecation from my other pull.

screenshot from 2018-02-13 13-46-37

@Aftabnack
Copy link
Contributor Author

How can I assert on what is being printed on terminal?

@thymikee
Copy link
Collaborator

Add an integration test and snapshot the stdout

@Aftabnack
Copy link
Contributor Author

I was trying to add that in the package tests under src, there I cannot do this right?

@thymikee
Copy link
Collaborator

thymikee commented Feb 13, 2018

You can add some unit tests for jest-validate and a module that's using it, but we'd still appreciate integration test with running actual CLI.

@Aftabnack
Copy link
Contributor Author

Aftabnack commented Feb 13, 2018

I think I am using the I think I am using jest.mock in a wrong way. Snapshot doesn't have the deprecation message. What am I doing wrong?

}));

it('Prints deprecation warnings for CLI flags', () => {
const {stderr, stdout} = runJest(dir, ['--cache']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's an end to end test, so we should assert on real values. Just pass --preprocessorIgnorePatterns which is already deprecated and you can get rid of mocking jest-config

Copy link
Contributor Author

@Aftabnack Aftabnack Feb 13, 2018

Choose a reason for hiding this comment

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

But the issue is preprocessorIgnorePatterns is not a CLI arg. It throws an error. None of the existing deprecations are in CLI args

And what happens in the future those deprecations are removed from the code. That's why I thought I should mock it.

Copy link
Member

@rickhanlonii rickhanlonii Feb 13, 2018

Choose a reason for hiding this comment

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

Maybe we add an internal/undocumented cli arg/config like --printDeprecated

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'd prefer being able to mock it rather than adding a hidden options. That maybe the last resort.


it('Prints deprecation warnings for CLI flags', () => {
const {stderr, stdout} = runJest(dir, ['--cache']);
expect(stderr).toMatchSnapshot();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's sufficient to just do a regex check for Test Suites: 1 passed, 1 total.
Snapshot contains changing data (execution time) which breaks on other machines, unless you replace them with something static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@Aftabnack Aftabnack force-pushed the jest-validate-cli-deprecations branch from 1400a03 to 1c2b47c Compare February 13, 2018 11:01
@Aftabnack
Copy link
Contributor Author

#5536 (comment)

How can I mock jest-config for an integration testcase?

@Aftabnack
Copy link
Contributor Author

Aftabnack commented Feb 15, 2018

Looks like there is no other way to do this.
Can I add a hidden parameter to the jest CLI which will be used to write integration test cases for deprecation.?

/cc @thymikee @SimenB @rickhanlonii

@Aftabnack Aftabnack force-pushed the jest-validate-cli-deprecations branch from 8eac9ce to f620a4c Compare February 15, 2018 17:55
@rickhanlonii
Copy link
Member

@Aftabnack I'd either add --printDeprecated or just use one that's already there and we can update the test if/when we remove it

@Aftabnack
Copy link
Contributor Author

Since my other PR is merged. I can use mapCoverage to test.

@Aftabnack Aftabnack force-pushed the jest-validate-cli-deprecations branch from f620a4c to 86ca4ae Compare February 15, 2018 18:42
@Aftabnack
Copy link
Contributor Author

Aftabnack commented Feb 15, 2018

I think this is also ready to go!

/cc @thymikee @cpojer

@@ -47,5 +46,4 @@ module.exports = {
installCommonGlobals,
isInteractive,
setGlobal,
validateCLIOptions,
Copy link
Member

Choose a reason for hiding this comment

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

technically a breaking change. not sure if it matters, though

Choose a reason for hiding this comment

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

It absolutely matters. Please see #5749

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.

Nice, thanks!

@cpojer cpojer merged commit ba8a2d5 into jestjs:master Feb 16, 2018
@Aftabnack Aftabnack deleted the jest-validate-cli-deprecations branch February 16, 2018 10:30
@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.

Allow jest-validate to print deprecation warnings for CLI flags
8 participants