Feature: Config validation #141

okonet · 2017-03-17T16:17:45Z

This PR implements config validation 🎉

Why

Invalid config is a constant source of issues for this library. To reduce the amount of such tickets and to make deprecations easier, I've introduced jest-validate to this repository. Additionally, lots of refactoring and additional tests were added in the process.

Previous scope:

This PR refactors the code in a way it's more testable and adds tests for uncovered parts. One big change is the getConfig function that is solely concerned about how configuration is done. This is the place where we will add config validation in the future.

The coverage went down because of this PR but in reality it should have been increased. The runAll function has least coverage right now and this is where I want to spend some time after this is merged.

codecov · 2017-03-17T17:27:04Z

Codecov Report

Merging #141 into master will decrease coverage by 10.55%.
The diff coverage is 85.58%.

@@             Coverage Diff             @@
##           master     #141       +/-   ##
===========================================
- Coverage     100%   89.44%   -10.56%     
===========================================
  Files           5       10        +5     
  Lines          65      161       +96     
  Branches        8       23       +15     
===========================================
+ Hits           65      144       +79     
- Misses          0       16       +16     
- Partials        0        1        +1

Impacted Files	Coverage Δ
src/printErrors.js	`100% <100%> (ø)`
src/runScript.js	`100% <100%> (ø)`	⬆️
src/generateTasks.js	`100% <100%> (ø)`	⬆️
testSetup.js	`100% <100%> (ø)`
src/resolveGitDir.js	`100% <100%> (ø)`
src/index.js	`64% <61.9%> (ø)`
src/runAll.js	`73.07% <73.07%> (ø)`
src/getConfig.js	`97.14% <97.14%> (ø)`
... and 4 more

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 5e3c04d...5b35048. Read the comment docs.

…tests

… fix some bugs in tests

luftywiranda13 · 2017-08-30T12:42:45Z

