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

Improved tests for default values #5

Closed
wants to merge 6 commits into from
Closed

Improved tests for default values #5

wants to merge 6 commits into from

Conversation

nicwaller
Copy link

This PR improves on the tests for default values. This is a superset of the changes in #4. Additions include:

  • Split test cases into smaller chunks with more descriptive names
  • Confirm that onRecommend still triggers even if a default is available
  • Test to ensure error output if there are conflicting validation rules (required & default)

Nic Waller added 6 commits August 23, 2015 17:22
Otherwise, only one test suite could check validationErrors.
Because all test suite definitions are run before any of the test
cases start, so if env.onError was set in more than one suite, each
suite would replace the previous suite's handler.

This seems better than putting a require() in each test suite.
At first it seems like we could test for validationErrors.length == 0
in cases where input passes all the tests. But that's not quite correct,
because validationErrors is only populated as a side-effect when the
onError handler is called. It never exists if there are no errors.

Instead, we actually need to test whether the onError handler is called.
So we clear a boolean before each test, then enable it from onError.

Of course, this required some of the test cases to be split up, which
also led to slightly clearer naming.

This edge case is unique to asynchronous programming because a normal
function will either return or throw an exception. It probably would
have been cleaner to use a node-style callback that is called exactly
once and has the signature function(error, result).
@af
Copy link
Owner

af commented Aug 24, 2015

Rebased and merged this from the CLI. Thanks!

@af af closed this Aug 24, 2015
@nicwaller nicwaller deleted the more-tests branch August 24, 2015 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants