Wpt: Auto-mirror "safe" tests

[RByers]

It's a little annoying to have to add the 'w3c-test:mirror' label to all PRs to try them out, and it sounds like we might want to have our Edge/Safari validation tooling pull PRs via w3c-test.org also.

@jgraham mentioned that there's really no reason why we couldn't auto-mirror a PR when we know it changed only safe files (eg. HTML, CSS and JS). The real risk here is changes to the python scripts or other things that actually run code on the server.

As someone who reviews a bunch of PRs (most of which are "manual" tests for now due to reliance on input) it would be so handy if most PRs got an auto-comment saying "click here to run live" (one link per changed test file).

Is this worth looking into?

/cc @sideshowbarker @foolip @tobie

[foolip]

That would be awesome! I can never even remember the special incantation 'w3c-test:mirror' so when I see a PR without mirroring it's no fun. It would also be great to paste links to the modified tests, as finding the tests is a bit of work too.

[tobie]

I could easily add this to @wpt-pr-bot provided there's clear agreement as to what files may be touched to allow the 'w3c-test:mirror' incantation.

[tobie]

Additionally, it would be nice to:

1. allow it as part of the same comment @wpt-pr-bot already makes to ping reviewers, and
2. have an explainer URL it can reference.

[foolip]

I suppose a safelist of filename patterns is the most straightforward, although there are probably safe resources with no file extension.

[tobie]

@foolip: for each PR, I build a metadata object which contains an array of filenames (metadata.filenames) of the form:

[
  'pointerevents/pointerevent_support.js', 
  'pointerevents/pointerlock/pointerevent_movementxy-manual.html', 
  'pointerevents/pointerlock/resources/pointerevent_movementxy-iframe.html'
]

which I also convert to an array of directories (for tagging purposes) available as metadata.paths like so:

[
  'pointerevents', 
  'pointerevents/pointerlock', 
  'pointerevents/pointerlock/resources'
]

Fill-in the following JS function and I'll handle the rest:

function isSafe(metadata) {
    var is_safe = false;
    // whitelist
    return !!is_safe;
}

[tobie]

I can certainly add the link to the URL while I'm at it provided I'm also told how to build it (and what data you need from me to construct that URL beyond the PR number).

[sideshowbarker]

I could easily add this to @wpt-pr-bot provided there's clear agreement as to what files may be touched to allow the 'w3c-test:mirror' incantation.

Basically everything is safe except Python files and shell scripts, and doing a recursive find+sort+uniq through the repo for file extensions and eliminating those gives this list:

_Update: edited this to add css, which I left off by mistake initially_…

css
dat
dtd
frag
gif
htm
html
ico
idl
jpg
js
json
manifest
mp3
mp4
oga
ogv
pem
png
svg
swf
template
ttf
txt
vert
vtt
wav
webm
widl
widlprocxml
woff
xht
xhtml
xml
xsl
yaml
yml

[tobie]

Thanks, @sideshowbarker.

What's the template string for the url on w3c-tests.org for a given PR?

Roughly how long after the comment shows up is that URL setup?

[RByers]

What's the template string for the url on w3c-tests.org for a given PR?

https://w3c-test.org/submissions/<prnumber>/path/to/test.html

Roughly how long after the comment shows up is that URL setup?

I don't know. But if we could somehow hook it up so that the comment ONLY shows once the submission has been updated, that would be ideal. I've been burned before by accidentally trying to understand the test results corresponding to a previous iteration of a pull request.

[tobie]

@sideshowbarker:

Basically everything is safe except Python files and shell scripts, and doing a recursive find+sort+uniq through the repo for file extensions and eliminating those gives this list:

I guess .css is missing from this list by accident?

[tobie]

@RByers

https://w3c-test.org/submissions//path/to/test.html

So I imagine we need a link to each html file? Or each directory which contains html files?

I don't know. But if we could somehow hook it up so that the comment ONLY shows once the submission has been updated, that would be ideal. I've been burned before by accidentally trying to understand the test results corresponding to a previous iteration of a pull request.

That's out of scope of what I can do trivially, tbh.

I also hadn't thought about what happens when new files are pushed to the PR. How is that currently handled?

[tobie]

Pushed this as part of https://github.com/tobie/ganesh/commit/e5b87a4b6828117b49e9b4db3dc072be5556dbc2.

Should be active but not use the magic incantation for now (uses w3c-test:do-not-mirror instead).

Need answers to the following:

• .css extensions were omitted, please confirm that's accidental so I can add it to the whitelist.
• I assumed extensions are case insensitive, please confirm.
• Right now, the comment is only made once, upon creation of the PR. What happens if you push new code later on? That seems like a pretty large security hole if it gets mirrored too. What's the strategy, here?

[sideshowbarker]

I guess .css is missing from this list by accident?

Yes, missing just by accident. .css files should be included in the list

[sideshowbarker]

what happens when new files are pushed to the PR. How is that currently handled

The new files/changes get mirrored to https://w3c-test.org/submissions/<prnumber> (they get fetched and merged into the copy of the PR branch there)

[sideshowbarker]

I assumed extensions are case insensitive, please confirm.

yes, they should be handled case-insensitively (though I’m not sure we actually have any in the tree with uppercase extensions or mixed-case ones).

[sideshowbarker]

Right now, the comment is only made once, upon creation of the PR. What happens if you push new code later on? That seems like a pretty large security hole if it gets mirrored too. What's the strategy, here?

There’s actually a w3c-test:unmirror comment that causes the copy of the PR branch at https://w3c-test.org/submissions/<prnumber> to be deleted. So if @wpt-pr-bot sees that any files with extensions not on the safelist have been added or changed, it can use w3c-test:unmirror to delete https://w3c-test.org/submissions/<prnumber>.

[tobie]

Added support to wipeout the mirror if unsafe files show-up in https://github.com/tobie/ganesh/commit/ee73891fdedc4684bf7298f2c5cb32e18547b1b6.

[tobie]

@sideshowbarker I can imagine this might be slightly racey, but I guess we're fine with it.

On my side, the bot is ready. I would love for someone to run a test PR, with safe stuff first and then push further non safe files.

I'll move to using w3c-test:mirror after that.

[RByers]

This is awesome, thank you!

The new files/changes get mirrored to https://w3c-test.org/submissions/ (they get fetched and merged into the copy of the PR branch there)

I thought you had to specify the magic string again to get it to mirror the latest version? Otherwise the race condition here is a potentially serious (probably not too hard to exploit) hole, right? Seems like the safest way to do this is in whatever code actually decides to do the mirroring - don't require any magic token when there's only safe files, right?

Having ganesh post the links in the PR comment is definitely great though.

[tobie]

Seems like the safest way to do this is in whatever code actually decides to do the mirroring - don't require any magic token when there's only safe files, right?

Well that's currently an open hole, right? Get anyone to approve your perfectly legit PR, then push whatever you want to the server. You can even force push to clean-up. The server could very well already be compromised.

Yes, automation removes the need to socially engineer access, but the bar is so low to do that right now (just do a basic PR that only touches files that look safe), I'm not sure it makes a huge difference.

Seems like the safest way to do this is in whatever code actually decides to do the mirroring - don't require any magic token when there's only safe files, right?

Well, once you've added a security check on the server, the only reason you'd want to keep tokens around would be to continue to grant full access to the server to contributors which are not collaborators. This begs the question: why are you giving unchecked server access to contributors but not making them GH collaborators? I'd suggest completely doing away with tokens. If the contributor's a collaborator, mirror all the things. If they're not, only mirror if all files are whitelisted.

[tobie]

Updated the bot to just add the test URLs (with a different comment for PRs issued by collaborators and non-collaborators) and leave the mirroring issues to the test server script.

[tobie]

So I'm considering rolling back the changes to @wpt-pr-bot to expose test URLs. They're partially incorrect (not taking in account ref tests, resources, etc.), unable to know whether the tests have been mirrored, can't automatically mirror tests for security reasons.

Links are already available from the stability tests. I think that's enough.

Thoughts?

[RByers]

Yeah having the links in the stability output is often good enough. But there are cases when the stability bot fails to run or otherwise doesn't provide useful links (eg. manual tests).

