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

WIP: Update .throw and add .error #1015

Closed
wants to merge 2 commits into from

Conversation

meeber
Copy link
Contributor

@meeber meeber commented Aug 2, 2017

DO NOT MERGE; THIS IS A WORK IN PROGRESS!!

I've been doing a lot of work on #930. This turned out to be a lot harder than expected; .throw couldn't be updated to simply call .error internally due to the difference in failed assertion messages between the two. I suspect this is the same kind of issue causing problems in plugins like chai-as-promised when they try to adapt .throw.

The more I looked at our current .throw implementation, the more I realized that the complexity wasn't because of negation as we originally thought but rather because of how the failed assertion messages were being composed. As a result, this PR turned into a pretty big refactor of .throw to reduce complexity. This also required changing some of .throw's failed assertion messages to be more consistent.

Some notes/thoughts:

  • This PR currently only adds .error to the BDD interface. I'll add the Assert interface later.
  • Is changing a failed assertion message considered a breaking change? On the one hand, it's likely to break the tests of any plugins that do string matching against failed assertion messages. On the other hand, it's unlikely to break the actual functionality of the plugin from the end user's perspective, or any of the end user's tests. I'm currently learning toward "no".
  • The matchError and decribeExpectedError helper functions should probably be moved to the check-error module.
  • Part of what .throw does is assert that a function throws, without taking into account what it throws. This means it will work if the target function throws something besides an Error object (e.g., a string, a number, null, or even undefined). I think it's important that the .throw logic retain this base functionality. However, when it comes to adding arguments to the .throw function to perform additional assertions on the value that was thrown, I think we should consider requiring that the object thrown be an Error object. Currently, the behavior of the various check-error functions is a little sketchy if the object being tested isn't an Error. This problem carries over into the matchError helper function as well.

meeber added 2 commits July 29, 2017 21:06
This commit is mostly about refactoring `.throw` in preparation for an
upcoming `.error` assertion, but in the process, failed assertion
messages for `.throw` were changed for simplicity, consistency and
correctness.
@meeber meeber requested a review from a team as a code owner August 2, 2017 01:18
@meeber meeber closed this Aug 6, 2017
@meeber meeber deleted the add-error-assertion branch August 6, 2017 13:47
@meeber meeber restored the add-error-assertion branch August 6, 2017 13:47
@meeber
Copy link
Contributor Author

meeber commented Aug 6, 2017

Didn't mean to close this. I think I broke it; the reopen button is grayed out for me. I'll create a new PR later if it doesn't magically fix itself.

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.

1 participant