Automated code review
Moodle issues submitted for review and inclusion into core are examined by our continuous integration (ci) system and checked against various automated tests. We use the automated checks to help improve the code quality in Moodle.
- 1 What is CiBot?
- 2 What checks is CiBot carrying out on each issue?
- 2.1 Will an issue be automatically rejected if cibot checks fail?
- 2.2 Should coding style issues in existing code be fixed?
- 2.3 Requesting CiBot checks an issue
- 2.4 Are additional CiBot checks possible?
- 2.5 The git commit summary limit is too small
- 2.6 See also
What is CiBot?
CiBot is our automated code checker and it will run checks against any issues waiting for peer review, sent for integration or requested to be checked by the developer. It will report problems discovered in lines of your patch which you are changing.
What checks is CiBot carrying out on each issue?
- Verify mergeability, number of commits and branch ages.
- Require testing instructions.
- Automated coding style checks using local_codechecker.
- Automated PHPDoc checks using local_moodlecheck.
- Automated commit message checks using these scripts.
- Automated upgrade savepoint checks using these scripts.
Will an issue be automatically rejected if cibot checks fail?
No. Developers should strive to have cibot approve every patch submitted, but the integration team will take a pragmatic approach to things like:
- Occasions where cibot will detect issues present in existing code (see next item)
- Variations from commit message style which make the commit message more understandable overall
Should coding style issues in existing code be fixed?
In general, please only update lines relating to your own change. Policy for this situation is being discussed in MDL-43233.
Spacing of if statements is incorrect on the line being changed
This can be corrected without affecting existing code, so should be fixed.
The line being changed contains an invalid variable name
If correcting this variable would affect other parts of the code not covered by the patch then its not reasonable to fix it.
Requesting CiBot checks an issue
At any time a developer can add the label 'cime' to an issue to request it runs it checks against it. The bot checks for issues with the cime label every 30mins and runs the checks then remove the cime label. (Note that because it removes the label, it is normal to 'create' the label).
Are additional CiBot checks possible?
Yes. It is planned to add more checks - (see MDLSITE-3267) and developers are encouraged to suggest new checks in that issue. Note that some limitations exist:
- The checks should be consistent and completely reproducible
- The check should complete in minutes not hours (other solutions are in planning for longer term tests)
The git commit summary limit is too small
When you overrun the length limit many git tools do not display commits well. See how github truncates ea6f548081 - it ruins the message and makes it harder for you to communicate your change to other developers.
It is acknowledged that its often tricky to get a useful message in such a short space on the first line. However, the coding guidelines for git summary line length were established on the basis of industry best practice. As the Coding_style#Git_commits mentions, do not be afraid to go into much more detail in your commit body.Try
git log --oneline --no-mergesif you want to see how other developers have tried to adapt to this situation.