Maybe it's worth keeping the 'These tests will be available shortly on w3c-test.org.' line but with a single link to whatever path is common among all changed files? So in the common case of a single test it just links to that test, for multiple tests in a directory it links to that directory, etc. Eg. cases like this it's still handy to have some useful link 10 minutes before the stability bot has posted it's comment, and in manual-test only cases like this it really does make reviewing a little quicker to just be able to click on a link.

[tobie]

I think's it's suboptimal for @wpt-pr-bot to display these rather than have the mirroring system post a comment when it's done mirroring.

[RByers]

Sure, that would certainly be better if it's not too hard. @sideshowbarker?

Also, we still need to change the mirroring bot to auto-mirror for non-contributors when all files are safe, right? @sideshowbarker are you the one who knows that code best?

[sideshowbarker]

I think's it's suboptimal for @wpt-pr-bot to display these rather than have the mirroring system post a comment when it's done mirroring.
Sure, that would certainly be better if it's not too hard. @sideshowbarker are you the one who knows that code best?

I’m not personally very familiar with the script that does the mirroring. I’ve made some minor hacks to it in the past but nothing more than that.

The source is at https://github.com/w3c/github_sync

Also, we still need to change the mirroring bot to auto-mirror for non-contributors when all files are safe, right?

I especially am not familiar with the mechanism which any of the existing bots use to post comments to github issues. I assume it’s some relatively simple thing that can be done with the github API but it’s not something I’ve ever used myself.

So if we want to have the mirroring script also post comments after it completes, it might be most productive (or at least quicker) if somebody already familiar with commenting mechanism were to raise a PR against https://github.com/w3c/github_sync with a patch. I would be happy to review and push it to w3c-test.org when it’s ready.

But if nobody else gets around to it, I can eventually make some time to look at writing it up myself.

[RByers]

Ok, thanks! @bobholt want to add this to your list?

[bobholt]

Sure thing, @RByers.

[bobholt]

A couple of questions:

The prbuildbot edits its existing comments on a new push or reopened PR. When this gets a new push, sync comment, or reopened PR, do you want to edit the existing comment or create a new one? I could probably even do something tricky like delete the original comment on sync start and create a new one on sync end, which could alleviate the condition of following a link to an old checkout.

The github_sync script doesn't know what has changed. It blindly checks out the PR, so the comment will have to be generic like "These tests are now available on w3c-test.org." If there's a link, it would only be to https://w3c-test.org/submissions/<prnumber>, not to the specific test directories. Is that still valuable?

[tobie]

The prbuildbot edits its existing comments on a new push or reopened PR. When this gets a new push, sync comment, or reopened PR, do you want to edit the existing comment or create a new one? I could probably even do something tricky like delete the original comment on sync start and create a new one on sync end, which could alleviate the condition of following a link to an old checkout.

I'd add the URLs at the bottom of the initial comment, similar to how I've added preview and diff links here: https://github.com/heycam/webidl/pull/304. Ideally inside a details element.

[bobholt]

@tobie: Sorry, my explanation conflated the two different bots. The prbuildbot would not update this comment - the links its adding are in its own comments (the comments now coming from @w3c-bots in each PR), and actually come not from the bot, but from the log output of check_stability.py. I was using it as an example of a bot that updated its comment.

I'm not sure what user is represented by https://github.com/w3c/github_sync, but that would be the user commenting with either no link or a generic link to https://w3c-test.org/submissions/<prnumber>, which would not be updated with discrete links to tests.

The more I think about it, the delete/re-comment functionality makes the most sense to me. It can also be leveraged to delete its comment when an w3c-test:unmirror command is sent.

[tobie]

I'm not sure what user is represented by https://github.com/w3c/github_sync, but that would be the user commenting with either no link or a generic link to https://w3c-test.org/submissions/, which would not be updated with discrete links to tests.

I think discrete links to tests is precisely where the value resides. How does the prbuildbot know which tests to run? Can this be somehow exposed?

The more I think about it, the delete/re-comment functionality makes the most sense to me. It can also be leveraged to delete its comment when an w3c-test:unmirror command is sent.

It's similar to what I was suggesting, except I used the issue itself rather than a comment.

[RByers]

The most important thing tracked by this issue IMHO is getting the mirroring occurring automatically in most cases (while remaining secure of course). If it's easy to post a simple "Available at http://w3c-test.org/submissions//" comment once mirroring is complete, that's great.

