akuhlens
commented
5 years ago - I think a lot of our focus should be on having obvious unit testing in any new / modified development.
- Any modification of a function needs to add the unit tests
- Any removal or modification of a previous unit test should be examined closely (by a reviewer)
- The default test requirements for a PR should be to run the integration tests and all the unit test.
- It would be nice if the manually running the benchmarks wasn't needed
- And is probably worth saving time on most small pull requests by not requiring the benchmarks to finish before accepting a PR. (We will usually be able to reset a pull request)
- I think there should be some sort of standardization of reviews based on the scale of the change.
- Admins on small pull requests don't need review (but still must PR) and only require an informal
review for non-admin. - Medium PRs can be quickly reviewed (based on tests, comments, and style)
- Large PRs must be understood as whole
- The difference between small, medium, and large should not just be code size
- Admins on small pull requests don't need review (but still must PR) and only require an informal
- Do we have a style guide yet?
In an untyped world where bugs are looming over, we better revisit our engineering processes. My thoughts are to enforce successful completion of our test suite as a requirement before merging, perhaps throw in our benchmarks too? looking for performance improvements/bugs, and perhaps add code coverage checker. I also think we should protect master, we can discuss if this is also required for admins. The end goal of this issue is to come up with new checklist of requirements on PRs.