Re: RFC: Add new BiDi tests exclusively via WPT PRs, not via custom vendor infra, to ensure cross-vendor reviews

Hello,

> Is part of the ask here that companies can't review their own code? E.g.
Google can't reviewer googlers code?

Not quite, reviewing your own code is OK.
What I meant was that, for new test additions (or other non-trivial and/or
contentious changes in general), we would like to have the opportunity to
review them before they land.

> I have concerns about how it might slow down development of these APIs at
Vendors as it's removed from browser code and not part of everyones
development process.
> I am supportive in having things done in WPT especially when there are
tests that support a spec PR but am worried moving to everything on WPT

The "Add new BiDi tests exclusively via WPT PRs" suggestion was *the most
straightforward* way to ensure that but, taking one step back, we don't
necessarily have to enforce it.
However, is there a way to ensure cross-vendor review when using our own
infra?

And, most importantly, is this an outcome that would be beneficial to this
group?

To give a bit of context and motivation: When I joined the project as part
of Google/Chromium, I was told to, in general, to wait for at least one
review from another vendor (currently, typically Mozilla) before landing my
PRs.
This is to diminish overall bias and to get new perspectives in PRs.
I am still following this philosophy, in general.
But then, if this is something we would like to keep doing, I'd like it to
be bidi (pun intended), and (for example) Mozilla PRs should have at least
one review from Google (or any other vendor, for that matter); and the same
goes for other vendors.

Is this a principle we would like to follow? There's no right answer here,
a "no" could be acceptable too. But, whatever we choose, I'd just like to
ensure consistency.


Hopefully now it's clearer what I meant? Let me know if you have any other
questions.

On Tue, Nov 14, 2023 at 8:28 PM David Burns <david.b@browserstack.com>
wrote:

> Thiago,
>
> Is part of the ask here that companies can't review their own code? E.g.
> Google can't reviewer googlers code?
> If we still allow
>
> I have concerns about how it might slow down development of these APIs at
> Vendors as it's removed from browser code and not part of everyones
> development process. I know when I was at Mozilla I used to get a lot of
> noise from github I stopped looking at the notifications on github.
>
> I am supportive in having things done in WPT especially when there are
> tests that support a spec PR but am worried moving to everything on WPT
> will just be shifting the burden of work to review from where code is
> written (Gerrit/Phabricator) to WPT which could lead to slow down of
> reviews.
>
> Since this is less likely to affect me I would like to hear what other
> implementors say.
>
> David
>
> On Tue, Nov 14, 2023 at 1:54 PM Thiago Perrotta <tperrotta@chromium.org>
> wrote:
>
>> Hello,
>>
>> I would like to ask that we *submit new BiDi tests exclusively via WPT
>> PRs*, instead of using our own infra (e.g. Bugzilla/Phrabricator for
>> Mozilla, Monorail/Gerrit for Google/Chromium).
>>
>> This is to ensure we can have *cross-vendor reviews, which should
>> increase the quality and interoperability of new tests.*
>>
>> If we add new tests via our own infra, then they *often come as a
>> surprise to other vendors*, and if there are any comments about them
>> then it is usually *a burden on these vendors to follow up* and ask the
>> original author to address the comments.
>>
>> A few recent examples include:
>>
>> - network.authRequired (https://phabricator.services.mozilla.com/D189517)
>> - network.addIntercept (https://phabricator.services.mozilla.com/D191237)
>> - browsingContext.contextDestroyed (
>> https://phabricator.services.mozilla.com/D189473)
>>
>> You can find most of them with "git log --grep=bugzilla-url
>> webdriver/tests/bidi" in the WPT repo.
>>
>>
>> *Ideally, most of these should have been submitted via WPT PRs.*
>> "Trivial" changes are fine to be exempted from this request. Examples of
>> trivial changes:
>>
>> - Remove "scrollIntoView" from "browsingContext.captureScreenshot" (
>> https://phabricator.services.mozilla.com/D192579)
>> - Extend timeout for an invalidation test file (
>> https://phabricator.services.mozilla.com/D188719)
>>
>> Naturally the definition of "trivial" may vary amongst different people. *A
>> good rule of thumb is: new test additions, especially new test file
>> additions, should be considered non-trivial.*
>>
>>
>> Thank you,
>> --
>> All the best,
>> - Thiago
>>
>
>
> --
> David
>


-- 
All the best,
- Thiago