Re: [admin] Testing and GitHub login names
Robin: The only thing that we ask is that pull requests not be merged by whoever made the request. Art: If a test facilitator submits tests (i.e. makes a PR) and everyone that reviews them says they are OK, it seems like the facilitator should be able to do the merge. Robin: Of course. OK, good (and presumably this means all of WebApps' test facilitators that will submit tests need appropriate permissions to run merges for at least their repo(s)). -AB
Critic [was: Re: [admin] Testing and GitHub login names]
On 04/23/2013 08:43 AM, Robin Berjon wrote: On 22/04/2013 13:12 , James Graham wrote: (as an aside, I note that critic does a much better job here. It allows reviewers to mark when they have completed reviewing each file in each commit. It also records exactly how each issue raised was resolved, either by the commit that fixed it or by the person that decided to mark the issue as resolved) You may wish to introduce Critic a bit more than that; I'm pretty sure that many of the bystanders in this conversation aren't conversant with it. Right, I lost track of which list this conversation was on :) "Opera Critic", commonly known as "Critic" is a code review system written by Jens Lindström to solve the major pain points that we had experienced with all other code review systems we had tried at Opera. Critic is written to work with git, and has the following workflow: * Each set of changes to be reviewed is a single branch containing one or more commits * A set of possible reviewers for each file changed is assigned based on preconfigured, path-based, filters * The reviewer(s) review the commits, raising issues in general, or against specific lines, marking each file as "reviewed" once they have finished commenting on it (irrespective of whether it has issues). * The submitter (or anyone else) pushes additional commits to the same branch in order to address the issues flagged by the reviewer. * Critic automatically resolves issues where the new commits have altered code that previously had an issue * The reviewer reviews the new commit, adding new issues or reopening old issues that were incorrectly marked as resolved. * In case that clarification is needed, reviewer and author add additional comments to the various issues. If it turns out that the issue is not a problem, or that it was fixed in some way undetected by critic, it is manually marked as resolved. * Finally all changes are marked as "reviewed" and there are 0 issues remaining so the code is automatically marked as "Accepted". * Someone (typically the author, but it could also be the reviewer) proceeds to integrate the review branch with master in the normal way. As you see this has a great deal in common with the github pull request process. Pull requests are branches, to which the author will push more commits in order to respond to issues. Compared to the github experience critic offers a number of significant advantages: * Automatic assignment of reviewers to changes based on reviewer-supplied filters (so e.g. an XHR test facilitator could set themselves up to review only XHR-related tests without also getting email related to 2dcontext tests). * The ability to review multiple commits squashed together into a single diff. * Tracking of which files in which commits have already received review and which still require review. * A record of which issues are still to be addressed and which have been resolved, and how. * A significantly better UI for doing the actual review, notably including a side-by-side view of the before/after code (with changes highlighted of course) so that one can read the final code without having to mentally apply a diff. * Less email (for some reason github think it's OK to send one email per comment on the review, whereas critic allows comments to be sent in batches). Noticing the similarity with the github workflow, I have extended critic to allow it to integrate with github. In particular I added two features: * Critic authentication against github using OAuth. * Integration with pull requests so that new pull requests create new review requests, pushes to pull requests are mirrored in the corresponding review and closing pull requests closes the review. I have set up a critic instance for the web-platform-tests repository at [1] (you have to sign in using your github credentials). So far my experience is that it makes it possible to review changes that are essentially unreviewable on github with a minimum of accidental complexity (e.g. [2]; yes that review still has a lot of work to be done). There is a certain amount of controversy around using critic for web-platform-tests, with some people taking the position that we should exclusively use the undoubtedly weaker, but more familiar, github tools to make things easier for new contributors. I consider that review has so much intrinsic difficulty that we should explore all the options we have for making it easier, especially given the particular dynamics of test reviews. Testsuites are often developed internally and then submitted to a standards body at the last moment, so they are commonly large code dumps rather than small changes that can be reviewed in a single sitting. This is the situation in which the github tools become almost useless. Also, even in an ideal situation with many contributors, I would expect test submissions to follow something like the 80/20 rule (80% of tests from 20% of subm
Re: [admin] Testing and GitHub login names
On Tuesday, April 23, 2013 at 9:55 AM, James Graham wrote: > On 04/23/2013 08:43 AM, Robin Berjon wrote: > > On 22/04/2013 13:12 , James Graham wrote: > > > On Mon, 22 Apr 2013, Arthur Barstow wrote: > > > > > The only thing that we ask is that pull requests not be merged by > > > > > whoever made the request. > > > > > > > > > > > > > > > > Is this to prevent the `fox guarding the chicken coop`, so to speak? > > > > > > > > If a test facilitator submits tests (i.e. makes a PR) and everyone > > > > that reviews them says they are OK, it seems like the facilitator > > > > should be able to do the merge. > > > > > > > > > > > > Yes, my view is that Robin is trying to enforce the wrong condition > > > here. > > > > No, I'm just operating under different assumptions. As I said before, if > > someone wants to review without having push/merge powers, it's perfectly > > okay. I don't even think we need a convention for it (at this point). I > > do however consider that this is an open project, so that whoever > > reviews tests can be granted push/merge power. > > > > Why? Because the alternative is this: you get an "accepted" comment from > > someone on a PR. Either you trust that person, in which case she could > > have merge powers; or you don't, in which case you have to review the > > review to check that it's okay. Either way, we're better off making that > > decision at the capability assignment level since it only happens once > > per person. > > FWIW I'm used to a situation in which the opposite approach is generally > taken; that is a reviewer is responsible for reviewing, but the > submitter is responsible for doing the final integration of their > changes. Here, the "final integration" consists of clicking the "merge button" followed by clicking the "are you sure?" button. Hardly a place where you'll catch a mistake. Generally, OS projects using the GH workflow tend to leave integration to the reviewers, as they're the only ones able to have access to merging content to the canonical repo. I suggest we do the same and grant collaborator access to anyone who has had one contribution merged in the canonical repo OR is a WG member. This is based on the "Pull Request Hack" system[1] described by Felix Geisendörfer. Here's a short FAQ around this proposal: 1. Who can review an merge content? Repo collaborators. 2. Who can become a repo collaborator? Anyone that fulfills either one of the below conditions: a) be a member of any WG which has tests in the web-platform-tests repo, OR b) have had at least one contribution to web-platform-tests be merged in the main repo. 3. How do you become a repo collaborator? Right now, ask Robin or myself. We're hoping to get tooling to simplify this in the near future. 4. Who is responsible for checking the CLA has been signed? The reviewer when merging a contribution (if the contributor is not a repo collaborator, no point if he/she is). We'll have tooling to help with this. --tobie --- [1]: http://felixge.de/2013/03/11/the-pull-request-hack.html
Re: [admin] Testing and GitHub login names
X-posting to public-infra for those that have missed the conversation going on in WebApps on the subject of test review within the web-platform-tests repository on GitHub. On Tuesday, April 23, 2013 at 9:55 AM, James Graham wrote: > On 04/23/2013 08:43 AM, Robin Berjon wrote: > On 22/04/2013 13:12 , James Graham wrote:On Mon, 22 Apr 2013, Arthur Barstow > wrote:The only thing that we ask is that pull requests not be merged by > > > > > whoever made the request. > > > > > > > > > > > Is this to prevent the `fox guarding the chicken coop`, so to speak? > > > > > If a test facilitator submits tests (i.e. makes a PR) and everyonethat > reviews them says they are OK, it seems like the facilitator > > > > should be able to do the merge. > > > > > > > Yes, my view is that Robin is trying to enforce the wrong condition > > > here. > > > > > No, I'm just operating under different assumptions. As I said before, > ifsomeone wants to review without having push/merge powers, it's > perfectlyokay. I don't even think we need a convention for it (at this > point). Ido however consider that this is an open project, so that > whoeverreviews tests can be granted push/merge power. > > > Why? Because the alternative is this: you get an "accepted" comment > fromsomeone on a PR. Either you trust that person, in which case she > couldhave merge powers; or you don't, in which case you have to review > thereview to check that it's okay. Either way, we're better off making > thatdecision at the capability assignment level since it only happens once > > per person. > > > FWIW I'm used to a situation in which the opposite approach is generally > taken; that is a reviewer is responsible for reviewing, but the > submitter is responsible for doing the final integration of their > changes.Here, the "final integration" consists of clicking the "merge button" > followed by clicking the "are you sure?" button. Hardly a place where you'll > catch a mistake. Generally, OS projects using the GH workflow tend to leave integration to the reviewers, as they're the only ones able to have access to merging content to the canonical repo. I suggest we do the same and grant collaborator access to anyone who has had one contribution merged in the canonical repo OR is a WG member. This is based on the "Pull Request Hack" system[1] described by Felix Geisendörfer. Here's a short FAQ around this proposal: 1. Who can review an merge content? Repo collaborators. 2. Who can become a repo collaborator? Anyone that fulfills either one of the below conditions: a) be a member of any WG which has tests in the web-platform-tests repo, OR b) have had at least one contribution to web-platform-tests be merged in the main repo. 3. How do you become a repo collaborator? Right now, ask Robin or myself. We're hoping to get tooling to simplify this in the near future. 4. Who is responsible for checking the CLA has been signed? The reviewer when merging a contribution (if the contributor is not a repo collaborator, no point if he/she is). We'll have tooling to help with this. --tobie --- [1]: http://felixge.de/2013/03/11/the-pull-request-hack.html
Re: [admin] Testing and GitHub login names
On 04/23/2013 08:43 AM, Robin Berjon wrote: On 22/04/2013 13:12 , James Graham wrote: On Mon, 22 Apr 2013, Arthur Barstow wrote: The only thing that we ask is that pull requests not be merged by whoever made the request. Is this to prevent the `fox guarding the chicken coop`, so to speak? If a test facilitator submits tests (i.e. makes a PR) and everyone that reviews them says they are OK, it seems like the facilitator should be able to do the merge. Yes, my view is that Robin is trying to enforce the wrong condition here. No, I'm just operating under different assumptions. As I said before, if someone wants to review without having push/merge powers, it's perfectly okay. I don't even think we need a convention for it (at this point). I do however consider that this is an open project, so that whoever reviews tests can be granted push/merge power. Why? Because the alternative is this: you get an "accepted" comment from someone on a PR. Either you trust that person, in which case she could have merge powers; or you don't, in which case you have to review the review to check that it's okay. Either way, we're better off making that decision at the capability assignment level since it only happens once per person. FWIW I'm used to a situation in which the opposite approach is generally taken; that is a reviewer is responsible for reviewing, but the submitter is responsible for doing the final integration of their changes. This has several advantages; it is not uncommon for code to go through review only for the submitter themselves to realise that there was an uncaught mistake or a piece missing. This prevents the overhead of a second review/test cycle just to fixup such an error. It also means that the submitter is very clear about what has been integrated and what has not. Indeed, there are currently 41 open pull requests and that number is not decreasing. Getting more help with the reviewing is essential. But that's a Hard Problem because reviewing is both difficult and boring. I would qualify that statement. If you're already pretty good with web standards and you wish to improve your understanding to top levels (and gain respect from your peers), this is actually a really good thing to work on. Or if you're implementing, it's likely a little bit less work to review than to write from scratch (and it can make you aware of corner cases or problems you hadn't thought of). Put differently, I think it can be a lot less boring if you're getting something out of it. Oh yeah, there are theoretically good reasons that people might want to spend time on doing test review. But as yet that doesn't seem to be enough to get people actually doing it; so far there have been a handful of people reviewing tests (I count 6 in total). Clearly we need to do something better here.
Re: [admin] Testing and GitHub login names
On 22/04/2013 13:12 , James Graham wrote: On Mon, 22 Apr 2013, Arthur Barstow wrote: The only thing that we ask is that pull requests not be merged by whoever made the request. Is this to prevent the `fox guarding the chicken coop`, so to speak? If a test facilitator submits tests (i.e. makes a PR) and everyone that reviews them says they are OK, it seems like the facilitator should be able to do the merge. Yes, my view is that Robin is trying to enforce the wrong condition here. No, I'm just operating under different assumptions. As I said before, if someone wants to review without having push/merge powers, it's perfectly okay. I don't even think we need a convention for it (at this point). I do however consider that this is an open project, so that whoever reviews tests can be granted push/merge power. Why? Because the alternative is this: you get an "accepted" comment from someone on a PR. Either you trust that person, in which case she could have merge powers; or you don't, in which case you have to review the review to check that it's okay. Either way, we're better off making that decision at the capability assignment level since it only happens once per person. The problem isn't with people merging their own changes; it's with unreviewed changes being merged. Yup. (as an aside, I note that critic does a much better job here. It allows reviewers to mark when they have completed reviewing each file in each commit. It also records exactly how each issue raised was resolved, either by the commit that fixed it or by the person that decided to mark the issue as resolved) You may wish to introduce Critic a bit more than that; I'm pretty sure that many of the bystanders in this conversation aren't conversant with it. Indeed, there are currently 41 open pull requests and that number is not decreasing. Getting more help with the reviewing is essential. But that's a Hard Problem because reviewing is both difficult and boring. I would qualify that statement. If you're already pretty good with web standards and you wish to improve your understanding to top levels (and gain respect from your peers), this is actually a really good thing to work on. Or if you're implementing, it's likely a little bit less work to review than to write from scratch (and it can make you aware of corner cases or problems you hadn't thought of). Put differently, I think it can be a lot less boring if you're getting something out of it. -- Robin Berjon - http://berjon.com/ - @robinberjon
Re: [admin] Testing and GitHub login names
On 22/04/2013 12:44 , Arthur Barstow wrote: The only thing that we ask is that pull requests not be merged by whoever made the request. Is this to prevent the `fox guarding the chicken coop`, so to speak? The way you put it ascribes malice, whereas we operate on the assumption that people are honest and trustworthy. This is different from the previous rules whereby you couldn't review tests from someone working in the same company as yourself. I'm pretty sure that people here are indeed honest, and at any rate if they aren't the cost in lost credibility along with the quasi-certainty of being caught when another vendor notices a problem with the tests ought to make them behave as if they were :) What we're trying to prevent is more "every terribly sucks at noticing their own typos". If a test facilitator submits tests (i.e. makes a PR) and everyone that reviews them says they are OK, it seems like the facilitator should be able to do the merge. Of course. If you submit a PR with tests and someone who doesn't happen to have push powers has okayed them, then you should just merge them with a comment to the effect that such and such has found them to be good. The point is to get some eyeballs that aren't the author's to look at the tests before they go in; whatever process makes that happen is good so long as it does not involve bureaucracy. -- Robin Berjon - http://berjon.com/ - @robinberjon
Re: [admin] Testing and GitHub login names
On Mon, 22 Apr 2013, Arthur Barstow wrote: The only thing that we ask is that pull requests not be merged by whoever made the request. Is this to prevent the `fox guarding the chicken coop`, so to speak? If a test facilitator submits tests (i.e. makes a PR) and everyone that reviews them says they are OK, it seems like the facilitator should be able to do the merge. Yes, my view is that Robin is trying to enforce the wrong condition here. The problem isn't with people merging their own changes; it's with unreviewed changes being merged. Unfortunately github doesn't naturally provide any way to track progress of a review and therefore there isn't any way to tell that review is complete. Just to signal the end of the review we could adopt some convention like leaving a comment "Accepted" to indicate that the reviewer believes that all commits have been fully reviewed and there are no further issues to be resolved. (as an aside, I note that critic does a much better job here. It allows reviewers to mark when they have completed reviewing each file in each commit. It also records exactly how each issue raised was resolved, either by the commit that fixed it or by the person that decided to mark the issue as resolved) So anyone with a GitHub account is already 100% set up to contribute. If you *do* wish to help with the reviewing and organisation effort, you're more than welcome to and I'll be happy to add you. I just wanted to make sure that people realise there's zero overhead for regular contributions. Indeed, there are currently 41 open pull requests and that number is not decreasing. Getting more help with the reviewing is essential. But that's a Hard Problem because reviewing is both difficult and boring.
Re: [admin] Testing and GitHub login names
On 4/22/13 2:29 PM, ext Robin Berjon wrote: On 19/04/2013 06:15 , Arthur Barstow wrote: Test Facilitators, Editors, All, If you intend to continue to participate in WebApps' testing effort or you intend to begin to participate, please send your GitHub login name to Robin (ro...@w3.org) so he can make sure you have appropriate access to WebApps' test directories. I would like to point out an important detail here: unless you want to review tests or to participate in the general shepherding of the test suite, you don't need to send me your GitHub login. More specifically, if you only plan to contribute tests, you don't need to send me anything: you already can. The way things works for contributors (irrespective of whether they have push access or not) is this: all contributions are made through pull requests. That's how we organise code review. Yes, thanks for this clarification. The only thing that we ask is that pull requests not be merged by whoever made the request. Is this to prevent the `fox guarding the chicken coop`, so to speak? If a test facilitator submits tests (i.e. makes a PR) and everyone that reviews them says they are OK, it seems like the facilitator should be able to do the merge. (My apologies if I still don't quite understand all of the work flow here.) So anyone with a GitHub account is already 100% set up to contribute. If you *do* wish to help with the reviewing and organisation effort, you're more than welcome to and I'll be happy to add you. I just wanted to make sure that people realise there's zero overhead for regular contributions. Excellent. -AB
Re: [admin] Testing and GitHub login names
On 19/04/2013 06:15 , Arthur Barstow wrote: Test Facilitators, Editors, All, If you intend to continue to participate in WebApps' testing effort or you intend to begin to participate, please send your GitHub login name to Robin (ro...@w3.org) so he can make sure you have appropriate access to WebApps' test directories. I would like to point out an important detail here: unless you want to review tests or to participate in the general shepherding of the test suite, you don't need to send me your GitHub login. More specifically, if you only plan to contribute tests, you don't need to send me anything: you already can. The way things works for contributors (irrespective of whether they have push access or not) is this: all contributions are made through pull requests. That's how we organise code review. The only thing that we ask is that pull requests not be merged by whoever made the request. So anyone with a GitHub account is already 100% set up to contribute. If you *do* wish to help with the reviewing and organisation effort, you're more than welcome to and I'll be happy to add you. I just wanted to make sure that people realise there's zero overhead for regular contributions. -- Robin Berjon - http://berjon.com/ - @robinberjon
[admin] Testing and GitHub login names
Test Facilitators, Editors, All, If you intend to continue to participate in WebApps' testing effort or you intend to begin to participate, please send your GitHub login name to Robin (ro...@w3.org) so he can make sure you have appropriate access to WebApps' test directories. -Thanks, AB
