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

to satisfy: compare functions by value #328

Merged
merged 7 commits into from
Oct 3, 2018

Conversation

papandreou
Copy link
Member

@papandreou papandreou commented Aug 17, 2016

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.

expect({
  value: 42
}, 'to satisfy', {
  value: v => {
    expect(v, 'to be a number')
  }
})

After:

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:

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:

expect(
  [0, 1, 2, 3, 4, 5],
  'to have values satisfying', 
  'to be a number'
)

@papandreou
Copy link
Member Author

@sunesimonsen, mind if I land ed4873e 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
Copy link
Member Author

papandreou commented Jun 16, 2017

Got bitten by this yesterday where I did something like this:

expect(myFunction, '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
Copy link
Member

I agree, that is too surprising. Let's do it.

@papandreou
Copy link
Member Author

@sunesimonsen Cool! Then we just need to figure out how to fix those failing tests. I remember I felt pretty stuck.

@sunesimonsen
Copy link
Member

I can try to take a look at it. But this weekend is occupied.

@papandreou
Copy link
Member Author

Appreciated :)

@sunesimonsen
Copy link
Member

We should get this rebased and pick it up again. Sorry for never returning to the issue.

@papandreou papandreou force-pushed the feature/toSatisfyCompareFunctionsByValue branch from f589d8d to 2686119 Compare April 17, 2018 08:41
@papandreou
Copy link
Member Author

Rebased.

@papandreou
Copy link
Member Author

... And no worries, this isn't exactly a high priority thing, since it requires a major version bump.

@alexjeffburke
Copy link
Member

@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
Copy link
Member

I guess this is me sort of volunteering..

❤️

@sunesimonsen
Copy link
Member

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.

@alexjeffburke alexjeffburke mentioned this pull request Jul 1, 2018
5 tasks
@sunesimonsen sunesimonsen self-assigned this Sep 30, 2018
@sunesimonsen sunesimonsen force-pushed the feature/toSatisfyCompareFunctionsByValue branch 3 times, most recently from 0f5080a to 76259d2 Compare September 30, 2018 18:54
@sunesimonsen sunesimonsen changed the base branch from master to v11 September 30, 2018 18:54
…re the functions with === instead of executing the RHS one with the LHS one as the argument.

(Breaking!)
@sunesimonsen sunesimonsen force-pushed the feature/toSatisfyCompareFunctionsByValue branch from 76259d2 to 930431c Compare September 30, 2018 18:55
@sunesimonsen sunesimonsen force-pushed the feature/toSatisfyCompareFunctionsByValue branch from 81afefc to 8b0cd9d Compare September 30, 2018 19:53
@sunesimonsen
Copy link
Member

@papandreou I'm pretty sure that we need to be able to detect when an expect.it(v => ...) function wrappers is used and handle it in the to satisfy object diff like we did with the functions before. I'll do fix this, but it wont happen tonight :-)

@sunesimonsen sunesimonsen force-pushed the feature/toSatisfyCompareFunctionsByValue branch from a41800b to d14d14b Compare September 30, 2018 20:15
@papandreou
Copy link
Member Author

@sunesimonsen, thanks, looking forward to it! (Kinda relieved that it wasn't easy :)

@sunesimonsen
Copy link
Member

Mhh this is more complicated than I though, you can in principal do this:

expect({ foo: 'text' }, 'to satisfy', {
  foo: expect.it('to be a string').and(v => expect(v, 'to match', /ext$/))
})

@papandreou
Copy link
Member Author

Hmm, yeah, you're right. We've supported that since 10.28.0 due to b562c46

It's probably a well kept secret, so I think we can consider unsupporting it if it makes this really hard.

@sunesimonsen sunesimonsen force-pushed the feature/toSatisfyCompareFunctionsByValue branch from d14d14b to 8b0cd9d Compare September 30, 2018 20:47
@sunesimonsen
Copy link
Member

@papandreou what I'm trying to get at, is that you can basically do anything, so it is kind of hard to inline the diff - and know when that is the right thing to do. I'm actually a little puzzled about the old behavior.

@sunesimonsen
Copy link
Member

I think we need to write some more tests for the expect.it(subject => ...) handling. But other than that, this PR now looks like it is working.

@papandreou
Copy link
Member Author

😌

describe('and the first expression is a function', () => {
it('fails with a diff including all items in the chain', () => {
expect(
function() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine to use arrow functions here, as long as they're not part of the expected error message.

Copy link
Member Author

@papandreou papandreou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks so much for fixing it up! I can't approve because I originally opened the PR, but :shipit:!

@sunesimonsen sunesimonsen changed the title WIP: to satisfy: compare functions by value to satisfy: compare functions by value Oct 3, 2018
@alexjeffburke
Copy link
Member

I really like this. It is my firm belief that it cleans up a potentially confusing inconsistency and provides a very uniform story around how to go about comparing values.

With the understanding that the expect.it(value => is a bit of a special case, I think it's very
powerful tool and expect.it is the right place for it. Ultimately users always have the power of the language available to them and making it regular is a huge plus.

Copy link
Member

@alexjeffburke alexjeffburke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a nice outcome that type check ends up being for expect.it specifically while shifted values gets dispatched the same way. Nice.

@alexjeffburke
Copy link
Member

A final thought while looking at this - there may be other implementations of "to satisfy" that need to catch up with this. I think at least U-map (since it sadly had to duplicate quite a bit). I'd love to catch that in an automated way but it might be that test cases need to be added. What do you both think?

@sunesimonsen sunesimonsen merged commit 8a2394e into v11 Oct 3, 2018
@sunesimonsen sunesimonsen deleted the feature/toSatisfyCompareFunctionsByValue branch October 3, 2018 16:03
@sunesimonsen sunesimonsen mentioned this pull request Oct 3, 2018
25 tasks
@papandreou
Copy link
Member Author

@alexjeffburke, could be! Let's hope we have good test coverage in those plugins 👿

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 this pull request may close these issues.

None yet

3 participants