@foolip has identified an unrelated use case for this functionality: submitting
patches that are under review in a downstream project. In these cases, the
tests are not ready for review by the OWNERS here, but the downstream reviewers
would still like to see stability results. All this is to say, I think a more
generic tag is called for.

How about this: any pull request whose title includes the string "[PREVIEW]"
should not receive an automated reply from the @wpt-pr-bot. A nice extension
would be to react to changes in the title--to make the bot comment when that
string is removed from the title... but maybe we could defer that to a
follow-up patch.

@tobie I'd like to implement this feature in your Ganesh project. What do you
think?

How about this: any pull request whose title includes the string "[PREVIEW]"
should not receive an automated reply from the @wpt-pr-bot.

I'd bikeshed this into [NOT-READY-FOR-REVIEW], [NO-REVIEW], [NOT-READY] or the like, as I'm really not sure what [PREVIEW] means.

A nice extension
would be to react to changes in the title--to make the bot comment when that
string is removed from the title... but maybe we could defer that to a
follow-up patch.

That shouldn't be too hard to implement. The bot already on each new comment so you'll have to guard against those anyway.

@tobie I'd like to implement this feature in your Ganesh project. What do you
think?

Awaiting your PR. :)

A nice extension would be to react to changes in the title--to make the bot comment when that
string is removed from the title

Thinking about this again, I doubt if this would actually be used for ongoing external reviews like from Chromium. At least there wouldn't be much point in notifying owners before actually merging, since that'll happen automatically, but an FYI after the fact might be nice.

@jeffcarp, @qyearsley, do you think a label or a special title format would be better for Chromium's wpt export eventually? My hunch is that labels are easier to manipulate via the GitHub APIs, and that it might be odd if the title doesn't match what is actually merged.

I suggest a do not notify owners label or perhaps something shorter.

Agree a label is a better fit… but labels can't be added until after the PR has been created. Pre-morning-coffee suggestions:

1. Add a 1 minute timeout before processing the PR creation event to account for labels added post creation. Brittle.
2. Let the API turn the title sigil into a label and remove it. Makes discovery of the feature difficult.
3. Add a whitelist of labels that can be added via the PR's body. E.g.: if you add the line Labels: foo, bar, do-not-merge, do-not-mirror than the labels do-not-merge and do-not-review would be added to the PR (but not foo or bar) and the whole label line would be removed. That's my favorite solution, tbh, but the more involved one.

Edit: please see post-coffee suggestions below.

Are you sure labels can't be added when the PR is created? It looks possible via the web interface, can't it be done using APIs?

So was definitely half-awake when I commented above and forgot creating a pull request was a two-step process.

This should indeed just be one (or multiple) labels.

We can either decide that there's a specific label to avoid triggering the bot (e.g. not-review-ready) or a number of labels which do that as a side effect (e.g.: downstream-review, infra, etc.). Implementation wise, it's similar.

It's not entirely clear I wouldn't want to be pinged about upstreamed changes. They're certainly not always correct. Especially as we're getting closer to 100% coverage for standards it seems important that OWNERS double check any changes made.

Seems that would push for a not not-review-ready type label, that would trigger a bot comment once removed.

I would just make the label no-ping or something that clearly specifies what it does rather than trying to capture the exact set of semantics of all the use cases.

Having said that, we should move all the ci stuff into tools/ci (awkward until tools isn't a submodule) and teach the bot not to ping changes that include changes in that directory, or something. People will forget to add a label all the time.

Second the more generic label. We often want a review on the infra changes while we just don't want to ping/notify the owners of the tests we've changed as demos. A no-review label or the like could be confusing to future contributors in such a case.

Second the more generic label. We often want a review on the infra changes while we just don't want to ping/notify the owners of the tests we've changed as demos. A no-review label or the like could be confusing to future contributors in such a case.

do-not-notify-owners seems more explicit, then.

Having said that, we should […] teach the bot not to ping changes that include changes in that directory, or something. People will forget to add a label all the time.

Actually, we can automatically tag such cases with the do-not-notify-owners label.

As a contributor, but not a member of the w3c org, I can't add labels to my own PRs, so perhaps a title/commit/comment string would be more broadly useful.

As a contributor, but not a member of the w3c org, I can't add labels to my own PRs, so perhaps a title/commit/comment string would be more broadly useful.

So we can:

1. automatically tag a bunch of common cases with a do-not-notify-owners label,
2. make contributors org members, and/or
3. create a whitelist of labels that can be added via the PR's body. E.g.: if you add the line
   Labels: foo, bar, do-not-notify-owners
   then the labels do-not-notify-owners gets added to the PR, but not the other two.

That sounds reasonable

That sounds reasonable

Can you be a tad more specific? ;)

