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 section to contribution guidelines #366

Merged
merged 2 commits into from
Oct 11, 2016
Merged

Conversation

arichardet
Copy link
Contributor

Add information on testing and including it when contributing.

@@ -2,7 +2,7 @@

Hi! Thanks for being interested in contributing to Converge. We're really happy
to have you (we have mini-parties in Slack whenever someone external opens a
PR!)
PR!).
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need the additional trailing periods here and in the contribution requirements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated: left the period for the fragment, and removed from the complete sentences.

We want to be sure of a few things, and testing will help us accomplish this:

- Help ensure a new feature or changes to an existing feature are properly
implemented.
Copy link
Contributor

Choose a reason for hiding this comment

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

adding two spaces before this line will fix code climate complaining


- Include tests for the way your code interacts with the core engine.
- Test functionality of the code you have introduced, say in a new module.
- Mock interaction with the system.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not always true. In fact, I'd say we should avoid mocking when possible. Too easy to get out of sync between mocks and reality.

Copy link
Contributor

Choose a reason for hiding this comment

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

This list should also include something like "tests that demonstrate a bug, and that the bug has been resolved."

Copy link
Contributor

Choose a reason for hiding this comment

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

We were talking about this a bit in the office. I think we should avoid mocking whenw e can, but prefer mocking over having untested/untestable code (e.g. when the code is only available on one platform)- but we should probably have further discussion about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does this sound: "Typically, refrain from mocking and intentionally exercise the code. In few cases, mocking interaction with the system may be useful; however, avoid it if testing can be accomplished by other means."

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we're going to tell people "you shouldn't mock, except when you should, but usually not" it's probably better to just not mention it at all and leave it at the fact that we should have tests for things.


##### Our Testing Goals

We want to be sure of a few things, and testing will help us accomplish this:
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove "help us accomplish this" and get the same meaning.

@@ -71,7 +95,7 @@ harder to review. Pretty please?
## Code of Conduct

Participation in the development process of Converge is subject to
the [code of conduct](CODE_OF_CONDUCT.md). Please familiarise yourself with that
the [code of conduct](CODE_OF_CONDUCT.md). Please familiarize yourself with that
Copy link
Contributor

@BrianHicks BrianHicks Oct 11, 2016

Choose a reason for hiding this comment

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

🇬🇧

@arichardet arichardet merged commit 84f22da into master Oct 11, 2016
@arichardet arichardet deleted the docs/contributing branch October 11, 2016 20:12
BrianHicks pushed a commit that referenced this pull request Dec 22, 2016
Add testing section to contribution guidelines
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.

3 participants