Strategies for improving integration testing: order of operations of testing
Background
Many members of the development team report frustration with the workflow experience related to integration testing. The key pain point is broken code being merged to dev prior to the automated run of integration tests. In short: unit tests pass and allow the merge, then integration tests fail after broken code is in dev.
Current system | Proposed new system | |
---|---|---|
Order of testing in CI related to PR activity |
| |
Pro | ||
Con |
|
Discussion
Please add your thoughts, experiences, and recommendations to this section. Select edit to add content.
Your thoughts and experiences
Your name | Your thoughts and experiences |
---|---|
Kelly Foster | I heard Adam and Tomáš mention a security risk of running both unit and integration tests at the opening of a PR. I don't understand the risk or the significance of taking on this risk. |
Łukasz Gołębiewski | We need to make sure that integration tests are run prior to merging to dev. This process could be semi-automatic and we would just need to make sure that the last (and not every) commit of a PR is tested. Once a PR has been reviewed and the reviewer has verified that no one is trying to exploit the vulnerabilities of our CI infrastructure, they could start the integration tests by some simple means, e.g. by commenting on github or by adding a label to the issue. A green integration tests report for a PR would be a prerequisite for merging |
Artur Gajowy | I understand the risk, but don't deem it significant or worth mitigating, even without additional precautions. I believe having the i-tests run only for the members of https://github.com/rchain-drone/ would be more than enough. |
Artur Gajowy | Why do we need the privileged mode for docker containers anyway? To package the current code into a docker container? How I see it, the container should be done in a way that allows us to "upgrade" it with newer code by simply mounting a volume with the updated code. We don't have to build or upload any docker artifacts in the PR build. Doesn't that invalidate the 'docker.sock' issue? |
Artur Gajowy | Running i-tests on every push to a PR seems wasteful (e.g. I do many pushes per PR, often within minutes). So maybe it should be done on demand. The 'demand' should also automatically include 'ok, I want to merge this'. |
Artur Gajowy | Because all of the above being solved/worked around by bors, the semantic conflict issue we haven't touched upon yet, and because of how bors optimizes the number of tests runs used to merge a set of PRs I recommend going with bors. |
Tomáš Virtus | https://www.projectatomic.io/blog/2015/08/why-we-dont-let-non-root-users-run-docker-in-centos-fedora-or-rhel/ |
Recommendations for changes
Your name | Change recommendation | +/- (feedback from others) |
---|---|---|
Kelly Foster | I'd like to see both unit and integration tests run via ci before a PR merge. Reasons we didn't do this in the past related to ability of Travis alone and later Travis + GitLab to support this. I think Drone can support this now. | |
Adam | Introduce https://github.com/bors-ng/bors-ng or https://github.com/palantir/bulldozer and let them manage testing and merging instead of relying on GitHub. | I've reviewed both of their docs, and only bors mentions a sufficient feature set. And is much better documented. |
Artur Gajowy | Try setting up bors using their public instance (should be all we ever need). Just follow "setting up bors". Have the build for "staging" and "trying" branches include i-tests. Don't limit push access to bors only until we're happy with the integration. Document that we'll be using "bors delegate+" instead of "bors r+" most of the time. See reference. Include all developers as reviewers in bors settings. Make a POC on a fork/different repo first and consider if bors can push to non-master branches (we need dev). | |
Tomáš Virtus | https://github.com/opencontainers/runc#rootless-containers https://github.com/genuinetools/img/ | |
Action items
- Adam Szkoda to implement in Core sprint 21
Proposal
- Implement bors to support the following process:
- PR opened (run compilation and unit tests)
- Run tests at every push to PR
- PR review
- Run of all tests (unit+integration) at approval of PR (via comment on GitHub)
- Merge blocked until tests pass (we think merge is automated by bors)
- PR opened (run compilation and unit tests)