w3c / webdriver

Remote control interface that enables introspection and control of user agents.
https://w3c.github.io/webdriver/
Other
676 stars 190 forks source link

Review process of handling pull requests and code review #1681

Open whimboo opened 1 year ago

whimboo commented 1 year ago

Via the CODEOWNERS file we currently have three owners of the repository. These are @shs96c @AutomatedTester and @jgraham.

Whenever someone is working on a PR and requesting review the question comes up who can actually review the changes and if these code owners have to do a final review before a PR can be merged. As we all know the process of getting PRs landed is quite slow and it would be great to actually make them faster. Over the last couple of months we made a couple of good experiences with the current workflow in the WebDriver BiDi repository, and wonder if a similar handling of PRs could be accepted for this repository as well.

Questions that should be discussed:

It would be great to get this discussed in the next monthly WebDriver meeting or at latest during TPAC in Vancouver this month.

css-meeting-bot commented 1 year ago

The Browser Testing and Tools Working Group just discussed WebDriver-classic review process.

The full IRC log of that discussion <AutomatedTester> Topic: WebDriver-classic review process
<jgraham> Github: https://github.com/w3c/webdriver/issues/1681
<AutomatedTester> jgraham (IRC): I will take this. The topic is about getting review to the webdriver spec
<AutomatedTester> ... there are 2 reasons this relevant
<AutomatedTester> ... 1) we are adding features and bug features to the classic support
<AutomatedTester> ... 2) and this can block bidi
<AutomatedTester> ... in bidi where there are things duplicated we should reference things in the classic
<AutomatedTester> ... so we have a few issues that are waiting
<AutomatedTester> ... and there are PRs blocking working on bidi
<AutomatedTester> ... as it says in the issue
<AutomatedTester> ... there are only 3 codeowners... are we only expecting these people? How can we add new reviewers
<AutomatedTester> q+
<jgraham> AutomatedTester: Are those the only people who can review? No. If there are other people we should add them, and help them get started. One suggestion is that if these items are blocked and discussed in the BiDi editorial meeting and there's consensus then we could use that approval to get them landed. I'm happy to help make the process simpler.
<AutomatedTester> jgraham (IRC): so my question for sadym (IRC) and Brandon Walderman what can we do to make you feel comfortable in reviewing these PRs?
<AutomatedTester> Brandon Walderman: for my role in Edge on bidi, things have been scaled back at MS
<AutomatedTester> ... I am happy to help with reviews but can't commit to more
<AutomatedTester> sadym (IRC): I have the opposite issue, I can get more into the reviews... the issue is trying to get up to speed but I am happy to get up to speed more
<AutomatedTester> patrickangle: I would also be happy to review classic as I have been working on that implementation (at Apple)
<sadym> q+
<gsnedders> q+
<jgraham> q+
<AutomatedTester> ack
<AutomatedTester> ack next
<AutomatedTester> ack next
<sadym> q+
<jgraham> ack gsnedders
<AutomatedTester> Sam Sneddon [:gsnedders]: some of the reason I haven't done this is because there are items that have made things just sit there and I haven't felt like doing more... but if we are actively doing things then I am happy to help too
<AutomatedTester> ack next
<AutomatedTester> jgraham (IRC): I think we have a comms issue here and we need to make sure that we flag these things in editorial meetings moving forward
<AutomatedTester> ... maybe we set a process for review priority
<AutomatedTester> ... the other question is around the codeowners, do we want it or deleting it?
<jgraham> AutomatedTester: Happy to move towards a BiDi approach
<jgraham> AutomatedTester: We should try to keep things as simple as possible and use the same processes for both specs
<jgraham> q?
<jgraham> q+
<AutomatedTester> ack next
<AutomatedTester> sadym (IRC): a bit off topic; what is the process I try prototype it and then give review
<AutomatedTester> ack next
<AutomatedTester> jgraham (IRC): from the classic point a lot of things are refactors in the queue, so we don't need to have people trying to implement it
<AutomatedTester> action: remove CODEOWNERS from the repo
<AutomatedTester> action: add label for items that need to be reviewed in the next editorial meeting
whimboo commented 1 year ago

Btw. https://github.com/w3c/webdriver/pull/1684 has been opened to remove the CODEOWNERS.md file.

action: add label for items that need to be reviewed in the next editorial meeting

@AutomatedTester and @shs96c would you be fine with needs-discussion similar to what we have for WebDriver BiDi?

whimboo commented 1 year ago

Given that we already use needs-discussion I assume we can continue doing so? For meetings we can add a link and start with the oldest issue as listed?