Repository: unexpectedjs/unexpected
Pull request #328 (2016-08-17 22:31:52)

to satisfy: compare functions by value
This PR changes to satisfy to stop calling functions on the RHS with the value on the LHS, this behavior was too surprising. To replicate the old behavior you would have to use `expect.it(v => ...)` instead.

Before: 
(It is only in rare circumstances you need this functionality as you can usually just use a normal expect.it expression.  

```js
expect({
  value: 42
}, 'to satisfy', {
  value: v => {
    expect(v, 'to be a number')
  }
})
```

After:

```js
expect({
  value: 42
}, 'to satisfy', {
  value: expect.it(v => {
    expect(v, 'to be a number')
  })
})
```

We still support using a function in places where it is not confusing:

```js
expect(
  [0, 1, 2, 3, 4, 5],
  'to have values satisfying', (v) => {
    expect(v, 'to be a number')
  }
)
``` 

(This is also a rare case as you can write the same like this:

```js
expect(
  [0, 1, 2, 3, 4, 5],
  'to have values satisfying', 
  'to be a number'
)
``` 


papandreou: @sunesimonsen, mind if I land https://github.com/unexpectedjs/unexpected/pull/328/commits/ed4873e14b57a40d1cc0b0ecaa7c496737f11389 on its own? It seems pretty uncontroversial and provides that "escape" in a forwards compatible way, should we end up landing the rest of this (in a fixed up version :).
papandreou: Got bitten by this yesterday where I did something like this:

```js
expect(cb => myFunction(cb), 'to call the callback with error', httpErrors.BadRequest);
```

The mistake is that `httpErrors.BadRequest` is a function, not an error instance. I should have written `new httpErrors.BadRequest()`. That lead to a silent error because the assertion boiled down to `expect(new httpErrors.Conflict(), 'to satisfy', httpErrors.BadRequest)`, and that passes because `httpErrors.BadRequest` doesn't throw when passed an existing error instance.

To me that serves as a good example of why special casing functions in `to satisfy` RHS is a bad idea and we should fix it.
sunesimonsen: I agree, that is too surprising. Let's do it.
papandreou: @sunesimonsen Cool! Then we just need to figure out how to fix those failing tests. I remember I felt pretty stuck.
sunesimonsen: I can try to take a look at it. But this weekend is occupied.
papandreou: Appreciated :)
sunesimonsen: We should get this rebased and pick it up again. Sorry for never returning to the issue.
papandreou: Rebased.
papandreou: ... And no worries, this isn't exactly a high priority thing, since it requires a major version bump.
alexjeffburke: @papandreou the more I think about it the more I think this is a good idea. Question - I believe it should be possible to identify the cases that this change will no longer allow, for the sake of any large user of Unexpected I wonder if we might want to consider a release of v10 that actually warned this will be deprecated in v11? It could be seriously helpful for any large Unexpected user (ourselves at $work included) to check we don't rely on this. And for the record, I guess this is me sort of volunteering.. 🧐
sunesimonsen: > I guess this is me sort of volunteering..

❤️ 
sunesimonsen: I don't think the deprecation is a good idea now that we have a way forward and then we can remove the functionality in v11.

Patch:
diff --git a/lib/Unexpected.js b/lib/Unexpected.js
index a2597c51..ecdb5613 100644
--- a/lib/Unexpected.js
+++ b/lib/Unexpected.js
@@ -234,6 +234,16 @@ function createExpectIt(unexpected, expectations) {
         ? context
         : new Context(unexpected);
 
+    if (
+      orGroups.length === 1 &&
+      orGroups[0].length === 1 &&
+      orGroups[0][0].length === 1 &&
+      typeof orGroups[0][0][0] === 'function'
+    ) {
+      // expect.it(subject => ...)
+      return oathbreaker(orGroups[0][0][0](subject));
+    }
+
     const groupEvaluations = [];
     const promises = [];
     orGroups.forEach(orGroup => {
@@ -1326,7 +1336,7 @@ Unexpected.prototype._expect = function expect(context, args) {
 
   if (args.length < 2) {
     throw new Error('The expect function requires at least two parameters.');
-  } else if (testDescriptionString && testDescriptionString._expectIt) {
+  } else if (typeof testDescriptionString === 'function') {
     return that.expect.withError(
       () => testDescriptionString(subject),
       err => {
@@ -1401,7 +1411,7 @@ Unexpected.prototype._expect = function expect(context, args) {
           wrappedExpect,
           subject
         );
-      } else if (testDescriptionString && testDescriptionString._expectIt) {
+      } else if (typeof testDescriptionString === 'function') {
         wrappedExpect.errorMode = 'nested';
         return wrappedExpect.withError(
           () => testDescriptionString(subject),
diff --git a/lib/assertions.js b/lib/assertions.js
index 8bc22912..eedeb458 100644
--- a/lib/assertions.js
+++ b/lib/assertions.js
@@ -1094,10 +1094,11 @@ module.exports = expect => {
       const expected = {};
       keys.forEach((key, index) => {
         if (typeof nextArg === 'string') {
-          expected[key] = s => expect.shift(s);
+          expected[key] = expect.it(s => expect.shift(s));
         } else if (typeof nextArg === 'function') {
-          expected[key] = s =>
-            nextArg._expectIt ? nextArg(s, expect.context) : nextArg(s, index);
+          expected[key] = nextArg._expectIt
+            ? nextArg
+            : expect.it(s => nextArg(s, index));
         } else {
           expected[key] = nextArg;
         }
@@ -1504,7 +1505,7 @@ module.exports = expect => {
           const valueKey = valueType.valueForKey(value, keyInValue);
           const valueKeyType = expect.findTypeOf(valueKey);
 
-          if (valueKeyType.is('function')) {
+          if (valueKeyType.is('expect.it')) {
             return valueKey(subjectKey);
           } else {
             return expect(subjectKey, 'to [exhaustively] satisfy', valueKey);
@@ -1715,7 +1716,9 @@ module.exports = expect => {
                           } else if (type === 'insert') {
                             this.annotationBlock(function() {
                               if (
-                                expect.findTypeOf(diffItem.value).is('function')
+                                expect
+                                  .findTypeOf(diffItem.value)
+                                  .is('expect.it')
                               ) {
                                 this.error('missing: ').block(function() {
                                   this.omitSubject = undefined;
@@ -1867,8 +1870,6 @@ module.exports = expect => {
           if (valueKeyType.is('expect.it')) {
             expect.context.thisObject = subject;
             return valueKey(subjectKey, expect.context);
-          } else if (valueKeyType.is('function')) {
-            return valueKey(subjectKey);
           } else {
             return expect(subjectKey, 'to [exhaustively] satisfy', valueKey);
           }
@@ -1941,7 +1942,7 @@ module.exports = expect => {
                           conflicting = null;
                         }
                       } else if (!subjectType.hasKey(subject, key)) {
-                        if (expect.findTypeOf(valueKey).is('function')) {
+                        if (expect.findTypeOf(valueKey).is('expect.it')) {
                           if (promiseByKey[key].isRejected()) {
                             output.error('// missing:').sp();
                             valueOutput = output


Test patch:
diff --git a/test/api/it.spec.js b/test/api/it.spec.js
index 9b31e912..5a681369 100644
--- a/test/api/it.spec.js
+++ b/test/api/it.spec.js
@@ -346,14 +346,14 @@ describe('expect.it', () => {
 
   describe('when passed a function', () => {
     it('should succeed', () => {
-      expect.it(function(value) {
+      expect.it(value => {
         expect(value, 'to equal', 'foo');
       })('foo');
     });
 
     it('should fail with a diff', () => {
       expect(
-        function() {
+        () => {
           expect.it(function(value) {
             expect(value, 'to equal', 'bar');
           })('foo');
@@ -363,9 +363,22 @@ describe('expect.it', () => {
       );
     });
 
+    it('supports returning a promise from the function', () => {
+      return expect(
+        () =>
+          expect.it(value =>
+            expect.promise(run => {
+              setTimeout(run(() => expect(value, 'to equal', 'bar')));
+            })
+          )('foo'),
+        'to be rejected with',
+        "expected 'foo' to equal 'bar'\n" + '\n' + '-foo\n' + '+bar'
+      );
+    });
+
     it('should fail when passed more than two arguments', () => {
       expect(
-        function() {
+        () => {
           expect.it(function(value) {
             expect(value, 'to equal', 'bar');
           }, 'yadda')('foo');
@@ -374,5 +387,45 @@ describe('expect.it', () => {
         'expect.it(<function>) does not accept additional arguments'
       );
     });
+
+    describe('and chained the expression is chained', () => {
+      describe('and the first expression is a function', () => {
+        it('fails with a diff including all items in the chain', () => {
+          expect(
+            () => {
+              expect
+                .it(function(value) {
+                  expect(value, 'to equal', 'bar');
+                })
+                .and('to be a string')('foo');
+            },
+            'to throw',
+            "⨯ expected 'foo' to equal 'bar' and\n" +
+              '\n' +
+              '  -foo\n' +
+              '  +bar\n' +
+              "✓ expected 'foo' to be a string"
+          );
+        });
+      });
+
+      describe('and the second expression is a function', () => {
+        it('fails with a diff including all items in the chain', () => {
+          expect(
+            () => {
+              expect.it('to be a string').and(function(value) {
+                expect(value, 'to equal', 'bar');
+              })('foo');
+            },
+            'to throw',
+            "✓ expected 'foo' to be a string and\n" +
+              "⨯ expected 'foo' to equal 'bar'\n" +
+              '\n' +
+              '  -foo\n' +
+              '  +bar'
+          );
+        });
+      });
+    });
   });
 });
diff --git a/test/assertions/to-satisfy.spec.js b/test/assertions/to-satisfy.spec.js
index 0006efb8..47e75164 100644
--- a/test/assertions/to-satisfy.spec.js
+++ b/test/assertions/to-satisfy.spec.js
@@ -680,6 +680,33 @@ describe('to satisfy assertion', () => {
     });
   });
 
+  describe('with a expect.it function wrapper', () => {
+    it("succeeds if the function doesn't throw", () => {
+      expect({ foo: 'bar' }, 'to satisfy', {
+        foo: expect.it(v => expect(v, 'to be a string'))
+      });
+    });
+
+    it('should fail with an diff if the function fails', () => {
+      expect(
+        () => {
+          expect({ foo: 3 }, 'to satisfy', {
+            foo: expect.it(function(v) {
+              expect(v, 'to equal', 2);
+            })
+          });
+        },
+        'to throw',
+        'expected { foo: 3 }\n' +
+          "to satisfy { foo: expect.it(function (v) { expect(v, 'to equal', 2); }) }\n" +
+          '\n' +
+          '{\n' +
+          '  foo: 3 // should equal 2\n' +
+          '}'
+      );
+    });
+  });
+
   describe('with a synchronous expect.it in the RHS object', () => {
     it('should support an object with a property value of expect.it', () => {
       expect({ foo: 'bar' }, 'to satisfy', {
@@ -2232,23 +2259,26 @@ describe('to satisfy assertion', () => {
     });
   });
 
-  // Debatable:
-  describe('when an unpresent value to is satisfied against a function', () => {
-    it('should allow an unpresent value to be satisfied against a non-expect.it function', () => {
-      expect({}, 'to satisfy', { foo: function() {} });
+  describe('when an unpresent value to is satisfied against an expect.it function wrapper', () => {
+    it('should allow an unpresent value to be satisfied against the function', () => {
+      expect({}, 'to satisfy', {
+        foo: expect.it(v => {
+          expect(v, 'to be undefined');
+        })
+      });
     });
 
     it('should fail when the function throws', () => {
       expect(
-        function() {
+        () => {
           expect({}, 'to satisfy', {
-            foo(value) {
+            foo: expect.it(value => {
               expect(value, 'to be a string');
-            }
+            })
           });
         },
         'to throw',
-        function(err) {
+        err => {
           // Compensate for V8 5.1+ setting { foo: function () {} }.foo.name === 'foo'
           // http://v8project.blogspot.dk/2016/04/v8-release-51.html
           expect(
@@ -2271,7 +2301,7 @@ describe('to satisfy assertion', () => {
 
     it('should fail with a diff', () => {
       expect(
-        function() {
+        () => {
           expect({}, 'to satisfy', { foo: expect.it('to be a string') });
         },
         'to throw',
@@ -2286,11 +2316,16 @@ describe('to satisfy assertion', () => {
 
   it('should not break when the assertion fails and there is a fulfilled function in the RHS', () => {
     expect(
-      function() {
-        expect({}, 'to satisfy', { bar: 123, foo: function() {} });
+      () => {
+        expect({}, 'to satisfy', {
+          bar: 123,
+          foo: expect.it(function(v) {
+            expect(v, 'to be undefined');
+          })
+        });
       },
       'to throw',
-      function(err) {
+      err => {
         // Compensate for V8 5.1+ setting { foo: function () {} }.foo.name === 'foo'
         // http://v8project.blogspot.dk/2016/04/v8-release-51.html
         expect(
@@ -2299,17 +2334,40 @@ describe('to satisfy assertion', () => {
             .toString()
             .replace(/function foo/g, 'function '),
           'to satisfy',
-          'expected {} to satisfy { bar: 123, foo: function () {} }\n' +
+          'expected {} to satisfy\n' +
+            '{\n' +
+            '  bar: 123,\n' +
+            "  foo: expect.it(function (v) { expect(v, 'to be undefined'); })\n" +
+            '}\n' +
             '\n' +
             '{\n' +
             '  // missing bar: 123\n' +
-            '  // missing foo: should satisfy function () {}\n' +
+            "  // missing foo: should satisfy expect.it(function (v) { expect(v, 'to be undefined'); })\n" +
             '}'
         );
       }
     );
   });
 
+  it('should render a diff when the function differs', () => {
+    function myFunction() {}
+    function myOtherFunction() {}
+
+    expect(
+      () => {
+        expect({ foo: myFunction }, 'to satisfy', { foo: myOtherFunction });
+      },
+      'to throw',
+      'expected { foo: function myFunction() {} }\n' +
+        'to satisfy { foo: function myOtherFunction() {} }\n' +
+        '\n' +
+        '{\n' +
+        '  foo: function myFunction() {} // expected { foo: function myFunction() {} }\n' +
+        '                                // to satisfy { foo: function myOtherFunction() {} }\n' +
+        '}'
+    );
+  });
+
   describe('when matching the constructor property of an object', () => {
     function Foo() {}