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

Enhancement: Added validation to `include` #84

Merged
merged 4 commits into from Aug 28, 2016

Conversation

@blacksun1
Copy link
Contributor

blacksun1 commented Aug 23, 2016

Added an extra assertion to check that the include expectation is always
called with just one argument.

Fixes: #83

Added an extra assertion to check that the include expectation is always
called with just one argument.

Fixes: #83
@@ -2297,7 +2320,8 @@ describe('incomplete()', () => {

const a = Code.expect(1).to;
Code.expect(2).to.equal(2);
Hoek.assert(Code.incomplete().length === 1);
const actual = Code.incomplete();
Hoek.assert(actual.length === 1, actual);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Aug 24, 2016

Contributor

I'm not sure that just showing actual is any more meaningful.

This comment has been minimized.

Copy link
@blacksun1

blacksun1 Aug 24, 2016

Author Contributor

Sorry, didn't mean to make this change. I'll revert it.


it('asserts called with only one argument', (done) => {

let exception0 = false;

This comment has been minimized.

Copy link
@cjihrig

cjihrig Aug 24, 2016

Contributor

Instead of exception0, exception2, exception4, etc., can you do:

{
  let exception = false;
  try {
    ...
  }
}

i.e., separate the subtests with block scopes.

This comment has been minimized.

Copy link
@blacksun1

blacksun1 Aug 24, 2016

Author Contributor

Ordinarily I would have written something like

for ([const test of {name: "no", test: [], {name: "two", test: ['a', 'b']}, {name: "three", test: ['a', 'b', 'c']}]) {

  it('assertion fails when called with ${test.name} arguments', (done) => {

   let exception = false;
   try {
    Code.expect('abc').to.include.apply(to, test.arguments);
   }
   catch (err) {
    exception = err;
   }

   Hoek.assert(exception0.message === 'Can only assert include with a single parameter', exception);
   done();
  }
}

but I wasn't quite sure if this was to your style or not. Up to you. Let me know either way and I'll change it.

Fix as per [Pull request 84](#84)
@@ -1124,6 +1124,52 @@ describe('expect()', () => {
Hoek.assert(!exception, exception);
done();
});

const tests = [

This comment has been minimized.

Copy link
@cjihrig

cjihrig Aug 25, 2016

Contributor

This isn't what I meant. One it() block. No tests array. No loop. You can just use curly braces to define a scope and do a simple test inside of it so you can reuse the same variable names. See https://github.com/nodejs/node/blob/4a408321d9c4a6964c9d89a0dd09067f36b4dbfc/test/parallel/test-vm-debug-context.js#L34-L72 as an example.

Second refactor to fix tests as per
[Pull request 84](#84)
Hoek.assert(exception.message === 'Can only assert include with a single parameter', exception);
}

{

This comment has been minimized.

Copy link
@cjihrig

cjihrig Aug 27, 2016

Contributor

I think this second test can be dropped, as the functionality is tested elsewhere.

This comment has been minimized.

Copy link
@blacksun1

blacksun1 Aug 27, 2016

Author Contributor

@cjihrig Done.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Aug 27, 2016

One last comment, then this is good to go.

> I think this second test can be dropped, as the functionality is
> tested elsewhere.
> --cjihrig
@cjihrig cjihrig added the feature label Aug 28, 2016
@cjihrig cjihrig added this to the 4.0.0 milestone Aug 28, 2016
@cjihrig cjihrig self-assigned this Aug 28, 2016
@cjihrig cjihrig merged commit 4978bdd into hapijs:master Aug 28, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cjihrig cjihrig mentioned this pull request Sep 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.