Re: [Wikitech-l] Gerrit code review guidelines

[Juliusz Gonera]

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  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 mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Krinkle]

On Dec 27, 2012, at 7:18 PM, Juliusz Gonera  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

Re: [Wikitech-l] Gerrit code review guidelines

[Quim Gil]

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!


--
Quim Gil
Technical Contributor Coordinator @ Wikimedia Foundation
http://www.mediawiki.org/wiki/User:Qgil

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Matma Rex]

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.

--
Matma Rex

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Matthew Flaschen]

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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Arthur Richards]

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 <
mflasc...@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.

-- 
Matma Rex

__**_
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/**mailman/listinfo/wikitech-l
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Matma Rex]

On Fri, 28 Dec 2012 22:27:36 +0100, Matthew Flaschen  
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.

--
Matma Rex

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Matthew Flaschen]

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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Matthew Flaschen]

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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Brad Jorsch]

On Fri, Dec 28, 2012 at 12:58 PM, Juliusz Gonera  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.

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Brad Jorsch]

On Fri, Dec 28, 2012 at 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 ;)

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.

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Juliusz Gonera]

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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Juliusz Gonera]

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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Alex Monk]

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
wrote:

> 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
>
___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Re: [Wikitech-l] Gerrit code review guidelines

[Matthew Flaschen]

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

[Wikitech-l] Gerrit code review guidelines

[Juliusz Gonera]

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

___
Wikitech-l mailing list
Wikitech-l@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/wikitech-l