Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Agree on policy on reverting #9953

Closed
foolip opened this issue Mar 9, 2018 · 9 comments
Closed

Agree on policy on reverting #9953

foolip opened this issue Mar 9, 2018 · 9 comments
Labels

Comments

@foolip
Copy link
Member

foolip commented Mar 9, 2018

#9718 was a bit of non-fun, and I believe we need to agree on and document when we revert things and how fast it should be. This was recently also an issue with #8614, where the time between breakage and fix was longish, although nobody then considered reverting AFAICT.

So:

  • Roughly what kinds of breakage warrants reverting?
    • Breakage on Travis?
    • Breakage for wpt.fyi runners?
    • Breakage in browser repos like Chromium which import wpt?
    • Breakage in CSS WG tooling?
  • Should reverts be landed immediately, bypassing Travis, or wait for Travis, or even review?
@foolip foolip added the policy label Mar 9, 2018
@foolip
Copy link
Member Author

foolip commented Mar 9, 2018

What I think personally:

  • Stuff that makes Travis fail (Lint is failing on css/css-fonts/cascading-font-sizes.html #9304)
  • Stuff that makes wpt run fail on any browser and any platform that people are actively trying to run frequently and more reliably. (wpt.fyi)
  • Stuff that breaks running wpt in browser engine repos that import wpt frequently (Chromium, Gecko, hopefully more)

As for what to bypass, Travis breakage obviously need to bypass Travis. For the other cases, I think it comes down to whether waiting up to 50 minutes makes a difference, leaning towards bypassing if someone requests it.

CSSWG tooling, then. When it accidentally breaks, and CSSWG members asks for a revert, then I think we should revert. But I think we also need a way of reliably getting those issues resolved in a timely manner so that we can reland. I don't think we can come up with a constant number of days that we wait and then reland anyway, but I would hope we could coordinate and expect it to be resolved within a week if the change is something pretty superficial like how files are moved around, and not something like "let's break all reftests" (example!)...

@foolip
Copy link
Member Author

foolip commented Mar 9, 2018

@foolip
Copy link
Member Author

foolip commented Mar 9, 2018

@boazsender @mariestaver, I imagine you don't disagree with reverting for the "actively trying to run frequently and more reliably" case?

@jgraham
Copy link
Contributor

jgraham commented Mar 12, 2018

So I think when changes to infrastructure break the ability to run tests it may be appropriate to revert changes if doing so provides the fastest path to resolving the situation. There are all sorts of edge cases here like changes that fix previously broken A but break previosuly working B, where there's some decision required as to whether A or B is more important.

I think one could formulate a more detailed policy here and specify tiers and so on, but I'm not sure how much value there is in that compared to rough consensus.

However I think the case in hand is very different. What we had was a change to tests which conformed to the general rules for the project, but happened to break a legacy system which imposes its own WG-specific requirements and is not being actively improved to match the rest of the project. In that case I think reverting the patch is OK as long as the changes are relatively low value for the other participants, but such an action needs to be exceptional and there needs to be a clear plan to reland within a short timeframe.

In general it is important to the success of the project that people are able to land changes to tests either in wpt directly or via browser repositories without memorising a large set of project rules, and without fear that their changes will be rejected for non-obvious reasons. It is an fundamental and longstanding tenent of wpt that contribution must not be substantially more difficult than for a vendor-specific testsuite, which is why there has been so much effort on two-way sync and improving the developer workflow.

@mariestaver
Copy link
Contributor

@foolip Our policy is broadly to "revert when something breaks a run that previously worked". But as both you and @jgraham point out, there are a lot of potential edge-cases here! So far it's made the most sense to resolve those cases in conversation, and maybe with a group decision if it's a particularly tricky case (like "this breaks A but fixes B, which is more important", for example).

We're also happy to listen if someone has an urgent need to reland a PR, but again, the details of doing this feels like it'll come from conversation rather than policy. However, I'm happy to help you create a more specific policy if you think that will potentially avoid confusion -- let me know if I can assist.

@foolip
Copy link
Member Author

foolip commented Mar 14, 2018

@jgraham, I agree, I'd like to have zero lints that only apply to css/, no external build system and a https://wpt.fyi/ that fulfills the needs of CSS folks. But AFAICT nobody is really spending much time working towards that goal, so what do we do in the meantime?

@plinss, do you have something like an importance-ordered list of things that this repo and https://wpt.fyi/ would have to support in order for the old build system to be abandoned? I know of some things, but not their real importance. Should I ask www-style?

@foolip
Copy link
Member Author

foolip commented Aug 15, 2018

Ping @plinss on CSS build system requirements in #9953 (comment).

(I'm reminded of this issue by #12482.)

@foolip
Copy link
Member Author

foolip commented Aug 15, 2018

I've sent #12493 to document what I think are reasonable guidelines. Everyone in this issue, please review!

@foolip
Copy link
Member Author

foolip commented Aug 15, 2018

#12493 has a bunch of approvals. To allow for more comments/objections, I'll leave the PR up until Friday afternoon (~ 15:00 UTC) before merging.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 23, 2018
…anges, a=testonly

Automatic update from web-platform-testsDocument rough consensus on reverting changes (#12493)

Fixes web-platform-tests/wpt#9953.
--

wpt-commits: 7abda4e7227b65fba2452980d106ed5f11d51b2b
wpt-pr: 12493
jankeromnes pushed a commit to jankeromnes/gecko that referenced this issue Aug 23, 2018
…anges, a=testonly

Automatic update from web-platform-testsDocument rough consensus on reverting changes (#12493)

Fixes web-platform-tests/wpt#9953.
--

wpt-commits: 7abda4e7227b65fba2452980d106ed5f11d51b2b
wpt-pr: 12493
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…anges, a=testonly

Automatic update from web-platform-testsDocument rough consensus on reverting changes (#12493)

Fixes web-platform-tests/wpt#9953.
--

wpt-commits: 7abda4e7227b65fba2452980d106ed5f11d51b2b
wpt-pr: 12493

UltraBlame original commit: 4b4863b6778762764d7d8f9b4cb37f946019d800
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…anges, a=testonly

Automatic update from web-platform-testsDocument rough consensus on reverting changes (#12493)

Fixes web-platform-tests/wpt#9953.
--

wpt-commits: 7abda4e7227b65fba2452980d106ed5f11d51b2b
wpt-pr: 12493

UltraBlame original commit: 4b4863b6778762764d7d8f9b4cb37f946019d800
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…anges, a=testonly

Automatic update from web-platform-testsDocument rough consensus on reverting changes (#12493)

Fixes web-platform-tests/wpt#9953.
--

wpt-commits: 7abda4e7227b65fba2452980d106ed5f11d51b2b
wpt-pr: 12493

UltraBlame original commit: 4b4863b6778762764d7d8f9b4cb37f946019d800
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants