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

Reland "Add quota usage details tests for all other storage backends." #14531

Merged
merged 1 commit into from
Jan 24, 2019

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Dec 14, 2018

This is a reland of 548e7b5e622ad79155ff95ef26738d7a40fe9397

Original change's description:

Add quota usage details tests for all other storage backends.

IndexedDB test was included in parent CL.

Bug: 904000
Test:
Change-Id: Icc3462f13d0dce0197a8ec54f22d5ad794a51292
Reviewed-on: https://chromium-review.googlesource.com/c/1334659
Reviewed-by: Victor Costan <[email protected]>
Reviewed-by: Joshua Bell <[email protected]>
Commit-Queue: Jarryd Goodman <[email protected]>
Cr-Commit-Position: refs/heads/master@{#616398}

Bug: 904000
Change-Id: I507ec17eabc8e323a7c6418075d4be4787d4c5df
Reviewed-on: https://chromium-review.googlesource.com/c/1378805
Reviewed-by: Joshua Bell <[email protected]>
Commit-Queue: Jarryd Goodman <[email protected]>
Cr-Commit-Position: refs/heads/master@{#617285}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reviewed downstream.

This is a reland of 548e7b5e622ad79155ff95ef26738d7a40fe9397

Original change's description:
> Add quota usage details tests for all other storage backends.
>
> IndexedDB test was included in parent CL.
>
> Bug: 904000
> Test:
> Change-Id: Icc3462f13d0dce0197a8ec54f22d5ad794a51292
> Reviewed-on: https://chromium-review.googlesource.com/c/1334659
> Reviewed-by: Victor Costan <[email protected]>
> Reviewed-by: Joshua Bell <[email protected]>
> Commit-Queue: Jarryd Goodman <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#616398}

Bug: 904000
Change-Id: I507ec17eabc8e323a7c6418075d4be4787d4c5df
Reviewed-on: https://chromium-review.googlesource.com/c/1378805
Reviewed-by: Joshua Bell <[email protected]>
Commit-Queue: Jarryd Goodman <[email protected]>
Cr-Commit-Position: refs/heads/master@{#617285}
@foolip foolip force-pushed the chromium-export-cl-1378805 branch from 142e1bc to cc2ddff Compare December 19, 2018 14:09
@foolip
Copy link
Member

foolip commented Dec 19, 2018

Taskcluster flakiness in Chrome:

Test Subtest Results Messages
/storage/estimate-usage-details-application-cache.https.tentative.html estimate() shows usage increase after app is cached FAIL: 9/10, PASS: 1/10 assert_greater_than: expected a number greater than 3372 but got 3372

@foolip
Copy link
Member

foolip commented Dec 19, 2018

@jarryd999 @inexorabletash, see #14531 (comment) for reported flakiness of estimate-usage-details-application-cache.https.tentative.html. Is this expected to happen on Chrome Dev because of some code change that hasn't reached the dev channel yet, or might there be a problem with the test?

@foolip
Copy link
Member

foolip commented Jan 23, 2019

I was able to reproduce the flakiness with the changes in this PR rebased on master (commit 27085bc) running ./wpt run --verify --channel dev --binary which google-chrome-unstable --log-tbpl - chrome /storage/estimate-usage-details-application-cache.https.tentative.html:

/storage/estimate-usage-details-application-cache.https.tentative.html

Subtest Results Messages
OK
estimate() shows usage increase after app is cached FAIL: 5/10, PASS: 5/10 assert_greater_than: expected a number greater than 3364 but got 3364

Confusingly, the same command on master seems to pass, but that's because the test didn't exist before.

@foolip
Copy link
Member

foolip commented Jan 23, 2019

Like in #15015, seeing consistent 5/10 fail makes me suspect there's no randomness to this, just some test/infra issue.

@foolip
Copy link
Member

foolip commented Jan 23, 2019

Ah, it is a lot like #15015. If the cache is global and the test runs in parallel, then only one of the tests will add to the size of the cache...

@foolip
Copy link
Member

foolip commented Jan 23, 2019

@jarryd999 @inexorabletash @pwnall, this hasn't been merged in wpt yet because of the flakiness, which I think is because the cache is shared.

Do you have thoughts on how to fix this issue? A fix like #15018 doesn't seem as straightforward here, I think one would need a unique appcache.manifest for each load? But how does appcache interact, is it even possible to install arbitrary number of appcaches for the same domain/path/something?

@foolip
Copy link
Member

foolip commented Jan 24, 2019

I've run while true; do ./wpt run --no-pause --channel dev --binary which google-chrome-unstable chrome /storage/estimate-usage-details-application-cache.https.tentative.html || break; done for a while (~15 loops) to confirm that this probably isn't flaky in the usual sense. I'll file an issue and admin merge.

@foolip
Copy link
Member

foolip commented Jan 24, 2019

Filed #15036.

@foolip foolip merged commit 7e5cdea into master Jan 24, 2019
@foolip foolip deleted the chromium-export-cl-1378805 branch January 24, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants