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

Use chai in our tests #5894

Closed
epixa opened this issue Jan 13, 2016 · 14 comments
Closed

Use chai in our tests #5894

epixa opened this issue Jan 13, 2016 · 14 comments
Labels
dev Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@epixa
Copy link
Contributor

epixa commented Jan 13, 2016

I propose that we start using chai with mocha, which lets us use chai.except and sinon-chai for more flexible and expressive test errors.

Better errors:

expect(myspy).to.have.been.called; // "Expected {...} to have been called, but wasn't"
// instead of
expect(myspy.called).to.equal(true); // Expected false to equal true

Custom Plugins:

// sort of uses chai-as-promised, otherwise totally made up, but you could make this happen
expect(mypromise).to.eventually.be.rejected.with(Boom.notFound());

Many plugins available to us right away: http://chaijs.com/plugins

It's really setup, and I'm happy to do it.

@epixa epixa added the discuss label Jan 13, 2016
@kimjoar
Copy link
Contributor

kimjoar commented Jan 13, 2016

The one thing I hate about chai is this:

expect('test').to.be.undefied;

Nooothing happens and no errors because of a typo (undefied instead of undefined). Such pain. It also looks so un-JavaScript-y with all the .to.have.been.something.with.somethingelse.wtf, but that might just be me 😒 (I'll go rant somewhere else 💯)

Otherwise, totally agree on better error messages. It's very important when working with large amount of tests. I hate debugging for half an hour just because the error message was unclear. This is also why it's so important to actually see the test failing when you write it.

@epixa
Copy link
Contributor Author

epixa commented Jan 13, 2016

So what you're saying is... we need tools that allow for static analysis to reliably identify errors like typos. Sounds like another vote for es6 modules :P

@kimjoar
Copy link
Contributor

kimjoar commented Jan 13, 2016

That's just throwing more complexity at the problem ;) I'm thinking about something like this:

expect('test').to.be.undefined()

or, maybe even better with one of these:

expect('test').to(beUndefined)
expect('test').to(beUndefined())
expect('test').toBeUndefined()

Or something like that.

@epixa
Copy link
Contributor Author

epixa commented Jan 13, 2016

Understood. We could probably throw together that sort of behavior in a pretty simple plugin that adds method helpers instead of the default property helpers.

@kimjoar
Copy link
Contributor

kimjoar commented Jan 13, 2016

Oh, I forgot:

expect('test').toBe(undefined);

where toBe is just a === check. That would also catch the typo.

Some months ago I got so annoyed that I created my own https://github.com/kjbekkelund/expect-to ;) (But I'm absolutely not trying to "sell" this to Kibana — this was mostly to play around with ideas on how I would like a testing lib to work). Personally I might lean in the direction of https://www.npmjs.com/package/expect, but I absolutely realize the value in having chai with it's huge community. (So just consider just random ranting about testing and assertion frameworks in general 💯 Haven't found one I really liked yet.)

@spalger
Copy link
Contributor

spalger commented Jan 13, 2016

I've started using sinon's asserts when I'm verifying sinon stubs, mostly because they provide more readable error messages:

sinon.assert.calledOnce(obj.method);
// vs
expect(obj.method).to.have.propety('callCount', 1);
expect(obj.method.callCount).to.be(1);

@Bargs
Copy link
Contributor

Bargs commented Jan 13, 2016

👍 Chai provides an assertion for deep equality and supports promises via chai-as-promised.

@simianhacker
Copy link
Member

I'm +1 on chai... I've been using it for years (before I started working on Kibana) and I still use when I'm not working on this code base.

@bevacqua
Copy link
Contributor

bevacqua commented Jul 5, 2016

Guys why not use assert? BDD is the worst.

@scampi
Copy link
Contributor

scampi commented Aug 16, 2016

+1 on chai, also expectjs seems not to be maintained anymore (or very little...) Automattic/expect.js#100 Automattic/expect.js#151

@scampi
Copy link
Contributor

scampi commented Oct 4, 2016

Something I found out today: if you expect a method to throw an error, and the expectation is a string, then it will always pass:

expect(fn).to.throwError('correct message'); // pass
expect(fn).to.throwError('funky message'); // pass but shouldn't

However, if you use a regexp expression, it will assert as expected:

expect(fn).to.throwError(/correct message/); // pass
expect(fn).to.throwError(/funky message/); // fail

@epixa epixa added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Nov 29, 2016
@cjcenizal
Copy link
Contributor

I'm +100 for moving away from Expect.js, but I've found the Jasmine interface to be much simpler and easier to read than Mocha and Chai (https://jasmine.github.io/2.5/introduction).

Per @kjbekkelund 's earlier comment about asserting that something is undefined or making an assertion of strict equality, Jasmine offers both:

expect(a.bar).toBeUndefined();
expect(true).toBe(true); // strict equality
expect(1).toEqual(true); // loose equality

The interface is simpler, mostly by eliminating the pointless to object, and instead expecting you to either assert something by calling a method, or invert the assertion by prepending the method with the not property:

expect(foo).toThrowError("foo bar baz");
expect(foo).not.toThrowError("foo bar baz");

The interface also offers a rich number of assertions for increasing the readability of your assertions:

expect(0).toBeLessThan(1);
expect(1).toBeGreaterThan(0);
expect("bar room").toContain("bar");
expect({}).toBeTruthy();
// etc...

As a test runner, Jasmine simplified the interface to describe and it (no more context, which I find pretty confusing/meaningless since it's just an alias to describe). There are setup and teardown methods of course.

As another benefit, you can use fdescribe to focus on any suites you like, even if they're nested or in separate files. You can also use xit to temporarily disable a test.

@Bargs Bargs mentioned this issue Jan 19, 2017
13 tasks
@epixa epixa removed the P2 label Apr 25, 2017
@timroes
Copy link
Contributor

timroes commented Feb 9, 2018

I think our current decision is to use Jest for testing. Can this issue be closed then, or do we discuss if we want to introduce another expect library inside Jest?

@epixa
Copy link
Contributor Author

epixa commented Feb 9, 2018

@timroes If you think an issue should be closed, go ahead and close it along with your rationale. It can always be reopened if necessary.

You're right, we want to move to jest, so this isn't necessary any longer.

@epixa epixa closed this as completed Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

9 participants