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

Add Testing Style to Contributing docs #217

Merged
merged 3 commits into from
Feb 20, 2018

Conversation

paulmolluzzo
Copy link
Collaborator

What: Adds Testing Style section to CONTRIBUTING.md

Why: To make it easier for contributors to write quality tests that clearly explain the codebase.

How: Typing and copying from the comments on #207.

Checklist:

  • Has Breaking changes
  • Documentation
  • Tests
  • Ready to be merged
  • Added username to all-contributors list

@paulmolluzzo
Copy link
Collaborator Author

If anyone has more ideas of what we can/should add to this documentation, please let me know.

@coveralls
Copy link

coveralls commented Feb 16, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 7de6485 on paulmolluzzo:update-contributing-docs into 0b6384b on ViacomInc:master.

acatl
acatl previously approved these changes Feb 16, 2018
Copy link
Collaborator

@acatl acatl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is wonderful! love that we can already use our tests as examples

this is a great start! we can iterate on this as we go

@acatl
Copy link
Collaborator

acatl commented Feb 16, 2018

anyone has any ideas around test() vs it()? i like it() more, but at the same time dont feel like it should be mandatory to use one or the other?

@paulmolluzzo
Copy link
Collaborator Author

anyone has any ideas around test() vs it()?

We already have hundreds of tests using test, so to move over to it is a bit of a chore.


* Use the `describe` blocks to split up logically related tests, and write clear descriptions of what the tests relate to. (See tests in the [`base-entity`](https://github.com/ViacomInc/data-point/blob/b60824509467af599ef12d730a1b6cf8778d0b9d/packages/data-point/lib/entity-types/base-entity/resolve.test.js#L216) for an example.)
* Break up individual tests to have as few `expect`s in each test as possible. This way if a single `expect` fails for some reason, it immediately gets narrowed down to the one test that failed. Also when `jest` throws the error it'll write the description string in the console, so we'll be able to read the English description of what failed and not read any code to understand the problem. (See [these tests in `reducer-herlpers/utils`](https://github.com/ViacomInc/data-point/blob/b60824509467af599ef12d730a1b6cf8778d0b9d/packages/data-point/lib/reducer-types/reducer-helpers/utils/index.test.js#L5-L45) for an example.)
* Write simple result expectations directly in the test. Use `toMatchSnapshot()` if there is a large, complex expected result.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might also be good to mention toThrowErrorMatchingSnapshot - I've definitely found that useful when attaching custom data/messages to an error

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point, thanks!

Added a bullet below this one about writing tests for failures and mentioning toThrowErrorMatchingSnapshot as a way to do this.

@raingerber
Copy link
Contributor

I also like it because something like it('should do such and such...) reads like a full sentence. I think I've been using test more often though to match the current style. if we do try to standardize this, it might be a little annoying, but it's still just a search and replace

Copy link
Collaborator

@acatl acatl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you thanks thank you!!

@acatl acatl merged commit 756ba05 into ViacomInc:master Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants