-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Cache Storage API tests: Fix WPT test bugs, remove redundant local copies #5413
Conversation
…pies * Use promise_rejects consistently and fix parameters. * Don't use .php in upstreamed tests. * Add additional test cases from blink's copies. * Tightened up assertions about result array ordering. * Remove blink's redundant copies of tests. I left the credentials tests alone since Blink is mid-deprecation. BUG=655479 [email protected] Review-Url: https://codereview.chromium.org/2790143003 Cr-Commit-Position: refs/heads/master@{#462593}
Notifying @inexorabletash and @wanderview. (Learn how reviewing works.) |
Looks like we have instability on Firefox, but it's not being reported back as comments. @bobholt, any idea why? |
@foolip: Since there are no comments on this issue at all, it could be one of the following:
It looks like all but one PR since this one have commented to GitHub successfully. The other PR that did not was also a chromium-export PR. This is probably a coincidence. If you'd like to know more, we would have to ask @sideshowbarker to check the logs on w3c-test.org. |
@sideshowbarker, are you able to tell what happened? |
At this point I’m not able to tell what happened. I’ll need to look further |
FYI, one of the tests has assertions about ordering that don't hold in FF, leading to flaky failures. (Spec may require work here.) @wanderview and I have discussed an approach to make the test setup more precise which should resolve this; I'll tackle this once my dev machine is back online. That'd end up being a tweak to the test on the Blink side; I'm not sure what happens with this PR, though. |
https://codereview.chromium.org/2806793002/ for a fix (hopefully) on the Blink side. If/when that lands I'm not sure what our upstreaming infrastructure will do with this PR. |
@inexorabletash the exporter only works at the commit level of granularity, but since https://crrev.com/2806793002 won't apply cleanly to WPT@master, I don't think the exporter will be able to make a PR. Once https://crrev.com/2806793002 lands I can apply it on top of this PR if that works for you. |
@jeffcarp - sounds perfect, thanks! |
Chrome and Firefox differ in the order in which cache keys() are returned. Chrome orders by according to when the put()s were issued. Firefox orders by when the body is complete. The test helper prepopulated_cache_test did not guarantee that these matched, leading to the tests being flaky in Firefox. This change tweaks the helper so that the put()s are processed serially so that the order is deterministic for both. Spec issue: w3c/ServiceWorker#823 BUG=655479 Review-Url: https://codereview.chromium.org/2806793002 Cr-Commit-Position: refs/heads/master@{#463195}
These tests are now available on w3c-test.org |
Firefox (nightly channel)Testing web-platform-tests at revision 020907b All results25 tests ran/service-workers/cache-storage/serviceworker/cache-add.https.html
/service-workers/cache-storage/serviceworker/cache-delete.https.html
/service-workers/cache-storage/serviceworker/cache-keys.https.html
/service-workers/cache-storage/serviceworker/cache-match.https.html
/service-workers/cache-storage/serviceworker/cache-matchAll.https.html
/service-workers/cache-storage/serviceworker/cache-put.https.html
/service-workers/cache-storage/serviceworker/cache-storage-match.https.html
/service-workers/cache-storage/serviceworker/cache-storage.https.html
/service-workers/cache-storage/window/cache-add.https.html
/service-workers/cache-storage/window/cache-delete.https.html
/service-workers/cache-storage/window/cache-keys.https.html
/service-workers/cache-storage/window/cache-match.https.html
/service-workers/cache-storage/window/cache-matchAll.https.html
/service-workers/cache-storage/window/cache-put.https.html
/service-workers/cache-storage/window/cache-storage-keys.https.html
/service-workers/cache-storage/window/cache-storage-match.https.html
/service-workers/cache-storage/window/cache-storage.https.html
/service-workers/cache-storage/worker/cache-add.https.html
/service-workers/cache-storage/worker/cache-delete.https.html
/service-workers/cache-storage/worker/cache-keys.https.html
/service-workers/cache-storage/worker/cache-match.https.html
/service-workers/cache-storage/worker/cache-matchAll.https.html
/service-workers/cache-storage/worker/cache-put.https.html
/service-workers/cache-storage/worker/cache-storage-match.https.html
/service-workers/cache-storage/worker/cache-storage.https.html
|
*This report has been truncated because the total content is 66613 characters in length, which is in excess of GitHub.com's limit for comments (65536 characters). Chrome (unstable channel)Testing web-platform-tests at revision 020907b All results25 tests ran/service-workers/cache-storage/serviceworker/cache-add.https.html
/service-workers/cache-storage/serviceworker/cache-delete.https.html
/service-workers/cache-storage/serviceworker/cache-keys.https.html
/service-workers/cache-storage/serviceworker/cache-match.https.html
/service-workers/cache-storage/serviceworker/cache-matchAll.https.html
/service-workers/cache-storage/serviceworker/cache-put.https.html
/service-workers/cache-storage/serviceworker/cache-storage-match.https.html
/service-workers/cache-storage/serviceworker/cache-storage.https.html
/service-workers/cache-storage/window/cache-add.https.html
/service-workers/cache-storage/window/cache-delete.https.html
/service-workers/cache-storage/window/cache-keys.https.html
/service-workers/cache-storage/window/cache-match.https.html
/service-workers/cache-storage/window/cache-matchAll.https.html
/service-workers/cache-storage/window/cache-put.https.html
/service-workers/cache-storage/window/cache-storage-keys.https.html
/service-workers/cache-storage/window/cache-storage-match.https.html
/service-workers/cache-storage/window/cache-storage.https.html
/service-workers/cache-storage/worker/cache-add.https.html
/service-workers/cache-storage/worker/cache-delete.https.html
/service-workers/cache-storage/worker/cache-keys.https.html
/service-workers/cache-storage/worker/cache-match.https.html
/service-workers/cache-storage/worker/cache-matchAll.https.html
/service-workers/cache-storage/worker/cache-put.https.html
/service-workers/cache-storage/worker/cache-storage-match.https.html
/service-workers/cache-storage/worker/cache-storage.https.html
|
@jeffcarp, FYI, I was perfectly happy that the PR was merged automatically after the fix. |
Context: on another PR I was a bit surprised, but agreed that it's probably a good default. |
@jeffcarp, is the rebase strategy now used for merging PRs? That together with parallel exporting, made this a bit confusing. I cherry-picked the commit from #5521 onto this PR since they made sense together, but it looks like #5521 was automatically merged first because it didn't conflict. Then just the remaining commit from the PR was merged. The result is that the commits appear in wpt history in reverse order. However, if I had succeeded in getting https://github.com/w3c/web-platform-tests/pull/5413/commits merged together at the same time, it would have left #5521 as a no-op. I actually anticipated that and was curious to see what would happen :) |
@foolip thanks for testing that out in practice! landing commits out of order is the tradeoff we're making in order to be able to export asynchronously / get around stuck commits - is there something you would change? do you think we should have a way of saying on a PR "exporter, don't automatically merge this?" The exporter specifies that it wants to use the rebase method: |
I guess it came out of beta :) It would be good if the bot looked at the "do not merge yet" label, which I could have added to #5521 in this case. But for the ordering problem more generally, I might suggest that skipping over a commit and thus changing the order should require human intervention. However, that'd hurt the average time to export and most of the time it's fine. Do you expect that after PRs are created for in-progress Chromium reviews and we have the Travis results before landing in Chromium, that we could go back to upstream in order only? If so, then perhaps it's not worth spending any time improving the current situation. All in all, perhaps wait until there's a case where changing the order was a problem before acting. |
After looking through the logs I’m still not sure what all happened. For one thing, the logs are extremely verbose. Anyway as far as w3c-test.org goes, I do know that one problem that happened over the last few days is that we ran out of filesystem space for the The mount for the Another chronic problem we have is that webhook requests to w3c-test.org sometimes just timeout (and then don’t get re-tried). I haven’t been able to see a pattern to what causes them to timeout but I guess one way we can deal with it is to try to configure longer timeouts. I think the merger of the CSS tests may be making the timeout problem worse in the cause of the sync/submissions webhook at least. Anyway in general if you look at the itemization of possible causes in #5413 (comment) from @bobholt, there are some of those causes that are easier to identify and troubleshoot and there are some which are a lot harder and more time-consuming to try to do that for. In those cases I’m not sure what we can do expect re-trigger the webhooks and travis—and just watch/tail the logs when we do. |
@sideshowbarker, is the webhook for w3c-test.org using HTTPS? I frequently fail to reach https://w3c-test.org/ |
Yes the mirroring webhook is configured to use https://w3c-test.org/sync.py. I can try setting it to use http://w3c-test.org/sync.py instead but in the past I recall we had just as many problems with timeouts with http://w3c-test.org/sync.py as we did with https://w3c-test.org/sync.py. Maybe more, actually. |
Hmm, that is surprising, I've never had a problem with http://w3c-test.org and whenever https://w3c-test.org has been giving me trouble switching to HTTP has always seemed to fix the problem. It may all be coincidences, am I the only one who has drawn the conclusion that https://w3c-test.org is just generally unreliable? |
I left the credentials tests alone since Blink is mid-deprecation.
BUG=655479
R=[email protected]
Review-Url: https://codereview.chromium.org/2790143003
Cr-Commit-Position: refs/heads/master@{#462593}
This change is