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: Config validation #141

Merged
merged 49 commits into from
Sep 4, 2017
Merged

Feature: Config validation #141

merged 49 commits into from
Sep 4, 2017

Conversation

okonet
Copy link
Collaborator

@okonet okonet commented Mar 17, 2017

This PR implements config validation 🎉

2017-09-01 at 15 23

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
Copy link

codecov bot commented Mar 17, 2017

Codecov Report

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

Impacted file tree graph

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


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
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is incompatible with node 4, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, the import syntax

Copy link
Collaborator

@sudo-suhas sudo-suhas Aug 30, 2017

Choose a reason for hiding this comment

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

Correct, destructuring is not supported in node 4.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@okonet okonet self-assigned this Sep 1, 2017
src/getConfig.js Outdated
*/
if (isGlob(option)) {
const message = `
Unknown option ${chalk.bold(`"${option}"`)} with value ${chalk.bold(
Copy link
Collaborator

Choose a reason for hiding this comment

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

src/getConfig.js Outdated
* a typical mistake of mixing simple and advanced configs
*/
if (isGlob(option)) {
const message = `
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This new line should be removed

Copy link
Collaborator

@sudo-suhas sudo-suhas left a comment

Choose a reason for hiding this comment

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

Other than a couple of edge cases for tests, LGTM

src/getConfig.js Outdated
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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

src/getConfig.js Outdated
@@ -23,6 +29,13 @@ const defaultConfig = {
verbose: false
}

const exampleConfig = Object.assign({}, defaultConfig, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
  )

src/getConfig.js Outdated

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@okonet okonet Sep 4, 2017

Choose a reason for hiding this comment

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

Yes! Added. Thanks!

renderer: 'custom',
verbose: true
}
expect(getConfig(src)).toEqual(src)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. Nice catch!

expect(console.printHistory()).toMatchSnapshot()
})

it('should not throw and print nothing for valid config', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

Could we add a validate for advanced config?

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

@sudo-suhas sudo-suhas Sep 2, 2017

Choose a reason for hiding this comment

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

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.`

src/getConfig.js Outdated

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

Choose a reason for hiding this comment

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

This results in inconsistent indentation. Massaged snapshot equivalent:

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

src/getConfig.js Outdated
logValidationWarning(name, message, comment)
}
// If it is not glob pattern, when use default jest-validate reporter
return unknownOptionWarning
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See 0d210f4

@okonet
Copy link
Collaborator Author

okonet commented Sep 4, 2017

@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
sudo-suhas previously approved these changes Sep 4, 2017
Copy link
Collaborator

@sudo-suhas sudo-suhas left a comment

Choose a reason for hiding this comment

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

LGTM

})

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

Choose a reason for hiding this comment

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

Could you rename this to validAdvancedConfig.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

expect(() => validateConfig(getConfig(invalidAdvancedConfig))).toThrowErrorMatchingSnapshot()
})

it('should not throw and print validation warnings for mixed config', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@sudo-suhas sudo-suhas left a comment

Choose a reason for hiding this comment

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

Looks super great to me 😁

@okonet okonet merged commit d99eb38 into master Sep 4, 2017
@okonet okonet deleted the more-tests branch September 4, 2017 10:07
@okonet
Copy link
Collaborator Author

okonet commented Sep 4, 2017

🍾

@luftywiranda13
Copy link
Collaborator

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

@sudo-suhas
Copy link
Collaborator

@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
Copy link
Collaborator Author

okonet commented Sep 4, 2017

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants