Hello,
Earlier today I have slightly changed our review / test workflow on mediawiki/core.git . That will let us do more tests and scale things in the long term.
The new workflow is described on the wiki (including a flowchart):
https://www.mediawiki.org/wiki/Continuous_integration/Workflow
How it works?
Whenever a patch is submitted, Jenkins will attempt to merge the patchset against the latest master and abort whenever it fails asking for the submitter to rebase the patchset.
Jenkins will then verify the PHP files are valid and run JSHint for the Javascript files.
If everything is fine, jenkins-bot will vote Code-Review +1 to let everyone know that the change look fine. Else, it will vote Verified -1 preventing the patchset from being submitted.
That is all. The slow unit tests are no more run on patchset submission.
To get the test to run, the patch will have to be reviewed first. Whenever someone vote CodeReview +2 Jenkins will run the Unit tests, then if: - tests are successful, jenkins-bot will vote Verified+1 which let us merge the patch. - tests are failing, jenkins-bot vote Verified-1 AND CodeReview -2 to prevent the patch from being merged.
If all works fine tonight, I will configure jenkins-bot so it merges the Change automatically whenever the test are successful.
That workflow has been applied to a few extensions this week. I will add it to more and more extensions and eventually will get all extensions to receive PHP Linting and a JSHint run.
Antoine Musso hashar+wmf@free.fr wrote:
Earlier today I have slightly changed our review / test workflow on mediawiki/core.git . That will let us do more tests and scale things in the long term.
The new workflow is described on the wiki (including a flowchart):
https://www.mediawiki.org/wiki/Continuous_integration/Workflow
How it works?
Whenever a patch is submitted, Jenkins will attempt to merge the patchset against the latest master and abort whenever it fails asking for the submitter to rebase the patchset.
Jenkins will then verify the PHP files are valid and run JSHint for the Javascript files.
If everything is fine, jenkins-bot will vote Code-Review +1 to let everyone know that the change look fine. Else, it will vote Verified -1 preventing the patchset from being submitted.
That is all. The slow unit tests are no more run on patchset submission.
[...]
I'm with Erik on this one who posted on the talk page in March:
| I understand the concern, but if we're creating a world | where tests have to wait for developer review, we're doing | CI the wrong way around. The whole point of automated | testing is to minimize the need for human review, so we | should aim to run as many tests as possible as early as | possible. Both test performance and security considerations | are problems to be gradually resolved in aiming for highest | possible test execution for all code that gets submitted, | and minimizing the need for human review on code which is | obviously broken.
Why not just add (more) slaves? Computing power is much cheaper than developer time.
Tim
On Thu, Dec 6, 2012 at 10:52 PM, Tim Landscheidt tim@tim-landscheidt.de wrote:
That is all. The slow unit tests are no more run on patchset submission.
We really need them.
The tests philosophy is "there is an issue with this change".
The Jenkins philosophy is "automate as most as possible to discover them as soon as possible".
Furthermore, one of the current Wikimedia politics is "get more technical contributors" and do outreach tout azimut. The tests are really important there, as they don't know each particularities of each class of the code (and I'm including myself in this area, there are MediaWiki classes I haven't get the opportunity to read yet).
Should I really put logical constructors between these facts or the "we really need tests to run" is evident for all?
[...]
I'm with Erik on this one who posted on the talk page in March:
| I understand the concern, but if we're creating a world | where tests have to wait for developer review, we're doing | CI the wrong way around. The whole point of automated | testing is to minimize the need for human review, so we | should aim to run as many tests as possible as early as | possible. Both test performance and security considerations | are problems to be gradually resolved in aiming for highest | possible test execution for all code that gets submitted, | and minimizing the need for human review on code which is | obviously broken.
Why not just add (more) slaves? Computing power is much cheaper than developer time.
I absolutely agree.
On 06/12/2012 15:01, Sébastien Santoro wrote:
On Thu, Dec 6, 2012 at 10:52 PM, Tim Landscheidt tim@tim-landscheidt.de wrote:
That is all. The slow unit tests are no more run on patchset submission.
We really need them.
The tests philosophy is "there is an issue with this change".
The Jenkins philosophy is "automate as most as possible to discover them as soon as possible".
Furthermore, one of the current Wikimedia politics is "get more technical contributors" and do outreach tout azimut. The tests are really important there, as they don't know each particularities of each class of the code (and I'm including myself in this area, there are MediaWiki classes I haven't get the opportunity to read yet).
Should I really put logical constructors between these facts or the "we really need tests to run" is evident for all?
It should be - as a volunteer it can be quite difficult to find someone to review a change, and that is after it already passes the basic tests. Asking folks to review things when we don't even know if it passes the basics is not going to make the process go any easier or faster, and having to wait even longer and poke even harder is the sort of thing that drives people to give up on the entire process.
| I understand the concern, but if we're creating a world | where tests have to wait for developer review, we're doing | CI the wrong way around. The whole point of automated | testing is to minimize the need for human review, so we | should aim to run as many tests as possible as early as | possible. Both test performance and security considerations | are problems to be gradually resolved in aiming for highest | possible test execution for all code that gets submitted, | and minimizing the need for human review on code which is | obviously broken.
Why not just add (more) slaves? Computing power is much cheaper than developer time.
I absolutely agree.
I'm also in agreement on this one. Having tests run after code review negates the point of having tests run in the first place.
Thank you, Derric Atzrott
On Fri, Dec 7, 2012 at 1:58 PM, Derric Atzrott <datzrott@alizeepathology.com
wrote:
Why not just add (more) slaves? Computing power is much cheaper than developer time.
I absolutely agree.
I'm also in agreement on this one. Having tests run after code review negates the point of having tests run in the first place.
I also fully agree.
I would also like to point out that this new workflow requires unit tests to all pass out of the box, but they don't -- at least last time I checked.
Bryan
I'd suggest reverting this as well. It makes your dashboard basically useless when all of your patches are +1'd, and all of them by a bot.
-- Matma Rex
Le 06/12/12 22:52, Tim Landscheidt wrote:
Why not just add (more) slaves? Computing power is much cheaper than developer time.
Erik, everyone and I are eager to add more computing power. We are going to use Vagrant virtual machines setup as Jenkins slaves. It takes a bit more engineering to achieve that though.
Ori has already created a wonderful Vagrant image which is published at https://github.com/atdt/wmf-vagrant Next step is to make it fetch the submitted changes and act as a Jenkins slave.
On Thu, Dec 6, 2012 at 7:19 PM, Antoine Musso hashar+wmf@free.fr wrote:
(...) If everything is fine, jenkins-bot will vote Code-Review +1 to let everyone know that the change look fine. Else, it will vote Verified -1 preventing the patchset from being submitted.
Could you, pending the end of this discussion, remove this CR +1?
This is very disturbing when browsing Gerrit for revisions still to review and doesn't really give any useful information (as the important information here would be the verified -1).
Le 07/12/12 04:56, Sébastien Santoro wrote <snip>
This is very disturbing when browsing Gerrit for revisions still to review and doesn't really give any useful information (as the important information here would be the verified -1).
Lets open the can of worm here :)
Chad came up with another idea that would be to rename Verified to Automatic Tests and extends it to a range of -2 .. +2 where +1 will be lint ok and +2 unit tests ok. Not sure how feasible it is to tweak Gerrit this way though.
On Fri, Dec 7, 2012 at 4:19 PM, Antoine Musso hashar+wmf@free.fr wrote:
Le 07/12/12 04:56, Sébastien Santoro wrote
<snip> > This is very disturbing when browsing Gerrit for revisions still to > review and doesn't really give any useful information (as the > important information here would be the verified -1).
Lets open the can of worm here :)
Chad came up with another idea that would be to rename Verified to Automatic Tests and extends it to a range of -2 .. +2 where +1 will be lint ok and +2 unit tests ok. Not sure how feasible it is to tweak Gerrit this way though.
It shouldn't be hard--just two SQL queries to adjust the review labels
-Chad
On 12/07/2012 04:19 PM, Antoine Musso wrote:
Le 07/12/12 04:56, Sébastien Santoro wrote
<snip> > This is very disturbing when browsing Gerrit for revisions still to > review and doesn't really give any useful information (as the > important information here would be the verified -1).
Lets open the can of worm here :)
Chad came up with another idea that would be to rename Verified to Automatic Tests and extends it to a range of -2 .. +2 where +1 will be lint ok and +2 unit tests ok
Should there be a separate label for human testing and verification (as opposed to Code Review, which could just mean "after visual review, the code looks right")?
Matt Flaschen
Le 06/12/12 19:19, Antoine Musso a écrit :
If all works fine tonight, I will configure jenkins-bot so it merges the Change automatically whenever the test are successful.
Jenkins now automatically merge changes made to mediawiki/core.git that have received CR+2 and pass all tests. The related Zuul change is:
https://gerrit.wikimedia.org/r/#/c/37420/
So we now just have to CR+2 and comment and the rest happens magically.
wikitech-l@lists.wikimedia.org