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

Some reporters don't show reason for failed assertion #991

Closed
blalor opened this issue Oct 5, 2013 · 5 comments
Closed

Some reporters don't show reason for failed assertion #991

blalor opened this issue Oct 5, 2013 · 5 comments

Comments

@blalor
Copy link
Contributor

blalor commented Oct 5, 2013

The test below doesn't show the reason for the failed assertion when using the spec reporter, other than that the expectation didn't match. The tap reporter does show the reason.

tap output

1..1
not ok 1 reporter shows reason for failed assertion
  AssertionError: the foo: expected 'foo' to equal 'bar'
# tests 1
# pass 0
# fail 1

spec output

  reporter
    1) shows reason for failed assertion


  0 passing (8ms)
  1 failing

  1) reporter shows reason for failed assertion:

      actual expected

      "barfoo"

package.json

{
  "name": "mocha_reporter_bug",
  "version": "0.0.0",
  "description": "Reporter does not show reason for failed expectation",
  "devDependencies": {
    "chai": "~1.8.0",
    "mocha": "~1.13.0"
  }
}

test/aTest.js:

var expect = require("chai").expect;

describe("reporter", function() {
    it("shows reason for failed assertion", function() {
        expect("foo", "the foo").to.equal("bar");
    });
});
@hallas
Copy link

hallas commented Oct 6, 2013

I don't see how AssertionError: the foo: expected 'foo' to equal 'bar' is not the same as the string diff outputted by the spec reporter. It tells you exactly what you need to know.

In a case where you are simply asserting the equality of two strings, then you don't need to know more. It's obvious. The tap reporter has to output more data though because it's a protocol.

@hallas hallas closed this as completed Oct 6, 2013
@blalor
Copy link
Contributor Author

blalor commented Oct 6, 2013

What's missing is the 2nd argument provided to expect -- the message or context -- in this case "the foo". The full message of the AssertionError in the tap reporter tells me where in my test "ok" and "error" are unequal, whereas the spec reporter leaves that out. This makes it very difficult to determine where the test's failing, especially if you happen to have several places where you're comparing "ok" and "error".

I would like to see the error detail include this message. Perhaps something like this:

  1) reporter shows reason for failed assertion:

      actual expected for `the foo`:

      "barfoo"

@hallas
Copy link

hallas commented Oct 6, 2013

Alright. I see your point. It would good if you could provide a pull request for the change, then we'll take it from there :-)

@travisjeffery
Copy link
Contributor

The full message of the AssertionError in the tap reporter tells me where in my test "ok" and "error" are unequal, whereas the spec reporter leaves that out. This makes it very difficult to determine where the test's failing, especially if you happen to have several places where you're comparing "ok" and "error".

Why not look for spec's name? e.g. search for: "shows reason for failed assertion"

@blalor
Copy link
Contributor Author

blalor commented Oct 7, 2013

That's not sufficient. The spec name only tells me which test failed, not which assertion in the test failed. "ok" and "error" might not be hard to find, but when you're given the error "expected undefined not to be undefined" and you've used expect(someVar).not.to.be.undefined in multiple places, you're soon reduced to console.logging your way out of the problem. Instead (especially if you're being forward-thinking) you can do expect(someVar, "someVar").not.to.be.undefined and expect(anotherVar, "anotherVar").not.to.be.undefined which will return AssertionError: someVar: expected undefined not to be undefined and AssertionError: anotherVar: expected undefined not to be undefined respectively. The message is already there, and it's part of the AssertionError spec. Mocha has been throwing away the extra bit of the message, instead building its own comparison message based on the presence of actual and expected properties of the AssertionError instance. My pull request addresses this problem and gives the user all of the information they expect.

travisjeffery pushed a commit that referenced this issue Oct 7, 2013
Resolves #991

This expect statement

    expect("foo", "the foo").to.equal("bar");

now includes "the foo" as context for the failed match.

    1) reporter shows reason for failed assertion:

        the foo
        + expected - actual

        +"bar"
        -"foo"
travisjeffery pushed a commit that referenced this issue Oct 12, 2013
Resolves #991

This expect statement

    expect("foo", "the foo").to.equal("bar");

now includes "the foo" as context for the failed match.

    1) reporter shows reason for failed assertion:

        the foo
        + expected - actual

        +"bar"
        -"foo"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants