Hi,
I'm a bit confused when it comes to various options I have in gerrit and it seems the docs are not up to date on that.
* What is the difference between Verified and Code Review? When would I put +1 in one of them but -1 in the other? * What is the difference between +1 and +2, especially in Verified? * Why do we even have +2? +1 means that someone else must approve. What does +2 mean? No one else has to approve but I'm not merging anyway, why?
It seems the docs (http://www.mediawiki.org/wiki/Code_review_guide) do not explain it.
Juliusz
On 12/27/2012 01:18 PM, Juliusz Gonera wrote:
- What is the difference between Verified and Code Review? When would I
put +1 in one of them but -1 in the other?
I'm relatively new, but this is my understanding. Verified means you actually tested it. Code Review means it "looks good"
- What is the difference between +1 and +2, especially in Verified?
I think just how certain you are.
- Why do we even have +2? +1 means that someone else must approve. What
does +2 mean? No one else has to approve but I'm not merging anyway, why?
+2 means it's ready to merge. In core, this will cause unit tests to run, and if they pass, it will automatically merge.
I don't know of any reason (in any code) to vote CR +2 if you don't think it's ready to merge.
Matt Flaschen
To add to this:
Jenkins adds Verified+1 if the lint tests pass, or Verified-1 if they fail. If the unit tests pass (If you're not on the whitelist, this will be once someone gives it CodeReview+2) it will give the change Verified+2 or Verified-2 if they fail.
Only some people (project owners, gerrit admins, some WMF staff, etc.) can give CodeReview+2 (approved), whereas everyone can give CodeReview+1. Only people able to approve can mess with Verified I think...
Alex
On Thu, Dec 27, 2012 at 6:31 PM, Matthew Flaschen mflaschen@wikimedia.orgwrote:
On 12/27/2012 01:18 PM, Juliusz Gonera wrote:
- What is the difference between Verified and Code Review? When would I
put +1 in one of them but -1 in the other?
I'm relatively new, but this is my understanding. Verified means you actually tested it. Code Review means it "looks good"
- What is the difference between +1 and +2, especially in Verified?
I think just how certain you are.
- Why do we even have +2? +1 means that someone else must approve. What
does +2 mean? No one else has to approve but I'm not merging anyway, why?
+2 means it's ready to merge. In core, this will cause unit tests to run, and if they pass, it will automatically merge.
I don't know of any reason (in any code) to vote CR +2 if you don't think it's ready to merge.
Matt Flaschen
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 12/27/2012 10:36 AM, Alex Monk wrote:
Only some people (project owners, gerrit admins, some WMF staff, etc.) can give CodeReview+2 (approved), whereas everyone can give CodeReview+1. Only people able to approve can mess with Verified I think...
But should we mess with Verified? Or should we just leave it to Jenkins?
Juliusz
On Fri, Dec 28, 2012 at 12:58 PM, Juliusz Gonera jgonera@wikimedia.org wrote:
On 12/27/2012 10:36 AM, Alex Monk wrote:
Only some people (project owners, gerrit admins, some WMF staff, etc.) can give CodeReview+2 (approved), whereas everyone can give CodeReview+1. Only people able to approve can mess with Verified I think...
But should we mess with Verified? Or should we just leave it to Jenkins?
IMO leave it to Jenkins, if Jenkins is set up for the project. But if you want to use it to indicate that you ran the unit tests yourself and/or extensively tested it manually, feel free I guess as long as it doesn't confuse Jenkins.
On 12/28/2012 02:09 PM, Brad Jorsch wrote:
IMO leave it to Jenkins, if Jenkins is set up for the project. But if you want to use it to indicate that you ran the unit tests yourself and/or extensively tested it manually, feel free I guess as long as it doesn't confuse Jenkins.
My understanding is you *should* use it. Jenkins only does V+1, which is "Checked". V+2 is required for some/all repos (e.g. extensions). Normally, the original developer at least should provide Verified (since they should have tested their own code).
Matt Flaschen
On Fri, 28 Dec 2012 22:27:36 +0100, Matthew Flaschen mflaschen@wikimedia.org wrote:
My understanding is you *should* use it. Jenkins only does V+1, which is "Checked". V+2 is required for some/all repos (e.g. extensions). Normally, the original developer at least should provide Verified (since they should have tested their own code).
Jenkins does both: V+1 is for lint check, V+2 is for unit tests.
Thanks everyone for helping to clarify. The fact that there is still confusion, uncertainty, and 'from my understating's though is disconcerting. Can someone in the know document the actual implications/guidelines/automated behavior/etc for this? And can someone take responsibility for updating the documentation as this stuff evolves? We should be able to refer ourselves and others to a definitive resource when questions like this arise. On Fri, 28 Dec 2012 22:27:36 +0100, Matthew Flaschen < mflaschen@wikimedia.org> wrote:
My understanding is you *should* use it. Jenkins only does V+1, which is "Checked". V+2 is required for some/all repos (e.g. extensions). Normally, the original developer at least should provide Verified (since they should have tested their own code).
Jenkins does both: V+1 is for lint check, V+2 is for unit tests.
On 12/29/2012 01:06 AM, Arthur Richards wrote:
Thanks everyone for helping to clarify. The fact that there is still confusion, uncertainty, and 'from my understating's though is disconcerting. Can someone in the know document the actual implications/guidelines/automated behavior/etc for this? And can someone take responsibility for updating the documentation as this stuff evolves? We should be able to refer ourselves and others to a definitive resource when questions like this arise.
Be bold! The definitive source is https://www.mediawiki.org/wiki/Git/Workflow#How_we_review_code , but there are some parts there that is obsolete. Don't wait for an expert to come along and document everything. You can help too.
Matt Flaschen
I think that nobody bothered with documenting because the process itself is greatly in flux now. People are e.g. working on sandboxing the unit tests, so they can be safely run on patchset submission, so jenkins could just use one Verified level after running all tests.
On 12/28/2012 10:27 PM, Matthew Flaschen wrote:
On 12/29/2012 01:06 AM, Arthur Richards wrote:
Thanks everyone for helping to clarify. The fact that there is still confusion, uncertainty, and 'from my understating's though is disconcerting. Can someone in the know document the actual implications/guidelines/automated behavior/etc for this? And can someone take responsibility for updating the documentation as this stuff evolves? We should be able to refer ourselves and others to a definitive resource when questions like this arise.
Be bold! The definitive source is https://www.mediawiki.org/wiki/Git/Workflow#How_we_review_code , but there are some parts there that is obsolete. Don't wait for an expert to come along and document everything. You can help too.
Along these lines, let me raise the attention for
Bug 36437 - A strict and correct Git workflow document is needed https://bugzilla.wikimedia.org/show_bug.cgi?id=36437
and the effort that Dan has offered to lead to get this fixed:
http://www.mediawiki.org/wiki/User:Dan-nl/Git_and_Gerrit
Please help Dan and help yourselves by commenting his proposals and editing in any way that improves the current situation. Thank you!
On 12/27/2012 10:31 AM, Matthew Flaschen wrote:
- What is the difference between +1 and +2, especially in Verified?
I think just how certain you are.
I still don't get it. I either think the code is good and should be merged or it's not good enough and shouldn't be merged. I don't see any situations in between, but maybe it's just me ;)
+2 means it's ready to merge. In core, this will cause unit tests to run, and if they pass, it will automatically merge.
I don't know of any reason (in any code) to vote CR +2 if you don't think it's ready to merge.
This still seems redundant. If Jenkins runs tests only when I give +2, but I am supposed to give +2 only if I already run the tests myself manually, then what's the point?
And I guess we don't have that auto-merging behavior in mobile.
Juliusz
On Fri, Dec 28, 2012 at 12:57 PM, Juliusz Gonera jgonera@wikimedia.org wrote:
On 12/27/2012 10:31 AM, Matthew Flaschen wrote:
- What is the difference between +1 and +2, especially in Verified?
I think just how certain you are.
I still don't get it. I either think the code is good and should be merged or it's not good enough and shouldn't be merged. I don't see any situations in between, but maybe it's just me ;)
In Verified, it's whether Jenkins ran only the linting tests or also the unit tests.
In Code Review for people who have +2 access, marking +1 typically means "I think it's good, but I'd like others to take a look", while marking it +2 typically means "Hey Jenkins, run the final unit tests and then merge this!".
This still seems redundant. If Jenkins runs tests only when I give +2, but I am supposed to give +2 only if I already run the tests myself manually, then what's the point?
Once things are set up to do it safely, Jenkins will start running unit tests again when the new patch set is initially uploaded.
On 12/28/2012 12:57 PM, Juliusz Gonera wrote:
On 12/27/2012 10:31 AM, Matthew Flaschen wrote:
- What is the difference between +1 and +2, especially in Verified?
I think just how certain you are.
I still don't get it. I either think the code is good and should be merged or it's not good enough and shouldn't be merged. I don't see any situations in between, but maybe it's just me ;)
Okay, some people (depending on permissions) can't vote +2, in which case it's easy.
But if you can vote either, it is somewhat of a weird gut thing. +2 means you're sure it's ready to merge.
+2 means it's ready to merge. In core, this will cause unit tests to run, and if they pass, it will automatically merge.
I don't know of any reason (in any code) to vote CR +2 if you don't think it's ready to merge.
This still seems redundant. If Jenkins runs tests only when I give +2, but I am supposed to give +2 only if I already run the tests myself manually, then what's the point?
No, you (people) do not have to test to do CR +2 (though if you do, you get bonus points). V (Verified) means you actually tested (manually or automatically, depending on project).
Also, Jenkins currently only does this "CR +2 and passing tests" == "automatic merge" in core. In other projects, people have to merge manually, but CR +2 should still be a pre-requisite.
Matt Flaschen
On Dec 27, 2012, at 7:18 PM, Juliusz Gonera jgonera@wikimedia.org wrote:
Hi,
I'm a bit confused when it comes to various options I have in gerrit and it seems the docs are not up to date on that.
- What is the difference between Verified and Code Review? When would I put +1 in one of them but -1 in the other?
- What is the difference between +1 and +2, especially in Verified?
- Why do we even have +2? +1 means that someone else must approve. What does +2 mean? No one else has to approve but I'm not merging anyway, why?
It seems the docs (http://www.mediawiki.org/wiki/Code_review_guide) do not explain it.
Juliusz
== Verified ==
Verified is for linting and executing unit tests. * Verified +1 means "Checked" for lint issues * Verified +2 means "Tested" by executing the Jenkins tests
If you are a human, do not use Verified (most people don't have user permissions to set this, anyway).
If you "tested" the change (either by checking it out locally and using the wiki and/or by running phpunit locally) and you have the ability to set Verified, still do not set it.
It does not mean the same thing. Because the Jenkins tests are much more elaborate than the testing you may do locally (e.g. different database backends, and soon also different browsers and test suites: jshint, phplint, phpunit, qunit, mysql, sqlite etc.).
We might rename this field to "Automated Testing" for clarification ("Verified" is a generic name that is the default in Gerrit) and (where not already) it will eventually be restricted to bots only[2].
== Code Review ==
Code Review is for human review of the code.
A positive value means you believe it is perfect and may be merged as-is (under the condition that the Jenkins test will pass[1]). If you have merge rights then you'd give +2. Others would give +1.
A negative value means there are issues.
[1] So if you set CR+2 you're saying "The code looks great, let Jenkins run it, and if it passes, merge it". [2] Except for wmf-deployment probably, to be able to override it if Jenkins is having issues and certain commits need emergency deployment.
Thanks, that clears up my doubts. I'll just assume that for now, as long as we don't have automatic merging in mobile, I should just merge myself instead of giving +2.
This information should definitely be included in one of the documents Dan is writing (https://bugzilla.wikimedia.org/show_bug.cgi?id=36437).
On 12/31/2012 10:39 AM, Krinkle wrote:
On Dec 27, 2012, at 7:18 PM, Juliusz Gonera jgonera@wikimedia.org wrote:
Hi,
I'm a bit confused when it comes to various options I have in gerrit and it seems the docs are not up to date on that.
- What is the difference between Verified and Code Review? When would I put +1 in one of them but -1 in the other?
- What is the difference between +1 and +2, especially in Verified?
- Why do we even have +2? +1 means that someone else must approve. What does +2 mean? No one else has to approve but I'm not merging anyway, why?
It seems the docs (http://www.mediawiki.org/wiki/Code_review_guide) do not explain it.
Juliusz
== Verified ==
Verified is for linting and executing unit tests.
- Verified +1 means "Checked" for lint issues
- Verified +2 means "Tested" by executing the Jenkins tests
If you are a human, do not use Verified (most people don't have user permissions to set this, anyway).
If you "tested" the change (either by checking it out locally and using the wiki and/or by running phpunit locally) and you have the ability to set Verified, still do not set it.
It does not mean the same thing. Because the Jenkins tests are much more elaborate than the testing you may do locally (e.g. different database backends, and soon also different browsers and test suites: jshint, phplint, phpunit, qunit, mysql, sqlite etc.).
We might rename this field to "Automated Testing" for clarification ("Verified" is a generic name that is the default in Gerrit) and (where not already) it will eventually be restricted to bots only[2].
== Code Review ==
Code Review is for human review of the code.
A positive value means you believe it is perfect and may be merged as-is (under the condition that the Jenkins test will pass[1]). If you have merge rights then you'd give +2. Others would give +1.
A negative value means there are issues.
[1] So if you set CR+2 you're saying "The code looks great, let Jenkins run it, and if it passes, merge it". [2] Except for wmf-deployment probably, to be able to override it if Jenkins is having issues and certain commits need emergency deployment.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
wikitech-l@lists.wikimedia.org