In addition we've been talking about the value of having links to the specific files that were changed (or a single link to the longest path in common amongst all changes). If that's non-trivial, then perhaps we should break that work out into a separate issue and discuss that there? I wouldn't want to block the 1st bit on that nice-to-have feature.

[bobholt]

I think discrete links to tests is precisely where the value resides. How does the prbuildbot know which tests to run? Can this be somehow exposed?

The check_stability script which runs on Travis figures this out: https://github.com/w3c/web-platform-tests/blob/master/check_stability.py#L401-L454. The build bot only gets that information because check_stability adds it to the Travis log and the bot parses it back out. The build bot is essentially a parrot - it reads the Travis log and comments to GitHub all of the lines that match r'^([A-Z])+:check_stability:'

The github_sync script, however only does a blind checkout of the PR. It doesn't know what tests have changed without doing that same check, so we are making a trade-off by assigning it commenting responsibility.

Getting that functionality in there is possible, but before doing it, it might be worth sitting down, mapping out all of these services, bots, and triggers, and re-assigning responsibility in a more holistic way. We may be progressing to a point with all of these features where if we want them, it makes sense to build something that _does_ maintain state and monitors all of these happenings. That's a big bite to take, though, and we've pointedly been avoiding it.

[tobie]

Removed the testing info from @wpt-pr-bot's comments which is now back to focusing on labelling and finding owners.

[bobholt]

Update: I opened a PR for the commenting behavior in github_sync on February 14. It hasn't landed yet.

[bobholt]

Update: that PR was merged and deployed. Whatever is mirrored will automatically comment to the page now.

[bobholt]

@tobie: What is the state of this? I was going to propose closing it as Done, but I'm seeing some PRs not mirroring. My quick survey seems like the tests in the new CSS directories aren't mirroring, but everything else is, though that may not be the actual root issue.

Some of the ones that mirror are Python-only changes and probably shouldn't. The first PR in mirrored is mine. I'm just a "contributor," so I don't think it has to do with my repo permissions.

Mirrored:

• https://github.com/w3c/web-platform-tests/pull/5231
• https://github.com/w3c/web-platform-tests/pull/5303
• https://github.com/w3c/web-platform-tests/pull/5276

Not Mirrored:

• https://github.com/w3c/web-platform-tests/pull/5316
• https://github.com/w3c/web-platform-tests/pull/5287
• https://github.com/w3c/web-platform-tests/pull/5271

[tobie]

@bobholt you want to ask @sideshowbarker.

[sideshowbarker]

About https://github.com/w3c/web-platform-tests/issues/4636#issuecomment-291490278 will take a look at the specific PRs and the logs and troubleshoot what the cause might be

[gsnedders]

5525 doesn't seem to be mirrored even after commenting and reopening.

[sideshowbarker]

Not sure how much of the specific problem cases are moot at this point but here’s a summary:

• #5316 is not mirrored but is merged now so would be un-mirrored at this point anyway
• #5287 is not mirrored but is closed now so would be un-mirrored at this point anyway
• #5271 is not mirrored but is merged now so would be un-mirrored at this point anyway

So… not sure what to do at this point as far as going back to troubleshoot what might have caused mirroring to fail for those but I suspect is was probably either just timeouts (which happen intermittently) or else the filesystem reached 100%.

The filesystem was quite full until today when I went in there and manually rm’ed some old PR dirs, and when I think it’s 100% full it can actually get un-full again if a PR is merged or closed, which would explain why some later PRs got mirrored.

Anyway I will be keeping any eye on the mirroring and stability-checker/commenting bots this week more actively, so if you notice specific failures and let me know (IRC is the best place to ping me), then I will try to act quickly to go in and look at the logs and see if I can spot the particular cause (waiting until later is harder, because the logs are quite noisy so it ends up being pretty time-consuming to wade through them after).

[bobholt]

From what I understand, auto-mirroring is in place and the only issues we're seeing are w3c-test.org server issues that cause the script to fail.

Can this be considered resolved and we deal with server issues as they arise?

[sideshowbarker]

Can this be considered resolved and we deal with server issues as they arise?

SGTM

As far as the server issues, I’m now checking for failures in the the Recent Deliveries for the mirroring webhook every day, and redelivering any failures I find.