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

Update testing guidelines, add dev container badge #4551

Merged
merged 4 commits into from
Aug 4, 2024

Conversation

jimmyli97
Copy link
Contributor

Together with #4550 updates documentation to include useful test classes and guidelines

Description

Adds information to make it easier for new contributors to contribute and write tests:

  • Codespaces uses Dev Containers so supporting Codespaces means Dev Containers running on local machines are also supported, just adding a badge so new contributors get that option
  • Link to wiki in CONTRIBUTING.md
  • Add notes on test guidelines, classes, Event Sourcing, and Continuous Integration
  • Remove reference to bundle exec rake (runs pretty much the same thing as bundle exec rspec)

Type of change

  • Documentation update

How Has This Been Tested?

Clicked Dev Container badge and opened dev container in VS Code on my local machine

* Add links to wiki
* Add extra test guidelines and helpful classes when writing tests
* Add notes on running tests with Event Sourcing
* Add notes on continuous integration
* Remove reference to bundle exec rake (it only runs default task which is spec)
* Codespaces uses Dev Containers so supporting Codespaces
  is the same as supporting Dev Containers locally
dorner
dorner previously requested changes Jul 26, 2024
Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

This looks great! I had one request if possible.

#### Guidelines
- When creating factories, in each RSpec test, hard code all values that you check with a RSpec matcher. Don't check FactoryBot default values. See [#4217](https://github.com/rubyforgood/human-essentials/issues/4217) for why.
- Write tests to pass with Event Sourcing turned both on and off, see the [Event Sourcing wiki page](https://github.com/rubyforgood/human-essentials/wiki/Event-Sourcing).
- Keep individual tests tightly scoped, only test the endpoint that you want to test. E.g. create inventory directly using `TestInventory` rather than using an additional endpoint.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a line about not adding system tests unless we actually need to test interactivity (e.g. JavaScript) and preferring request tests instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in b5e13e5

also added wiki contribution workflow and fixed dev container instructions

* Add reminder to add remote after opening dev container
* Add test guideline to prefer request specs
* Add wiki contribution workflow
Copy link
Collaborator

@awwaiid awwaiid left a comment

Choose a reason for hiding this comment

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

Looks like the other feedback was handled and this is a general improvement -- thank you!

@awwaiid awwaiid dismissed dorner’s stale review August 4, 2024 19:47

Changes applied, so closing this change-request

@awwaiid awwaiid merged commit c5fb10e into rubyforgood:main Aug 4, 2024
19 of 20 checks passed
@jimmyli97 jimmyli97 deleted the update-testing-guidelines branch August 5, 2024 00:26
Copy link
Contributor

@jimmyli97: Your PR Update testing guidelines, add dev container badge is part of today's Human Essentials production release: 2024.08.11.
Thank you very much for your contribution!

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