src/generateTasks.js


 module.exports = function generateTasks(config, files) {
-  const linters = config.linters !== undefined ? config.linters : config
-  const resolve = file => files[file]
+  const { linters, gitDir, globOptions } = getConfig(config) // Ensure we have a normalized config


this is incompatible with node 4, right?

I mean, the import syntax

Correct, destructuring is not supported in node 4.

Oh that sucks. I will need to update eslint node plugin settings...

I don't see an engines field in package.json.

Yeah, I was under impression I already have it in this project but I don't. See #243

luftywiranda13 · 2017-09-01T17:00:54Z

src/getConfig.js

+   */
+  if (isGlob(option)) {
+    const message = `
+  Unknown option ${chalk.bold(`"${option}"`)} with value ${chalk.bold(


IMO, "Unknown filter..." is better, to match https://github.com/okonet/lint-staged#filtering-files

okonet · 2017-09-01T17:04:50Z

src/getConfig.js

+   * a typical mistake of mixing simple and advanced configs
+   */
+  if (isGlob(option)) {
+    const message = `


This new line should be removed

sudo-suhas

Other than a couple of edge cases for tests, LGTM

sudo-suhas · 2017-09-02T11:33:22Z

src/getConfig.js

 const intersection = require('lodash/intersection')
 const defaultsDeep = require('lodash/defaultsDeep')
 const isObject = require('lodash/isObject')
+const validate = require('jest-validate').validate
+const unknownOptionWarning = require('jest-validate/build/warnings').unknownOptionWarning
+const logValidationWarning = require('jest-validate/build/utils').logValidationWarning


You can change this to const logValidationWarning = require('jest-validate').logValidationWarning

sudo-suhas · 2017-09-02T11:38:33Z

src/getConfig.js

@@ -23,6 +29,13 @@ const defaultConfig = {
  verbose: false
 }

+const exampleConfig = Object.assign({}, defaultConfig, {


This one is very minor and optional(obviously).

Can be changed to:

const exampleConfig = Object.assign( { linters: { '*.js': ['eslint --fix', 'git add'], '*.css': 'stylelint' } }, defaultConfig )

sudo-suhas · 2017-09-02T11:39:44Z

src/getConfig.js

+
+    const comment = options.comment
+    const name = (options.title && options.title.warning) || 'WARNING'
+    logValidationWarning(name, message, comment)


Should there be a return after this? I haven't used jest-validate before so not too sure about this.

Yes! Added. Thanks!

sudo-suhas · 2017-09-02T11:46:19Z

test/getConfig.spec.js

+      renderer: 'custom',
+      verbose: true
+    }
+    expect(getConfig(src)).toEqual(src)


If src was being mutated by the getConfig, this test would still pass. How about we use lodash#cloneDeep to pass a copy of src to getConfig?

You're right. Nice catch!

sudo-suhas · 2017-09-02T11:49:34Z

test/getConfig.spec.js

+    expect(console.printHistory()).toMatchSnapshot()
+  })
+
+  it('should not throw and print nothing for valid config', () => {


A small gripe but this test title is ambiguous. Should it not throw and not print nothing? should not throw and should print nothing for valid config is clearer.

sudo-suhas · 2017-09-02T11:50:15Z

test/getConfig.spec.js

+      '*.js': ['eslint --fix', 'git add']
+    }
+    expect(() => validateConfig(getConfig(validSimpleConfig))).not.toThrow()
+    expect(console.printHistory()).toMatchSnapshot()


Could we add a validate for advanced config?

sudo-suhas · 2017-09-02T12:15:08Z

src/getConfig.js

+   * a typical mistake of mixing simple and advanced configs
+   */
+  if (isGlob(option)) {
+    const message = `  Unknown option ${chalk.bold(`"${option}"`)} with value ${chalk.bold(


Realised this would not be correct, ignore.

This fails the snapshot right now but could we change this to the following:

const formattedOption = format(config[option], { inlineCharacterLimit: Number.POSITIVE_INFINITY }) const message = ` Unknown option ${chalk.bold(`"${option}"`)} with value ${chalk.bold( formattedOption )} was found in the config root. You probably trying to mix simple and advanced config formats. Adding ${chalk.bold(`"linters": { "${option}": ${formattedOption} }`)} will fix it and remove this message.`

Also, You probably trying ➡ You are probably trying.

Edit: Just realised, the message printed is not valid JSON. Should be changed to the following:

const message = ` Unknown option ${chalk.bold(`"${option}"`)} with value ${chalk.bold( format(config[option], { inlineCharacterLimit: Number.POSITIVE_INFINITY }) )} was found in the config root. You probably trying to mix simple and advanced config formats. Adding ${chalk.bold( `"linters": ${JSON.stringify({ [option]: config[option] })}` )}) will fix it and remove this message.`

sudo-suhas · 2017-09-04T06:58:56Z

src/getConfig.js


  ${chalk.bold(`"linters": {
-    "${option}": ${format(config[option], { inlineCharacterLimit: Number.POSITIVE_INFINITY }, '  ')}
+    "${option}": ${JSON.stringify(config[option], null, '\t    ')}


This results in inconsistent indentation. Massaged snapshot equivalent:

"linters": { "*.js": [ "eslint --fix", "git add" ]

Yeah, I've noticed that. Do you know how to fix that?

Don't add the indentation while formatting using JSON.stringify. This will print the commands on a single line but that's okay I think.

Hmm, okay, I guess that can be improved later on

sudo-suhas · 2017-09-04T07:00:49Z

src/getConfig.js

+    logValidationWarning(name, message, comment)
+  }
+  // If it is not glob pattern, when use default jest-validate reporter
+  return unknownOptionWarning


unknownOptionWarning is a function. So should we be invoking it here instead of returning it?

You're right! I thought I've updated that already :-/

Mentioned this before but a return after logValidationWarning is also required.

See 0d210f4

okonet · 2017-09-04T08:47:16Z

@luftywiranda13 @sudo-suhas I've updated the branch. I'd like to merge it today. Thanks for reviews. That was very valuable feedback. Final "approve", please?

sudo-suhas

LGTM

sudo-suhas · 2017-09-04T08:52:38Z

test/getConfig.spec.js

+  })
+
+  it('should not throw and should print nothing for advanced valid config', () => {
+    const validSimpleConfig = {


Could you rename this to validAdvancedConfig.

sudo-suhas · 2017-09-04T08:54:56Z

test/getConfig.spec.js

+    expect(() => validateConfig(getConfig(invalidAdvancedConfig))).toThrowErrorMatchingSnapshot()
+  })
+
+  it('should not throw and print validation warnings for mixed config', () => {


Change title similarly - should not throw and should print validation warnings for mixed config

sudo-suhas

Looks super great to me 😁

okonet · 2017-09-04T10:08:05Z

🍾

luftywiranda13 · 2017-09-04T10:44:51Z

@okonet @sudo-suhas
great! sorry i've been quiet lately. we're currently have Eid al-Adha in Indonesia (islamic celebration)

sudo-suhas · 2017-09-04T11:50:59Z

@luftywiranda13 No need to apologise. We are doing this in whatever time we can find. I am sure you'll fill in for us when needed.

okonet · 2017-09-04T12:45:04Z

@luftywiranda13 I'm doubling it: no need to apologize! Thanks for all you help so far and I'm looking forward for more good stuff coming from you! Enjoy your time!

okonet added 9 commits May 11, 2017 16:26

tests: Update eslintrc to not fail on Jest

4512209

tests: Add resolveGitDir and tests and refactor generateTasks

5dd8f0e

tests: Introduce getConfig + tests

2c5c2c1

tests: Refactor index.js in a way we can test the output

4a3c6a4

tests: Do not use async/await

e0833e2

tests: Simplify and fix the implementation of getConfig

618636f

Refactor code to use getConfig

e000ea1

Update runAll and tests

ded405b

tests: Updated Jest snapshots

4c5a18b

okonet force-pushed the more-tests branch from 8130668 to 4c5a18b Compare May 11, 2017 14:27

This was referenced May 11, 2017

Run linters with configurable concurrency #149

Merged

Add logLevel config and log verbose task output after task has finished #168

Closed

okonet and others added 10 commits June 9, 2017 15:43

tests: Serialize stdout into string instead of Array

de55135

Merge branch 'master' into more-tests

00e4e28

Reformat code

33b0d7c

Fix merge mistake

5a6e2a4

Update snapshot

33bc21c

Fix and skip some tests

483edfd

tests: Fixed weird tests bug when config.gitRoot was affecting other …

596c041

…tests

Removed .only from test suite

de0dc17

fixup! Removed .only from test suite

b41c826

Deeply merge config

7df4b72

okonet mentioned this pull request Aug 30, 2017

style: Use dedent for prettier multi-line template literals #242

Merged

okonet requested a review from luftywiranda13 August 30, 2017 07:46

Andrey Okonetchnikov added 3 commits August 30, 2017 10:48

Add chunkSize and subTaskConcurrency properties to default config and…

9b60a42

… fix some bugs in tests

Remove readConfigOption and use getConfig instead

9b80589

Use stringify-object when printing config in verbos mode.

226f03c

okonet force-pushed the more-tests branch from 5ca4417 to 226f03c Compare August 30, 2017 11:48

luftywiranda13 reviewed Aug 30, 2017

View reviewed changes

okonet self-assigned this Sep 1, 2017

okonet added the enhancement label Sep 1, 2017

luftywiranda13 requested changes Sep 1, 2017

View reviewed changes

okonet commented Sep 1, 2017

View reviewed changes

fix: Remove new line in the vaslidation reporter

07274f5

sudo-suhas requested changes Sep 2, 2017

View reviewed changes

sudo-suhas reviewed Sep 2, 2017

View reviewed changes

fix: Print valid JSON in the warning message. Fix grammar.

18cc38b

sudo-suhas reviewed Sep 4, 2017

View reviewed changes

Andrey Okonetchnikov added 4 commits September 4, 2017 10:35

fix: Fix indentation and return after glob warning

0d210f4

tests: Add test for valid advanced config

d8f4be6

tests: Better test for immutability

6ae9fb7

style: Better require

8bae467

sudo-suhas previously approved these changes Sep 4, 2017

View reviewed changes

tests: Update test titles

5b35048

okonet dismissed sudo-suhas’s stale review via 5b35048 September 4, 2017 09:06

sudo-suhas approved these changes Sep 4, 2017

View reviewed changes

okonet merged commit d99eb38 into master Sep 4, 2017

okonet deleted the more-tests branch September 4, 2017 10:07

This was referenced Sep 4, 2017

config should distinct files and options #210

Closed

Error: spawn git ENOENT in events.js:160 #240

Closed

This was referenced Oct 19, 2017

Update lint-staged from 3.6.1 to 4.3.0 opencomponents/base-templates#98

Closed

Update lint-staged from 3.6.1 to 4.3.0 opencomponents/base-templates#121

Closed

Update lint-staged from 3.6.1 to 4.3.0 opencomponents/base-templates#128

Merged

sudo-suhas mentioned this pull request Dec 25, 2017

chore(package): upgrade to jest 22 #367

Merged

Feature: Config validation #141

Feature: Config validation #141

Conversation

okonet commented Mar 17, 2017 • edited Loading

Why

Previous scope:

codecov bot commented Mar 17, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudo-suhas Aug 30, 2017 • edited Loading

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

sudo-suhas left a 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

okonet Sep 4, 2017 • edited Loading

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

sudo-suhas Sep 2, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okonet commented Sep 4, 2017

sudo-suhas left a 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

sudo-suhas left a comment

Choose a reason for hiding this comment

okonet commented Sep 4, 2017

luftywiranda13 commented Sep 4, 2017

sudo-suhas commented Sep 4, 2017

okonet commented Sep 4, 2017

okonet commented Mar 17, 2017 •

edited

Loading

codecov bot commented Mar 17, 2017 •

edited

Loading

sudo-suhas Aug 30, 2017 •

edited

Loading

okonet Sep 4, 2017 •

edited

Loading

sudo-suhas Sep 2, 2017 •

edited

Loading