Repository: chaijs/chai
Pull request #1117 (2018-01-05 19:24:54)

feat: Add the assert.fail([message]) interface
In relation to #1116 the `assert.fail` interface should accept only 1 arguments to fail with a custom message.

codecov[bot]: # [Codecov](https://codecov.io/gh/chaijs/chai/pull/1117?src=pr&el=h1) Report
> Merging [#1117](https://codecov.io/gh/chaijs/chai/pull/1117?src=pr&el=desc) into [master](https://codecov.io/gh/chaijs/chai/commit/f54f71c23493681deed2b22accb1adf1f9a677c3?src=pr&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/chaijs/chai/pull/1117/graphs/tree.svg?height=150&width=650&token=J8Fz3KwOQq&src=pr)](https://codecov.io/gh/chaijs/chai/pull/1117?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1117      +/-   ##
==========================================
- Coverage   93.61%   93.56%   -0.05%     
==========================================
  Files          32       32              
  Lines        1628     1631       +3     
  Branches      393      394       +1     
==========================================
+ Hits         1524     1526       +2     
- Misses        104      105       +1
```


| [Impacted Files](https://codecov.io/gh/chaijs/chai/pull/1117?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [lib/chai/interface/assert.js](https://codecov.io/gh/chaijs/chai/pull/1117/diff?src=pr&el=tree#diff-bGliL2NoYWkvaW50ZXJmYWNlL2Fzc2VydC5qcw==) | `89.92% <100%> (+0.08%)` | :arrow_up: |
| [lib/chai/utils/overwriteProperty.js](https://codecov.io/gh/chaijs/chai/pull/1117/diff?src=pr&el=tree#diff-bGliL2NoYWkvdXRpbHMvb3ZlcndyaXRlUHJvcGVydHkuanM=) | `90.9% <0%> (-4.55%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/chaijs/chai/pull/1117?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/chaijs/chai/pull/1117?src=pr&el=footer). Last update [f54f71c...84a378e](https://codecov.io/gh/chaijs/chai/pull/1117?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

s-leroux: OK @keithamus, I will change `should.fail` and `expect.fail` too.
s-leroux: 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.
keithamus: > 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: @keithamus Good point. I agree. Not a breaking change.
s-leroux: First, thank you @keithamus and @meeber for having reviewed that PR.

I don't think this is a breaking change since currently `fail` is documented to only accepting 2, 3 or 4 arguments. As a matter of fact, I was initially not sure if I should mark that as a "feature" and a "fix"--since, [according to the website](http://chaijs.com/guide/styles/#assert) (emphasis mine):

> The assert style is exposed through assert interface. This provides the **classic assert-dot notation, similar to that packaged with node.js**. This assert module, however, **provides several additional tests** and is browser compatible.


So, I understand `chai.assert` as being a super-set of node.js `.assert`. And  [`assert.fail` with only one argument](https://nodejs.org/api/assert.html#assert_assert_fail_message) is indeed supported there.

Anyway, whatever is your decision, I will follow you.

----

> 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: 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: 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](https://github.com/s-leroux/chai/blob/8a987b4c8b7c45f5280e7e081fb3564e6e0880b7/test/assert.js#L37-L47) that can be deleted.

Patch:
diff --git a/lib/chai/interface/assert.js b/lib/chai/interface/assert.js
index 7772e2c..5f46c80 100644
--- a/lib/chai/interface/assert.js
+++ b/lib/chai/interface/assert.js
@@ -41,10 +41,18 @@ module.exports = function (chai, util) {
   };
 
   /**
+   * ### .fail([message])
    * ### .fail(actual, expected, [message], [operator])
    *
    * Throw a failure. Node.js `assert` module-compatible.
    *
+   *     assert.fail();
+   *     assert.fail("custom error message");
+   *     assert.fail(1, 2);
+   *     assert.fail(1, 2, "custom error message");
+   *     assert.fail(1, 2, "custom error message", ">");
+   *     assert.fail(1, 2, undefined, ">");
+   *
    * @name fail
    * @param {Mixed} actual
    * @param {Mixed} expected
@@ -55,6 +63,13 @@ module.exports = function (chai, util) {
    */
 
   assert.fail = function (actual, expected, message, operator) {
+    if (arguments.length < 2) {
+        // Comply with Node's fail([message]) interface
+
+        message = actual;
+        actual = undefined;
+    }
+
     message = message || 'assert.fail()';
     throw new chai.AssertionError(message, {
         actual: actual
diff --git a/lib/chai/interface/expect.js b/lib/chai/interface/expect.js
index 8c34072..6867e2a 100644
--- a/lib/chai/interface/expect.js
+++ b/lib/chai/interface/expect.js
@@ -10,10 +10,18 @@ module.exports = function (chai, util) {
   };
 
   /**
+   * ### .fail([message])
    * ### .fail(actual, expected, [message], [operator])
    *
    * Throw a failure.
    *
+   *     expect.fail();
+   *     expect.fail("custom error message");
+   *     expect.fail(1, 2);
+   *     expect.fail(1, 2, "custom error message");
+   *     expect.fail(1, 2, "custom error message", ">");
+   *     expect.fail(1, 2, undefined, ">");
+   *
    * @name fail
    * @param {Mixed} actual
    * @param {Mixed} expected
@@ -24,6 +32,11 @@ module.exports = function (chai, util) {
    */
 
   chai.expect.fail = function (actual, expected, message, operator) {
+    if (arguments.length < 2) {
+        message = actual;
+        actual = undefined;
+    }
+
     message = message || 'expect.fail()';
     throw new chai.AssertionError(message, {
         actual: actual
diff --git a/lib/chai/interface/should.js b/lib/chai/interface/should.js
index d7525b9..5406e5e 100644
--- a/lib/chai/interface/should.js
+++ b/lib/chai/interface/should.js
@@ -42,10 +42,19 @@ module.exports = function (chai, util) {
     var should = {};
 
     /**
+     * ### .fail([message])
      * ### .fail(actual, expected, [message], [operator])
      *
      * Throw a failure.
      *
+     *     should.fail();
+     *     should.fail("custom error message");
+     *     should.fail(1, 2);
+     *     should.fail(1, 2, "custom error message");
+     *     should.fail(1, 2, "custom error message", ">");
+     *     should.fail(1, 2, undefined, ">");
+     *
+     *
      * @name fail
      * @param {Mixed} actual
      * @param {Mixed} expected
@@ -56,6 +65,11 @@ module.exports = function (chai, util) {
      */
 
     should.fail = function (actual, expected, message, operator) {
+      if (arguments.length < 2) {
+          message = actual;
+          actual = undefined;
+      }
+
       message = message || 'should.fail()';
       throw new chai.AssertionError(message, {
           actual: actual
diff --git a/lib/chai/utils/proxify.js b/lib/chai/utils/proxify.js
index 4bbbee3..51e0e63 100644
--- a/lib/chai/utils/proxify.js
+++ b/lib/chai/utils/proxify.js
@@ -49,31 +49,19 @@ module.exports = function proxify(obj, nonChainableMethodName) {
             nonChainableMethodName + '".');
         }
 
-        // If the property is reasonably close to an existing Chai property,
-        // suggest that property to the user. Only suggest properties with a
-        // distance less than 4.
-        var suggestion = null;
-        var suggestionDistance = 4;
-        getProperties(target).forEach(function(prop) {
-          if (
-            !Object.prototype.hasOwnProperty(prop) &&
-            builtins.indexOf(prop) === -1
-          ) {
-            var dist = stringDistanceCapped(
-              property,
-              prop,
-              suggestionDistance
-            );
-            if (dist < suggestionDistance) {
-              suggestion = prop;
-              suggestionDistance = dist;
-            }
-          }
+        var orderedProperties = getProperties(target).filter(function(property) {
+          return !Object.prototype.hasOwnProperty(property) &&
+            builtins.indexOf(property) === -1;
+        }).sort(function(a, b) {
+          return stringDistance(property, a) - stringDistance(property, b);
         });
 
-        if (suggestion !== null) {
+        if (orderedProperties.length &&
+            stringDistance(orderedProperties[0], property) < 4) {
+          // If the property is reasonably close to an existing Chai property,
+          // suggest that property to the user.
           throw Error('Invalid Chai property: ' + property +
-            '. Did you mean "' + suggestion + '"?');
+            '. Did you mean "' + orderedProperties[0] + '"?');
         } else {
           throw Error('Invalid Chai property: ' + property);
         }
@@ -101,44 +89,36 @@ module.exports = function proxify(obj, nonChainableMethodName) {
 };
 
 /**
- * # stringDistanceCapped(strA, strB, cap)
- * Return the Levenshtein distance between two strings, but no more than cap.
+ * # stringDistance(strA, strB)
+ * Return the Levenshtein distance between two strings.
  * @param {string} strA
  * @param {string} strB
- * @param {number} number
- * @return {number} min(string distance between strA and strB, cap)
+ * @return {number} the string distance between strA and strB
  * @api private
  */
 
-function stringDistanceCapped(strA, strB, cap) {
-  if (Math.abs(strA.length - strB.length) >= cap) {
-    return cap;
-  }
-
-  var memo = [];
-  // `memo` is a two-dimensional array containing distances.
-  // memo[i][j] is the distance between strA.slice(0, i) and
-  // strB.slice(0, j).
-  for (var i = 0; i <= strA.length; i++) {
-    memo[i] = Array(strB.length + 1).fill(0);
-    memo[i][0] = i;
-  }
-  for (var j = 0; j < strB.length; j++) {
-    memo[0][j] = j;
+function stringDistance(strA, strB, memo) {
+  if (!memo) {
+    // `memo` is a two-dimensional array containing a cache of distances
+    // memo[i][j] is the distance between strA.slice(0, i) and
+    // strB.slice(0, j).
+    memo = [];
+    for (var i = 0; i <= strA.length; i++) {
+      memo[i] = [];
+    }
   }
 
-  for (var i = 1; i <= strA.length; i++) {
-    var ch = strA.charCodeAt(i - 1);
-    for (var j = 1; j <= strB.length; j++) {
-      if (Math.abs(i - j) >= cap) {
-        memo[i][j] = cap;
-        continue;
-      }
-      memo[i][j] = Math.min(
-        memo[i - 1][j] + 1,
-        memo[i][j - 1] + 1,
-        memo[i - 1][j - 1] +
-          (ch === strB.charCodeAt(j - 1) ? 0 : 1)
+  if (!memo[strA.length] || !memo[strA.length][strB.length]) {
+    if (strA.length === 0 || strB.length === 0) {
+      memo[strA.length][strB.length] = Math.max(strA.length, strB.length);
+    } else {
+      var sliceA = strA.slice(0, -1);
+      var sliceB = strB.slice(0, -1);
+      memo[strA.length][strB.length] = Math.min(
+        stringDistance(sliceA, strB, memo) + 1,
+        stringDistance(strA, sliceB, memo) + 1,
+        stringDistance(sliceA, sliceB, memo) +
+          (strA.slice(-1) === strB.slice(-1) ? 0 : 1)
       );
     }
   }


Test patch:
diff --git a/test/assert.js b/test/assert.js
index d23cb37..10e1621 100644
--- a/test/assert.js
+++ b/test/assert.js
@@ -14,10 +14,24 @@ describe('assert', function () {
     }, "expected foo to equal `bar`");
   });
 
-  it('fail', function () {
-    chai.expect(function () {
-      assert.fail(0, 1, 'this has failed');
-    }).to.throw(chai.AssertionError, /this has failed/);
+  describe("fail", function() {
+    it('should accept a message as the 3rd argument', function () {
+      err(function() {
+        assert.fail(0, 1, 'this has failed');
+      }, /this has failed/);
+    });
+
+    it('should accept a message as the only argument', function () {
+      err(function() {
+        assert.fail('this has failed');
+      }, /this has failed/);
+    });
+
+    it('should produce a default message when called without any arguments', function () {
+      err(function() {
+        assert.fail();
+      }, /assert\.fail()/);
+    });
   });
 
   it('isTrue', function () {
diff --git a/test/expect.js b/test/expect.js
index dd16994..8d658f2 100644
--- a/test/expect.js
+++ b/test/expect.js
@@ -236,10 +236,24 @@ describe('expect', function () {
     , 'of', 'same', 'but', 'does' ].forEach(test);
   });
 
-  it('fail', function () {
-    err(function() {
-      expect.fail(0, 1, 'this has failed');
-    }, /this has failed/);
+  describe("fail", function() {
+    it('should accept a message as the 3rd argument', function () {
+      err(function() {
+        expect.fail(0, 1, 'this has failed');
+      }, /this has failed/);
+    });
+
+    it('should accept a message as the only argument', function () {
+      err(function() {
+        expect.fail('this has failed');
+      }, /this has failed/);
+    });
+
+    it('should produce a default message when called without any arguments', function () {
+      err(function() {
+        expect.fail();
+      }, /expect\.fail()/);
+    });
   });
 
   it('true', function(){
diff --git a/test/should.js b/test/should.js
index 2b62af3..1eb10a4 100644
--- a/test/should.js
+++ b/test/should.js
@@ -233,10 +233,24 @@ describe('should', function() {
     , 'of', 'same', 'but', 'does' ].forEach(test);
   });
 
-  it('fail', function () {
-    err(function() {
-      should.fail(0, 1, 'this has failed');
-    }, 'this has failed');
+  describe("fail", function() {
+    it('should accept a message as the 3rd argument', function () {
+      err(function() {
+        should.fail(0, 1, 'this has failed');
+      }, /this has failed/);
+    });
+
+    it('should accept a message as the only argument', function () {
+      err(function() {
+        should.fail('this has failed');
+      }, /this has failed/);
+    });
+
+    it('should produce a default message when called without any arguments', function () {
+      err(function() {
+        should.fail();
+      }, /should\.fail()/);
+    });
   });
 
   it('root exist', function () {