Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Breaking: Validate test options (refs eslint/eslint#2179) #21

Merged
merged 1 commit into from
Jun 15, 2015

Conversation

btmills
Copy link
Member

@btmills btmills commented Jun 4, 2015

This is the other part of enabling automatic validation of rule options. With this change, eslint-tester will begin validating options in test cases, a process which has already caught a few bugs.

@@ -48,7 +48,10 @@ var assert = require("chai").assert,
path = require("path"),
merge = require("lodash.merge"),
omit = require("lodash.omit"),
clone = require("lodash.clonedeep");
clone = require("lodash.clonedeep"),
validator = require("eslint/lib/config-validator"),
Copy link
Member

Choose a reason for hiding this comment

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

We should export configvalidator so we don't have to do it this way, but OK for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll push an update to both PRs to do it that way.

@nzakas
Copy link
Member

nzakas commented Jun 4, 2015

Looks good to me. What happens when a rule has no schema?

@ilyavolodin thoughts?

@btmills
Copy link
Member Author

btmills commented Jun 4, 2015

What happens when a rule has no schema?

If you look at config-validator line 25, it will provide a default schema which validates only that the first value is either 0, 1, or 2 and allows any other options.

@ilyavolodin
Copy link
Member

Looks good to me. My only concern is the order in which we need to merge this in between eslint-tester and eslint core.

@btmills
Copy link
Member Author

btmills commented Jun 4, 2015

Core then tester. Core will work without the changes in tester, but but test case options won't be validated, so we'll need to be vigilant and not let any changes to options sneak through until tester is updated.

@nzakas
Copy link
Member

nzakas commented Jun 4, 2015

So we need to do a new ESLint release before we can release the tester?

@btmills
Copy link
Member Author

btmills commented Jun 4, 2015

So we need to do a new ESLint release before we can release the tester?

Correct, unless you want me to do a two-phase update for tester that only conditionally tests validation in the first phase, then makes it always-on in the second?

@ilyavolodin
Copy link
Member

So we could merge this in, merge the one in core, and then make sure that we release eslint-tester right before we do next release for core? Would that work?

@btmills btmills changed the title Validate test options (refs eslint/eslint#2179) Breaking: Validate test options (refs eslint/eslint#2179) Jun 4, 2015
@btmills
Copy link
Member Author

btmills commented Jun 4, 2015

So I think, taking all constraints into account, it should look like this:

  1. Merge Breaking: Automatically validate rule options (fixes #2595) eslint#2685
  2. Merge Breaking: Validate test options (refs eslint/eslint#2179) #21
  3. Release core
  4. Release tester

If we consider ourselves at step 0 right now, tests in both core and tester are both passing. The change in tester depends on the change in core. Though the reverse is not true, if we don't catch them, changes to options could slip through in core that would break core's CI when tester is released. In theory nothing should break in between steps so there aren't technical time constraints, but minimizing the time between now and the end of step 4 would reduce the chance that something gets missed.

@nzakas
Copy link
Member

nzakas commented Jun 6, 2015

OK, 1 is done.

@btmills
Copy link
Member Author

btmills commented Jun 15, 2015

Just confirmed that eslint v0.23 is still working with this branch of eslint-tester. Are we good to merge this for a release?

@ilyavolodin
Copy link
Member

Yes, I think we should be good. Should I merge this in and do a release of eslint-tester?

@btmills
Copy link
Member Author

btmills commented Jun 15, 2015

Yep! 🚢

ilyavolodin added a commit that referenced this pull request Jun 15, 2015
Breaking: Validate test options (refs eslint/eslint#2179)
@ilyavolodin ilyavolodin merged commit b998879 into master Jun 15, 2015
@ilyavolodin ilyavolodin deleted the validate branch June 15, 2015 01:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants