feat: Add the assert.fail([message]) interface #1117

s-leroux · 2018-01-05T19:24:54Z

In relation to #1116 the assert.fail interface should accept only 1 arguments to fail with a custom message.

codecov · 2018-01-05T19:26:48Z

Codecov Report

Merging #1117 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1117      +/-   ##
==========================================
+ Coverage   93.64%   93.68%   +0.03%     
==========================================
  Files          32       32              
  Lines        1637     1646       +9     
  Branches      393      396       +3     
==========================================
+ Hits         1533     1542       +9     
  Misses        104      104

Impacted Files	Coverage Δ
lib/chai/interface/assert.js	`89.92% <100%> (+0.08%)`	⬆️
lib/chai/interface/should.js	`97.36% <100%> (+0.22%)`	⬆️
lib/chai/interface/expect.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c02f64b...0d1b586. Read the comment docs.

keithamus

This looks reasonable to me! Ideally, this PR will also need to change should.fail and expect.fail though.

s-leroux · 2018-01-07T16:21:20Z

OK @keithamus, I will change should.fail and expect.fail too.

s-leroux · 2018-01-07T16:53:20Z

Done.

I noticed chai.js was automatically modified while running the tests. But I left it out of the commit since it incorporated changes that were not caused by my own modifications.

meeber

@s-leroux Thanks for contributing!

I have a few inline comments regarding this PR. Also, my initial feeling is that this constitutes a breaking change, since it's changing how parameter handling is performed on an existing assertion. If other maintainers agree that this is the case, then prior to being merged, the commits in this PR should be squashed into a single one with a commit message body that begins with "BREAKING CHANGE:" (commit message title can remain as-is).

There may also need to be some follow-up actions regarding TypeScript per #1119, but I don't think that needs to be resolved in this PR.

meeber · 2018-01-07T20:11:23Z

lib/chai/interface/assert.js

