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

Handle expect(0) as expected #158

Merged
merged 2 commits into from
Oct 10, 2011
Merged

Handle expect(0) as expected #158

merged 2 commits into from
Oct 10, 2011

Conversation

mmchaney
Copy link
Contributor

I think that if something like the following fails

test("foo", function() {
    expect(1); ok(true, "foo"); ok(true, "bar");
});

then the following should fail too

test("foo", function() {
    expect(0); ok(true, "foo");
});

@@ -122,7 +122,7 @@ Test.prototype = {
}
},
finish: function() {
if ( this.expected && this.expected != this.assertions.length ) {
if (this.expected !== undefined && this.expected !== null && this.expected != this.assertions.length ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do this.expected != null to simplify this. Also please keep the whitespace intact (missing space after opening paren).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and done. Thanks for the heads up.

@Krinkle
Copy link
Member

Krinkle commented Sep 29, 2011

We should also add unit tests for these two cases:

  • expect(0) with >= 1 assertion should fail
  • expect(0) with 0 assertions should succeed.

@mmchaney
Copy link
Contributor Author

mmchaney commented Oct 1, 2011

Writing the first test is easy but I don't think it would be good to have a test case that purposely fails. Actually we should test Test.prototype.finish to see if it adds a negative assertion for > 0 assertions and 0 expected. Yet I don't see how that's possible without mocking a bunch of internal methods and variables. Maybe I'm just missing something...

The second case would also pass without my changes.

Maybe the following argument will suffice to pull in these changes:

QUnit.test checks if expected is supplied and if not it sets it to null and not 0. So clearly there should be a difference between expected being 0 and expected being null. But the current check in QUnit.prototype.finish does not account for that (due to 0 being falsy). I think it's just an oversight that should not require a test case.

@jzaefferer
Copy link
Member

The suggested change to have null mean something else then 0 sounds good.

@jzaefferer jzaefferer merged commit 6f1f26c into qunitjs:master Oct 10, 2011
@jzaefferer
Copy link
Member

Landed!

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

Successfully merging this pull request may close these issues.

3 participants