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

Fix false positives on errors with different messages. #23

Merged
merged 3 commits into from
Aug 9, 2017

Conversation

kurtcorbett
Copy link
Contributor

@kurtcorbett kurtcorbett commented Aug 9, 2017

This PR:

  • Sets up mocha and refactors tests in src/specs/test.spec.js to run with either mocha or Jest. This was done to ensure that we can verify that our functional tests work properly with either framework. It could be part of the CI to run both sets of tests.
  • Adds regression test and addresses the bug in node's assert.deepEqual where all Errors are considered equal. This is a known and accepted issue as described here Comparing errors through deepEqual nodejs/node#3122

Concerns:

  • The modified deepEqual check on Errors is naive, creating two "error" objects with a message and name property and comparing them. I'm concerned about browser compatibility with this. Each browser has different property names on Errors and we could run into problems if someone is testing this with jasmine or some other client side testing framework. We may consider creating a function to normalize error properties across the different browsers and node.

@kurtcorbett
Copy link
Contributor Author

Added changes per @orourkedd suggestions.

@orourkedd orourkedd merged commit 3c2444c into orourkedd:master Aug 9, 2017
@orourkedd
Copy link
Owner

Great idea of using the deepEqual and running the tests with both runners.

@kurtcorbett
Copy link
Contributor Author

We could consider adding Jasmine to ensure client side behavior consistency, possibly across multiple (headless) browsers. It adds overhead to the project, but it'd be nice to be sure that we are covering all likely environments.

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.

2 participants