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

Queen attack: force test failure when exception is not thrown #346

Merged
merged 1 commit into from Aug 30, 2017

Conversation

@alexashley
Copy link
Contributor

commented Aug 25, 2017

I noticed that this test will pass as long as the Queens constructor is implemented, because the assertion is in the catch. Since it was a quick fix, I didn't make an issue.

@alexashley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2017

I think the build is failing because the example implementation is incorrect.

> console.log([1] === [1])
> false
@alexashley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2017

I tried to follow the existing conventions with my changes to the example, but it added some lint errors 😞
I also changed the example to throw a string instead of an Error since that's what the test was expecting.

@matthewmorgan

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2017

@alexashley thanks for taking this on! I have a couple of thoughts on this PR:

  1. If you're getting linting errors, you should be able to automatically fix them using eslint --fix . If this doesn't work, please let me know.
  2. The correct pattern to use when expecting an error is to use the toThrow() matcher. As long as you're changing this code, let's do it right! The Jest docs have some good examples: https://facebook.github.io/jest/docs/en/expect.html#tothrowerror

Please let me know if you have any questions or if I can help in any way.

@alexashley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2017

@matthewmorgan

  1. 👍 No more lint errors in queen-attack.spec.js or example.js
  2. Fixed!
@matthewmorgan

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2017

OK, looks great! One last thing: we like to have the commits squashed to a single commit for this repo. We can do that as part of the merge, but usually ask contributors to do it themselves if they're game.

Let me know if you need some tips on this, I'm happy to provide them.

@alexashley

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2017

@matthewmorgan No problem!

@matthewmorgan matthewmorgan merged commit a1e609f into exercism:master Aug 30, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewmorgan

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2017

Woohoo! Thanks @alexashley !

@alexashley alexashley deleted the alexashley:fix-assertion branch Aug 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.