@@ -41,6 +41,7 @@ module.exports = function (chai, util) {
  };

  /**
+   * ### .fail([message])
   * ### .fail(actual, expected, [message], [operator])


@chaijs/chai Anyone know how the website generation script handles listing multiple function signatures in this way? Or the proper way to do this with jsdoc in general? I struggled with this problem quite a bit in chaijs/check-error#18, and ended up including a single function signature at the top (with optional parameters) but documenting the alternate signatures in the jsdoc body. Not sure what the best approach is.

There is a possible solution for handling multiple function signature used in the assert.throws documentation. But I'm not sure this is the proper way of doing though: as a user, this is only by looking at the example section I was able to understand the various signatures for that method.

FWIW, I passed the source code through dox (which I assume to be the jsdoc parser used for the website). Here is the relevant part of the output:

"description": { "full": "<h3>.fail([message])</h3>\n<h3>.fail(actual, expected, [message], [operator])</h3>\n<p>Throw a failure.</p>", "summary": "<h3>.fail([message])</h3>\n<h3>.fail(actual, expected, [message], [operator])</h3>", "body": "<p>Throw a failure.</p>" },

As you can see, it produces two <h3> elements--once for each signature. Which seems quite reasonnable. Maybe we could ping someone at the chai-doc team to check if this is an issue?

Yup, chai-docs is using dox. That behavior looks promising. @astorije might know for sure.

meeber · 2018-01-07T20:11:59Z

lib/chai/interface/expect.js

@@ -10,6 +10,7 @@ module.exports = function (chai, util) {
  };

  /**
+   * ### .fail([message])
   * ### .fail(actual, expected, [message], [operator])


Same question here regarding function signatures.

meeber · 2018-01-07T20:12:25Z

lib/chai/interface/should.js

@@ -42,6 +42,7 @@ module.exports = function (chai, util) {
    var should = {};

    /**
+     * ### .fail([message])
     * ### .fail(actual, expected, [message], [operator])


Same question here regarding function signatures.

meeber · 2018-01-07T20:15:26Z

test/assert.js

@@ -20,6 +20,18 @@ describe('assert', function () {
    }).to.throw(chai.AssertionError, /this has failed/);
  });

+  it('fail', function () {
+    chai.expect(function () {


Rater than using chai.expect.to.throw, I think we can use the same err() helper function for these assert interface tests that are used in the expect and should interface tests. Doing so has the advantage of the test making sure implementation frames aren't in the Assertion Error's stack trace.

meeber · 2018-01-07T20:18:23Z

test/assert.js

@@ -20,6 +20,18 @@ describe('assert', function () {
    }).to.throw(chai.AssertionError, /this has failed/);
  });

+  it('fail', function () {


I'd like the test descriptions to be more descriptive than "fail", even if it's as simple as "fail (1 arg)" and "fail (no args)". Another option would be to create a new describe block for fail, and then put descriptive it blocks inside of it.

meeber · 2018-01-07T20:19:03Z

test/assert.js

+    chai.expect(function () {
+      assert.fail();
+    }).to.throw(chai.AssertionError, /assert\.fail/);
+  });


Same comments as above regarding description and err helper function.

meeber · 2018-01-07T20:19:47Z

test/expect.js

+    }, 'this has failed');
+  });
+
+  it('fail', function () {


Same comment as the assert interface test regarding it block description.

meeber · 2018-01-07T20:19:57Z

test/expect.js

+    }, 'this has failed');
+  });
+
+  it('fail', function () {


Same comment as the assert interface test regarding it block description.

meeber · 2018-01-07T20:20:04Z

test/should.js

@@ -239,6 +239,18 @@ describe('should', function() {
    }, 'this has failed');
  });

+  it('fail', function () {


Same comment as the assert interface test regarding it block description.

meeber · 2018-01-07T20:20:09Z

test/should.js

+    }, 'this has failed');
+  });
+
+  it('fail', function () {


Same comment as the assert interface test regarding it block description.

keithamus · 2018-01-07T21:05:21Z

Also, my initial feeling is that this constitutes a breaking change, since it's changing how parameter handling is performed on an existing assertion.

My feeling is this is a feature. My reasoning is that the behaviour of expect.fail with one argument, certainly as per the docs, is undefined. Because the behavior is undefined, any existing behavior is able to change. To rephrase: anyone using expect.fail with less than two arguments is relying on ondocumented bevhaior.

meeber · 2018-01-07T21:38:26Z

@keithamus Good point. I agree. Not a breaking change.

s-leroux · 2018-01-07T21:55:53Z

First, thank you @keithamus and @meeber for having reviewed that PR.

I'd like the test descriptions to be more descriptive than "fail", even if it's as simple as "fail (1 arg)" and "fail (no args)". Another option would be to create a new describe block for fail, and then put descriptive it blocks inside of it.

Of course.

Rater than using chai.expect.to.throw, I think we can use the same err() helper function for these assert interface tests that are used in the expect and should interface tests.

I will fix that. I just copied-pasted existing tests. And I assumed there was some obscure reason for not using it in the assert test suite.

Anyone know how the website generation script handles listing multiple function signatures in this way? Or the proper way to do this with jsdoc in general?

I have no idea how to do it properly. Any suggestion welcome!

There may also need to be some follow-up actions regarding TypeScript per #1119, but I don't think that needs to be resolved in this PR.

For now, I have absolutly no idea what your talking about ;)
So I suggest to let that out of this PR, but I would be happy to help with that in a second time.

If other maintainers agree that this is the case, then prior to being merged, the commits in this PR should be squashed into a single one

I will do that once I will have your green light for the other changes.

s-leroux · 2018-01-11T21:15:41Z

I fixed the tests, add a little bit of documentation in the method header and squashed the commit.

I left the "double function signature" in the method header since this is definitely the most understandable way to describe the different function signatures and that seems to be supported by dox:

    * ### .fail([message])
    * ### .fail(actual, expected, [message], [operator])

But let me know if you want I change that to something else, in case this would cause an issue with the chai-doc tool.

meeber · 2018-01-11T21:34:55Z

Thanks @s-leroux. I like the changes. I'd say let's move forward with the double function signatures unless someone chimes in otherwise.

One thing: there's a couple of duplicate tests that can be deleted.

Fix chaijs#1116. The `assert.fail` interface should accept being called with only 1 arguments to fail with a custom message.

s-leroux · 2018-01-12T08:58:49Z

Thank you for your review @meeber.
I removed the duplicate tests.

keithamus

This is great work @s-leroux! Really clean PR with excellent tests, incuding bumping existing coverage. Also thank you so much for responding to our reviews so quickly and properly! You should consider yourself proud!

s-leroux · 2018-01-12T21:55:15Z

Thank you @keithamus. It was a pleasure!

s-leroux requested a review from a team as a code owner January 5, 2018 19:24

s-leroux changed the title ~~Add the assert.fail([message]) interface [#1116]~~ feat: Add the assert.fail([message]) interface Jan 5, 2018

s-leroux force-pushed the assert/fail/only-one-argument branch from 84a378e to 8f7addc Compare January 5, 2018 19:33

keithamus requested changes Jan 7, 2018

View reviewed changes

meeber requested changes Jan 7, 2018

View reviewed changes

s-leroux force-pushed the assert/fail/only-one-argument branch 3 times, most recently from bde6607 to 8a987b4 Compare January 11, 2018 21:09

feat: Add the .fail([message]) interface

0d1b586

Fix chaijs#1116. The `assert.fail` interface should accept being called with only 1 arguments to fail with a custom message.

s-leroux force-pushed the assert/fail/only-one-argument branch from 8a987b4 to 0d1b586 Compare January 12, 2018 07:33

keithamus approved these changes Jan 12, 2018

View reviewed changes

meeber approved these changes Jan 12, 2018

View reviewed changes

meeber merged commit d3908a6 into chaijs:master Jan 12, 2018

snyk-bot mentioned this pull request Jan 29, 2020

[Snyk] Upgrade chai from 4.1.2 to 4.2.0 davemenninger/socket.io-chat-fargate#3

Open

snyk-bot mentioned this pull request Mar 15, 2020

[Snyk] Upgrade chai from 4.0.2 to 4.2.0 postmanlabs/sails-env-switch#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add the assert.fail([message]) interface #1117

feat: Add the assert.fail([message]) interface #1117

s-leroux commented Jan 5, 2018

codecov bot commented Jan 5, 2018 •

edited

keithamus left a comment

s-leroux commented Jan 7, 2018

s-leroux commented Jan 7, 2018

meeber left a comment

meeber Jan 7, 2018

s-leroux Jan 7, 2018

s-leroux Jan 7, 2018

meeber Jan 8, 2018

meeber Jan 7, 2018

meeber Jan 7, 2018

meeber Jan 7, 2018

meeber Jan 7, 2018

meeber Jan 7, 2018

meeber Jan 7, 2018

meeber Jan 7, 2018

meeber Jan 7, 2018

meeber Jan 7, 2018

keithamus commented Jan 7, 2018

meeber commented Jan 7, 2018

s-leroux commented Jan 7, 2018 •

edited

s-leroux commented Jan 11, 2018

meeber commented Jan 11, 2018

s-leroux commented Jan 12, 2018

keithamus left a comment

s-leroux commented Jan 12, 2018

feat: Add the assert.fail([message]) interface #1117

feat: Add the assert.fail([message]) interface #1117

Conversation

s-leroux commented Jan 5, 2018

codecov bot commented Jan 5, 2018 • edited

Codecov Report

keithamus left a comment

Choose a reason for hiding this comment

s-leroux commented Jan 7, 2018

s-leroux commented Jan 7, 2018

meeber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keithamus commented Jan 7, 2018

meeber commented Jan 7, 2018

s-leroux commented Jan 7, 2018 • edited

s-leroux commented Jan 11, 2018

meeber commented Jan 11, 2018

s-leroux commented Jan 12, 2018

keithamus left a comment

Choose a reason for hiding this comment

s-leroux commented Jan 12, 2018

codecov bot commented Jan 5, 2018 •

edited

s-leroux commented Jan 7, 2018 •

edited