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

Inconsistent between API guide and message from assert.equal #790

Closed
sbidolach opened this issue Sep 9, 2016 · 2 comments · Fixed by #794
Closed

Inconsistent between API guide and message from assert.equal #790

sbidolach opened this issue Sep 9, 2016 · 2 comments · Fixed by #794

Comments

@sbidolach
Copy link

sbidolach commented Sep 9, 2016

Could you verify why API guide says something different than message from exception ?

Guide:

.equal(actual, expected, [message])

Example:

// test case
assert.equal(result[0].status, 405, 'Response HTTP status equal 405');
// message when assert.equal fire exception
AssertionError: Response HTTP status equal 405: expected 404 to equal 405
@meeber
Copy link
Contributor

meeber commented Sep 9, 2016

@sbidolach Looks like a bug to me.

it("expect equal with comment at start", () => {
  expect(42, "meep").to.equal(43);    // meep
});

it("expect equal with comment at end", () => {
  expect(42).to.equal(43, "meep");    // meep
});

it("assert strictEqual", () => {
  assert.strictEqual(42, 43, "meep"); // meep
});

it("assert equal", () => {
  assert.equal(42, 43, "meep");       // AssertionError: meep: expected 42 to equal 43
});

@meeber meeber added the bug label Sep 9, 2016
@lucasfcosta
Copy link
Member

lucasfcosta commented Sep 10, 2016

Hi @sbidolach, thanks for the issue and thanks @meeber for writing some examples! 😄
I think this problem involves not only Chai, but Mocha too, let me explain why:
When running tests like @meeber described we can really see this difference between the error messages displayed, but that is not due to Chai itself, since it always ends up concatenating the message flag value and the message passed to the assertmethod, as you can see here.

When running tests like this:

it("expect equal with comment at start", () => {
  err(function() {
    expect(42, "meep").to.equal(43);
  }, 'meep')
});

it("expect equal with comment at end", () => {
  err(function() {
    expect(42).to.equal(43, "meep");
  }, 'meep')
});

it("assert strictEqual", () => {
  assert.strictEqual(42, 43, "meep");
  err(function() {
    assert.strictEqual(42, 43, "meep");
  }, 'meep')
});

it("assert equal", () => {
  err(function() {
    assert.equal(42, 43, "meep");
  }, 'meep')
});

Every single one fails and that is because the message really isn't just meep, it is meep: <message_of_the_assert_method_here>.

What makes Mocha show just meep or meep: <message> is the showDiff flag, which, when set to true, shows only meep and the rest gets ignored because the diff is already being shown.

This happens because instead of calling the .equal assertion from the core, the assert.equal assertion works a little bit different, since it does a non-strict comparison (==). So all we have to do is change this:

  assert.equal = function (act, exp, msg) {
    var test = new Assertion(act, msg, assert.equal);

    test.assert(
        exp == flag(test, 'object')
      , 'expected #{this} to equal #{exp}'
      , 'expected #{this} to not equal #{act}'
      , exp
      , act
    );
  };

To this:

  assert.equal = function (act, exp, msg) {
    var test = new Assertion(act, msg, assert.equal);

    test.assert(
        exp == flag(test, 'object')
      , 'expected #{this} to equal #{exp}'
      , 'expected #{this} to not equal #{act}'
      , exp
      , act
      , true
    );
  };

Unfortunately there is no way we can test this, since this is a behavior of the test runner and it involves how it handles showing the test results, its not our own bug.
But since the other assertions have the showDiff flag set to true I think we should also set the showDiff flag to true on this assertion too, just to keep everything consistent.

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

Successfully merging a pull request may close this issue.

4 participants