Improve Pull-Request Process

[dkroenke]

Brief feature description

In iceoryx are Pull-Requests often a long time open because of several inconsistencies:

• New contributors often don't know who is the committer with the best knowledge about the codebase where he is working on
• There are formal aspects expected from committer side which are not visible to the contributors (e.g. AAA Pattern in tests, which cases are needed to test? How to name tests properly?)
• The expectations regarding the scope of a PR are sometimes unclear (e.g. When is a PR too huge? What is expected on tests?)

Of course a Pull-Request is something iterative and the last thing we want is to have fast merges without reviewing it accordingly.
For more efficient Pull-Request reviews we need to improve the Contribution Process and give more information about it.

In the Contributing.md are already helpful hints listed: https://github.com/eclipse-iceoryx/iceoryx/blob/master/CONTRIBUTING.md#coding-style
When i take a look at https://github.com/eclipse-iceoryx/iceoryx/blob/master/CONTRIBUTING.md#testing i see not much info here, this can be a starting point to extend it.

Possible solutions (to be extended):

• extend the Contributor instructions with more hints
• add codeowners to iceoryx components which are automatically added to the Pull-Request. https://docs.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners
• more automatic checks at Pull-Request like if all points on the PR checklist are marked
• add a PR Merge-Bot which ensures syncing with master and do automatic checks
• add labels to PR template so that Contributors can set them
• enable annotations for codecov.io

Codecov:

• checkout why the coverage is flacky
• exclude non-testable parts like the posix-wrapper which uses os-related functions
• rework the annotations

Maybe we can have here a starting point with a checklist (unordered and incomplete):

• build everything and execute tests: ./tools/iceoryx_build_test.sh build-strict build-all out-of-tree test clean
• are unittests complete? Link
• documentation in doxygen complete? Link
• clang-format the code?
• naming correct? See: https://github.com/eclipse-iceoryx/iceoryx/blob/master/CONTRIBUTING.md#naming-conventions
• fileheader correct? (company)
• use const & in function arguments wherever possible
• avoid using fundamental types like int, use fixed width integer types like uint32_t
• always initialize member variables and use uppercase literals if possible uint32_t foo{0};
• no commented code
• Are the labels in the PR properly set? (E.g. test, refactoring etc.)
• ...

checklist for tests in Contributing.md(incomplete):

• use AAA pattern for testcase structure https://medium.com/@pjbgf/title-testing-code-ocd-and-the-aaa-pattern-df453975ab80
• use sut as member name for the object to test
• prefer to use stack-allocated objects for testing, if only heap is possible then wrap them in smart-pointers to ensure correct cleanup and avoid memleaks
• create a meaningful name for your testcase, e.g. TEST_F(MemoryManager_test, PerformSomethingAndExpectThis)
• don't test multiple aspects in one testcase, prefer to split up in different little testcases
• avoid usage of new in tests as much as possible because you need to delete it afterwards
• test negative cases too
• possible abstract testcases: zero, one, many, corner cases, limits
• ...

Feel free to make proposals and extend the checklist.
@eclipse-iceoryx/technology-iceoryx-committers

[elBoberido]

* [ ]  avoid usage of `new` in tests as much as possible because you need to `delete` it afterwards

@dkroenke how about phrasing this like use only smart pointer in units tests and "new/delete" only if there is no other possibility

[FerdinandSpitzschnueffler]

We could add following rule of thumb for the unit tests (probably in the contributing.md, not in the checklist):

Try to test following abstract cases:

• zero
• one
• many
• corner cases
• limits

[dkroenke]

how about phrasing this like use only smart pointer in units tests and "new/delete" only if there is no other possibility

This reads somehow like that we always need to use smart pointers in tests, which we don't need for stack-objects.
Maybe something like this:

prefer to use stack-allocated objects for testing, if only heap is possible then wrap them in smart-pointers to ensure correct cleanup and avoid memleaks.

[dkroenke]

Another question, there is the possibility to prevent Committers from merging PRs without having all checkboxes marked as resolved.
It also prevents merging when someone has commented a checkbox in the PR which is not resolved.
Would this help you?

[mossmaurice]

Another question, there is the possibility to prevent Committers from merging PRs without having all checkboxes marked as resolved.
It also prevents merging when someone has commented a checkbox in the PR which is not resolved.
Would this help you?

Yeah, I think that would be great. If we see that this leads to problems at some point we can refine the check again.

[dkroenke]

Request for additional PR check for all checkboxes marked: https://bugs.eclipse.org/bugs/show_bug.cgi?id=570680

[dkroenke]

For now i would close this ticket due to the fact that we improved the PR-Template, Contributing.md and Testing Guidelines.