It's not entirely clear I wouldn't want to be pinged about upstreamed changes. They're certainly not always correct. Especially as we're getting closer to 100% coverage for standards it seems important that OWNERS double check any changes made.

This is about the kind of scenario described in https://bugzilla.mozilla.org/show_bug.cgi?id=1336422, where a PR is created while the Gecko (or Chromium) review is still in progress, and presumably the PR is automatically updated if the upstream review is. In that state, the PR would be used to make sure it can later be merged, actually having a split review would be weird.

Ideally the label would be removed right after merging, causing the owners to be notified.

After sleeping on this, I think it's possible that I may have been suffering
from functional fixedness. Whatever the patch's origin, the use case is: "I want
to validate this change in CI without notifying any AUTHORS in a pull request."
TravisCI is currently configured to "build branches," meaning that simply
pushing to a branch in the upstream repository triggers a build--no pull request
necessary. So whether you are testing infrastructure changes or validating a
downstream patch to test files, you can already enjoy the benefit of CI results
without creating spam for others: just create a branch with your work in the
upstream repository.

This practice would not only avoid additional complexity in the infrastructure
code, it would also limit the amount of domain-specific details that
contributors have to discover/learn/memorize. Admittedly, it doesn't address
two scenarios supported by the proposed "label"/"sigil" feature:

1. "I want to validate a patch that specifically concerns the logic for posting
   a comment to GitHub."
2. "I want to 'pre-validate' my patch, but I do not have the necessary
   permissions to create a branch in the upstream repository."

I think 1 is probably rare enough to forget about. I'm not sure 2 is
actually valid since the "pre-validation" use case has only been requested by
downstream consumers that already have merge rights.

What do folks here think?

Are you sure "I want to validate this change in CI without notifying any AUTHORS in a pull request" is the use case that prompted this issue? Is "mergability" something that is tracked outside of a PR? Is the social signal of opening a PR for downstream work not important? etc.? (I don't have an opinion on the topic, btw. just asking.)

I'm also unsure that 1) is a good description of what @domenic experienced and caused him to file this issue. If it is, I suggest just creating a test directory should solve it. @jgraham on irc suggested there was more to it than that.

Are you sure "I want to validate this change in CI without notifying any
AUTHORS in a pull request" is the use case that prompted this issue? [...]
I'm also unsure that 1) is a good description of what @domenic experienced
and caused him to file this issue.

I have been operating under the assumption that my recent activity (along with
that of @bobholt) is what inspired Domenic's feature request. @domenic: could
you verify that?

If it is, I suggest just creating a test directory should solve it. @jgraham
on irc suggested there was more to it than that.

Actually, that seems like another way we could have avoided all this noise with
our infrastructure work... Though it wouldn't help with @foolip's use case
(2). @jgraham could you elaborate here?

Yes, verified that it's all the pings on supposed "URL" test changes where in actually you all are just modifying URL files to see what happens with the CI.

Based on the suggestion above, let's talk about the test directory solution:

I know creating a new directory with tests will run them. If it does not include an OWNERS file, I assume nobody will be notified (unless there's a default fall-back I'm unaware of).

In terms of resolving this issue, there are still some open questions:

1. Should we commit an always-passing folder of tautologies to the repo that people can quickly modify to get failing results if needed or force users to create this on their own?
2. Where should we document the recommendation for creating intentionally-flaky tests for infrastructure without causing unnecessary notifications?
3. Is there still a need for a "don't ping reviewers" sigil outside of this use case?

Should we commit an always-passing folder of tautologies to the repo that
people can quickly modify to get failing results if needed or force users to
create this on their own?

That may be more convenience than we need. I think for this use case, the problem is one of documentation, which leads nicely into your second question:

Where should we document the recommendation for creating intentionally-flaky
tests for infrastructure without causing unnecessary notifications?

Probably we should extend http://web-platform-tests.org/ with a section for folks looking to contribute to the infrastructure.

Is there still a need for a "don't ping reviewers" sigil outside of this use
case?

Based on @foolip's input (and discussion for Firefox and Chromium), it sounds like there is a need to vet patches from automated processes prior to requesting WPT review. Since this involves the eventual notification of OWNERS, a GitHub issue "tag" is probably a better fit than a commit message "sigil."

A not-ready-for-review (or similar) label seems perfectly fine.

Happy to review a PR for it on https://github.com/tobie/ganesh. If not, I might decide to look into it myself at some point.

Draft pull requests fill this purpose now.