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

Added fail() method to Should and Expect interfaces #356

Merged
merged 6 commits into from Feb 10, 2015

Conversation

Projects
None yet
3 participants
@Soviut

Soviut commented Jan 30, 2015

Should and Expect now have fail() methods like Assert does.

should.fail(0, 1, 'failure message');
expect().fail('failure message');

Please pay special attention to the Expect implementation as I wasn't totally certain how all of it worked. It may not be fully chainable, though it's not really intended to be.

@@ -21,7 +21,7 @@
},
"main": "./index",
"scripts": {
"test": "make test"
"test": "mocha --require ./test/bootstrap"

This comment has been minimized.

@keithamus

keithamus Jan 30, 2015

Member

This looks like an extraneous change. Could you rebase it out please?

}
Assertion.addMethod('fail', assertFail);

This comment has been minimized.

@keithamus

keithamus Jan 30, 2015

Member

Curious why you added it as a method for all assertions, rather than just tacking it onto expect. I was thinking you would just add expect.fail = function much like you did with should.fail

This comment has been minimized.

@Soviut

Soviut Jan 30, 2015

I was having issues adding fail directly to expect because it's a function. Are there any examples of how functions are directly added to expect anywhere else in the code base?

This comment has been minimized.

@Soviut

Soviut Feb 10, 2015

@keithamus Can you advise on how to add a fail method directly to expect? I can't find concrete examples and it seems somewhat impossible because expect is a function, unless I'm misunderstanding the architecture.

This comment has been minimized.

@keithamus

keithamus Feb 10, 2015

Member

Shouldn't matter that expect is a function. Fail can just be added as a property of it. See the example below:

> function x() {}
undefined
> x.fail = function () {}
[Function]
> x
{ [Function: x] fail: [Function] }
> x.fail
[Function]
>
@keithamus

This comment has been minimized.

Member

keithamus commented Jan 30, 2015

Some good work so far @Soviut. Couple of notes above which I'd like to see addressed - if you could be so kind. Also, as you've noted in the code, it'd be great to get some documentation put in here too 😄

if (msg) flag(this, 'message', msg);
var obj = flag(this, 'object');
throw new chai.AssertionError(msg, {}, obj);
}

This comment has been minimized.

@keithamus

keithamus Jan 30, 2015

Member

Another point to make here; for API consistency I think should.fail and expect.fail should work exactly the same as assert.fail. Method signature and all.

This comment has been minimized.

@Soviut

Soviut Jan 30, 2015

I agree with the consistency. I chose this route because I was trying to be consistent with the interface. Namely, that values are usually passed along the chain with expect. I'll try reworking it.

@Soviut Soviut force-pushed the Soviut:master branch from c8ba9ae to 61d8718 Jan 30, 2015

@Soviut

This comment has been minimized.

Soviut commented Feb 10, 2015

@keithamus There is now an expect.fail(0, 1, 'message'); method. Does this look good now?

@keithamus

This comment has been minimized.

Member

keithamus commented Feb 10, 2015

Looks fantastic @Soviut.

Could you add a little bit of documentation above each of the methods in the code? This way it'll get carried over onto the site documentation when the site is regenerated.

@Soviut

This comment has been minimized.

Soviut commented Feb 10, 2015

Will do.

@Soviut

This comment has been minimized.

Soviut commented Feb 10, 2015

@keithamus documentation added. I basically just copied what was already in the assert interface.

@keithamus

This comment has been minimized.

Member

keithamus commented Feb 10, 2015

Perfect. Consider it merged 👍

keithamus added a commit that referenced this pull request Feb 10, 2015

Merge pull request #356 from Soviut/master
Added fail() method to Should and Expect interfaces

@keithamus keithamus merged commit 9b25d0c into chaijs:master Feb 10, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@sieira

This comment has been minimized.

sieira commented Aug 22, 2015

Sorry for bothering you, but I can't manage to find the docs for this...

@keithamus

This comment has been minimized.

Member

keithamus commented Aug 23, 2015

@sieira the website is outdated right now. We're working on it. chaijs/chaijs.github.io#74

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency chai to v4 #30

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