jquery / 2012-dev-summit

Information regarding the 2012 Developer Summit in DC
14 stars 14 forks source link

Ensure all tests have an expect() or the expect argument. #53

Closed mikesherov closed 12 years ago

mikesherov commented 12 years ago

So, a Qunit test can be written like this:

test("Unit Testing Environment", function () {
    expect(1);
    ok( true, "message );
});

but it can also be written like this:

test("Unit Testing Environment", 1, function () {
    ok( true, "message );
});

All tests that don't have a logically determined number of expected tests should use the second signature.

rwaldron commented 12 years ago

Would be nice, but some tests have generated assertions, so the expect isn't known until execution enters the test callback.. Not sure how often that happens, maybe those tests could be rewritten

dmethvin commented 12 years ago

Right, I think that was Mike's "logically determined" point. The fact that you don't need either a test count argument or an expect() is a bug IMO.

scottgonzalez commented 12 years ago

Maybe a bug in core :-P

https://github.com/jquery/jquery-ui/blob/master/tests/unit/testsuite.js#L24

rwaldron commented 12 years ago

I was conversationally adding details for whoever reads this ;-)

Maybe we should shim a temporary "expect detect" path to identify missing "expects"

rwaldron commented 12 years ago

Ugh, was typing that on mobile... Please ignore. I'd forgotten about the requireExpects option

dmethvin commented 12 years ago

I'd just never gone looking for it. Let's reject the heck out of tests with no expect.

jzaefferer commented 12 years ago

@rwldrn look at what Scott posted, QUnit already has a feature.

As for the test argument vs expect, I strongly prefer calling expect all the time. I probably should've just removed the expect argument to test a long time ago.

jzaefferer commented 12 years ago

Btw. requireExpects is "now" documented here: http://api.qunitjs.com/QUnit.config/ - maybe there's something else people haven't been aware of?

rwaldron commented 12 years ago

On Oct 14, 2012 3:55 PM, "Jörn Zaefferer" notifications@github.com wrote:

@rwldrn look at what Scott posted, QUnit already has a feature.

I think the lines are crossed.

As for the test argument vs expect, I strongly prefer calling expect all the time. I probably should've just removed the expect argument to test a long time ago.

— Reply to this email directly or view it on GitHub.

rwaldron commented 12 years ago

From a DRY perspective, I agree with Mike, but the benefit of an explicit expect() call is complete clarity of intention

mikesherov commented 12 years ago

@gnarf37 has already been writing his tests with the 3 arguments version, most other tests call expect in the body. I don't care either way as long as we're consistent.

gnarf commented 12 years ago

I started copying someone else's pattern - I could go for either.

rwaldron commented 12 years ago

James Huston is on this ticket @james-huston

rwaldron commented 12 years ago

https://github.com/jquery/jquery/commit/c2a6bad60a2b2d84081ef369a023f75c4bd32ad5

https://github.com/jquery/jquery/commit/57aa7977dd6d88898b5d1cf748a4f85c1e7a3bee

https://github.com/jquery/jquery/commit/6ac87167893e1de24761eaff47bbf9a20d0d6f98

https://github.com/jquery/jquery/commit/0c447434963bbdd6c631acfc38e51022db949a8b

rwaldron commented 12 years ago

https://github.com/jquery/jquery/commit/611d7660cd23747bcc7948ad2ee2a663e00ff105

rwaldron commented 12 years ago

Done! Thanks @james-huston