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

Custom message not always respected #923

Closed
meeber opened this issue Jan 29, 2017 · 2 comments
Closed

Custom message not always respected #923

meeber opened this issue Jan 29, 2017 · 2 comments

Comments

@meeber
Copy link
Contributor

meeber commented Jan 29, 2017

As noted here, there's a problem with some assertions not always respecting a custom failed assertion error message. This problem is different (and more widespread) than the flag transfer problem mentioned in #904.

Here's an example:

expect('blah').to.be.closeTo(2, 1, 'CUSTOM');  // Correctly includes custom error message
expect('blah', 'CUSTOM').to.be.closeTo(2, 1); // Incorrectly excludes custom error message

The culprit for the above example is this line. If the msg argument isn't provided, then it needs to look at the value of the msg flag instead.

Many other occurrences of this issue are similar to the above example. Also there are some assertions that fail to pass on the msg at all to internal assertions or errors, but #922 coincidentally fixes those occurrences.

@meeber meeber added the bug label Jan 29, 2017
@keithamus
Copy link
Member

Something I'd like to just throw out there: is it worth fixing these issues given that if we implement #585 anything like it currently looks, then messaging will be centralised and automated, and any efforts will be refactored away?

@meeber
Copy link
Contributor Author

meeber commented Jan 30, 2017

I think it's worth it, it's a quick-n-easy fix, low-impact. Just waiting for #922 so don't have to mess with resolving merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants