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

referee.exception doesn't test for exact match of exception properties #252

Open
gukoff opened this issue Aug 14, 2023 · 1 comment
Open

Comments

@gukoff
Copy link

gukoff commented Aug 14, 2023

We understand you have a problem and are in a hurry, but please provide us with some info to make it much more likely for your issue to be understood, worked on and resolved quickly.

  • library version : 10.0.0
  • Environment : OSX
  • Other libraries you are using: sinon

How to reproduce

assert.exception(
    function () {
        throw new TypeError('long exception message');
    },
    {
        message: "message",
        name: "rr",
    }
);

What did you expect to happen?

Test fails

What actually happens

Test succeeds

Reason

When samsam library detects there's no match, referee tries to verify that by iterating property-by-property and comparing again: https://github.com/sinonjs/referee/blob/main/lib/assertions/exception.js#L52

The problem is, comparing objects is different from comparing strings in samsam.

When iterating property-by-property, we don't check for string equality, rather that one string is a substring of anoter: https://github.com/sinonjs/samsam/blob/main/lib/match.js#L71-L74

This makes the check in assert.exception weaker - it succeeds if the strings in the matcher strings are just the substrings of the corresponding properties.

I would expect this behaviour to be either changed or explicitly documented.

@mantoni
Copy link
Member

mantoni commented Aug 15, 2023

I agree with this and I ran into the same issue myself. My preference would be to change the current behaviour and release a major. If someone wants the current behaviour, they can wrap the expectation like match({ ... }), so it's quite easy to migrate.

Would you be able to send a pull request for this?

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

No branches or pull requests

2 participants