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

Set expected number of assertions #257

Closed
Marsup opened this issue Oct 22, 2014 · 31 comments
Closed

Set expected number of assertions #257

Marsup opened this issue Oct 22, 2014 · 31 comments
Assignees
Labels
Milestone

Comments

@Marsup
Copy link
Member

@Marsup Marsup commented Oct 22, 2014

Related to #115.

Now that we can count assertions, it would be very useful to assert all the expected assertions ran.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 22, 2014

It will do this if you pass -a code. In lab though, since we have https://github.com/hapijs/lab/blob/master/test/runner.js#L475 using it for lab itself is a bit of a problem

@geek geek added the non issue label Oct 22, 2014
@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 22, 2014

An example of the output on missing an assertion from running is:

Incomplete assertion at /lab/test/runner.js:466.28
@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 22, 2014

I don't see how that's related. That doesn't cover missing assertions hidden in some asynchronous callback never called. My request is to have something like tap module's plan and explicitly tell : this test will run exactly 5 assertions, if it does more or less, that's a failure.

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 28, 2014

@geek Can we reopen this please ?

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 28, 2014

It will also tell you the number of assertions that were executed in verbose mode: https://github.com/hapijs/lab/blob/master/lib/reporters/console.js#L207 Were you expecting a verbose output on a per test basis instead of per script?

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 28, 2014

I'm expecting something like :

it('test', { assert: 4 }, (done) => {
  expect(true).to.be.true();
  done();
});

To produce an error test was supposed to make 4 assertions and only made 1.
Whatever the API, that's the kind of functionality I'd like.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 28, 2014

I like the current implementation better... it will tell you the exact line where the assertion wasn't made as well as giving you a total count for assertions across all tests. What is the end goal with what you want... is it to know when an assertion wasn't called or to know how many assertions were executed for each test case?

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 28, 2014

To know when an asserts wasn't called. The current implementation is weak because it only works when you actually reach your assertion. As I understand it, the only way to trigger an error is by forgiving parenthesis, which is far from what I'm asking.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Oct 28, 2014

You are basically asking for coverage for the tests through assertion count.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 28, 2014

Opening the issue, the proposed interface makes sense here: #257 (comment)

@geek geek reopened this Oct 28, 2014
@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 28, 2014

That's some kind of coverage yes, but getting over the assertion count might mean you trigger an event one too many times which is a bit different from coverage. Anyway now you get the idea :)

@geek geek added request and removed non issue labels Oct 29, 2014
@geek geek removed their assignment Nov 5, 2014
@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Nov 17, 2014

@Marsup just so you know, I am happy to accept this as a PR if you want it added. Otherwise, it may take a few days for me to get to it.

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Nov 17, 2014

I don't have time for this currently. Is @hueniverse on board ? code will have to change as well to support this.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Nov 19, 2014

What do you think the API looks like?

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Nov 19, 2014

I think the only technically possible way is to create a context for the tests to attach expectations. Did that a while ago in Marsup/lab@a1b1b3f. Maybe you'll find a smarter way but I don't.

@aymericbeaumet

This comment has been minimized.

Copy link

@aymericbeaumet aymericbeaumet commented Oct 5, 2015

What's the current status about this? What about Marsup@a1b1b3f?

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 5, 2015

My patch was working when lab was explicitly using chai, it would need an update to work with various assertion framework now. I'd also be curious to explore a solution based on continuation-local-storage.

@aymericbeaumet

This comment has been minimized.

Copy link

@aymericbeaumet aymericbeaumet commented Oct 5, 2015

Hum yeah, not sure how to tackle the assertion-framework-agnostic issue here. Maybe something like this: var expect = Lab.assert(require('chai')).expect;. But that would imply to explicitly support a list of assertion libraries.

How would you would leverage continuation-local-storage?

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 5, 2015

There's already the -a switch, we could use that and add hooks when we know the library.
CLS could be used as a context to store assertion counts to avoid using the this.

@aymericbeaumet

This comment has been minimized.

Copy link

@aymericbeaumet aymericbeaumet commented Oct 5, 2015

Yeah I suppose we could support the main libraries at first. -a is fine, but we should also expose an API for people loading their assertion library manually.
CLS > Fine by me. In that case we would need a way to generate unique string ids for each test (unless that's already done by lab).

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 5, 2015

Unless I misunderstood CLS, it doesn't have to be unique. I'd pick a single one here.

@aymericbeaumet

This comment has been minimized.

Copy link

@aymericbeaumet aymericbeaumet commented Oct 5, 2015

Unless I misunderstood CLS

The misunderstanding was on my side. I re-read the doc and played with it a bit: a unique id is enough.

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 7, 2015

@geek time to take a decision, should anyone take action or shall we close the issue ?

@devinivy

This comment has been minimized.

Copy link
Member

@devinivy devinivy commented Oct 7, 2015

Just adding that I would use this feature if the API for it were simple enough.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 8, 2015

@Marsup I like the idea and will gladly merge it in :)

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 8, 2015

My vision of the API is :

it('works', { plan: 4 }, (done) => {
  4x expect(...)
});

So no this if possible.

I'm assuming CLS but maybe the future is zones.js or async-listener, it's still not clear to me, maybe someone followed that better than me ?
Also only libraries using that would work of course.

@aymericbeaumet

This comment has been minimized.

Copy link

@aymericbeaumet aymericbeaumet commented Oct 8, 2015

👍

Note that we might not require a done parameter if the user specifies options.plan.

@Marsup

This comment has been minimized.

Copy link
Member Author

@Marsup Marsup commented Oct 8, 2015

I'd keep it anyway for consistency.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 8, 2015

Looks good to me. I am fine with naming it plan, might consider naming it expect instead.

it('works', { expect: 4 }, (done) => {
  4x expect(...)
});
@devinivy

This comment has been minimized.

Copy link
Member

@devinivy devinivy commented Oct 8, 2015

I like plan, since that's possibly more familiar due to popularization in tap. Either way looks good, though.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Oct 8, 2015

@devinivy sounds good, I know thats what tape names it... so should be easier for people who are familiar with that style.

@geek geek added feature and removed request labels Mar 1, 2016
@geek geek added this to the 10.1.1 milestone Mar 1, 2016
@geek geek self-assigned this Mar 1, 2016
@geek geek closed this in #538 Mar 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.