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

Add --showConfig argument #3296

Merged
merged 2 commits into from
Apr 18, 2017
Merged

Add --showConfig argument #3296

merged 2 commits into from
Apr 18, 2017

Conversation

wyze
Copy link
Contributor

@wyze wyze commented Apr 12, 2017

Summary

This solves #3288. It adds a JSON output of the config and then exits.

  • Adds a --showConfig argument.
  • Updates the --debug output to be JSON.
  • Updates the jest-editor-support package to use --showConfig.

Test plan

  • Integration tests pass.
  • jest --showConfig outputs config and exits.

@codecov-io
Copy link

codecov-io commented Apr 13, 2017

Codecov Report

Merging #3296 into master will increase coverage by 0.33%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3296      +/-   ##
==========================================
+ Coverage    63.9%   64.23%   +0.33%     
==========================================
  Files         176      176              
  Lines        6475     6465      -10     
  Branches        4        4              
==========================================
+ Hits         4138     4153      +15     
+ Misses       2336     2311      -25     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-editor-support/src/Settings.js 100% <100%> (+100%) ⬆️
packages/jest-cli/src/lib/logDebugMessages.js 100% <100%> (ø) ⬆️
packages/jest-cli/src/watch.js 64.61% <0%> (-0.54%) ⬇️
packages/jest-editor-support/src/Process.js 20% <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 d9c2f99...5617a2f. Read the comment docs.

it('outputs config info and exits', () => {
/* eslint-disable sort-keys */
const {stdout} = runJest(dir, ['--showConfig', '--no-cache']);
expect(JSON.parse(stdout)).toEqual({
Copy link
Contributor

Choose a reason for hiding this comment

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

We should change this to a snapshot test... If there are any fullPaths we can fix them with a serializer for paths that replaces /some/full/path/ -> /mocked/full/path/, something like:

const rootPath = path.resolve(/* resolve root path */);
expect.addSnapshotSerializer({
  test: val => typeof val === 'string' && val.startsWith(rootPath),
  print: val => val.replace(rootPath, '/mocked/root/path'),
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I was going to do snapshot at first but because of the paths, I went this way. Will add the serializer and change this to a snapshot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if you have some questions about the serializer 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, snapshot sounds like a great idea here. Don't wanna keep this in sync manually.

const config = readConfig(argv, root);
if (argv.showConfig) {
const output = ({config}) => {
pipe.write(JSON.stringify(config, null, 2));
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on what @cpojer mentioned I think we can make this use logDebugMessage and update logDebugMesage to print out a JSON... Is that ok @cpojer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, maybe we can move this to where argv.debug is used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't see that right below inside of _run. I agree, I'll get that moved.

Copy link
Contributor Author

@wyze wyze Apr 13, 2017

Choose a reason for hiding this comment

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

I said back in the issue that it appeared to be 2 changes here. One being adding this flag and outputting JSON config. The other being to change --debug output to be JSON as well. I'm willing to make the change in this PR, but I figured they would be separate changes therefor separate PRs.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can do it in a single PR.

@wyze
Copy link
Contributor Author

wyze commented Apr 14, 2017

Alright, changed the test over to snapshot and made the additional requested changes.

One thing I couldn't get working is the Settings test for jest-editor-support. If someone could point me in the right direction to fix that test, that would be great.

@@ -15,42 +15,37 @@ jest.mock('../../../package.json', () => ({version: 123}));
jest.mock('myRunner', () => ({name: 'My Runner'}), {virtual: true});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file lacks 'use strict'; declaration, can you add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing!

expect.addSnapshotSerializer({
print: val => val.replace(new RegExp(root, 'g'), '/mocked/root/path'),
test: val => typeof val === 'string' && val.indexOf(root) > -1,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

You also need to change "name" to something like "[MD5 HASH]" because by default it gets assigned random hash.

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 noticed that last night before I went to bed. Will get that fixed up!

@wyze wyze force-pushed the show-config-arg branch 3 times, most recently from ab88e82 to 290fd30 Compare April 14, 2017 20:06
@wyze
Copy link
Contributor Author

wyze commented Apr 14, 2017

I've updated the original description to include everything changed in here. I tried my best adding a test for the Settings module for jest-editor-support package, please give feedback on that. I've address all other feedback so far, good for another round of reviews. :)

@cpojer cpojer merged commit 54cff1e into jestjs:master Apr 18, 2017
@cpojer
Copy link
Member

cpojer commented Apr 18, 2017

cc @orta – we may need to make the code in jest-editor-support work with older releases prior to 20.

@wyze wyze deleted the show-config-arg branch April 18, 2017 00:56
skovhus pushed a commit to skovhus/jest that referenced this pull request Apr 29, 2017
* Add --showConfig argument

* Update CLI.md
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Add --showConfig argument

* Update CLI.md
@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 13, 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.

6 participants