LGTM3
On 12/13/23 11:04 AM, Rick Byers wrote:
LGTM2
+1 to raising the bar on test automation, but only in the cases where
there aren't unreasonable burdens and there is clear significant test
coverage value (i.e., not this specific case). Thanks for the analysis
Evan and Philip!
On Wed, Dec 13, 2023 at 8:10 AM Philip Jägenstedt
<[email protected]> wrote:
Hi Evan,
Thanks for looking into this, and for answering some of my
questions off-list. To summarize, eviction is spec'd very loosely
and a testdriver.js API could probably only be "evict everything"
or "evict data in this bucket". Neither of those would test the
interesting parts of how quota, expiry time and probably other
signals are used to determine which buckets to evict. Bottom line,
if we can't see a WebDriver endpoint that would poke at the right
internals /in all browsers/, then it wouldn't be testing
production code beyond what is already tested.
It looks like get or expire a bucket
<https://wicg.github.io/storage-buckets/#get-or-expire-a-bucket> could
be tested using bucket.setExpires(), but AFAICT this isn't tested.
So, LGTM1 to ship with added tests for bucket expiration.
Best regards,
Philip
P.S. I disagree that testdriver.js should only be used in
exceptional cases. Rather often there's a spec concept which is
invoked by the user or under implementation-defined conditions,
and it's straightforward to poke this concept with a WebDriver
endpoint. https://fedidcg.github.io/FedCM/#automation is a good
recent example. API owners don't insist on this currently mainly
because testdriver.js needs to be implemented for both Chrome and
content_shell. With wptrunner in Chromium CI (thanks @Weizhong Xia
<mailto:[email protected]>!) I expect the need for double work
will be removed, and I would like to raise the bar for test
automation. (But even then there will be cases where it wouldn't
be testing additional production code and isn't worthwhile.)
On Thu, Dec 7, 2023 at 6:43 PM Evan Stade <[email protected]> wrote:
Hi Philip,
After conferring with the team, we feel that there is an
appropriate distribution of coverage provided by WPT and
Chromium unit and browser tests. Leveraging testdriver for
one-off hooks such as this would introduce substantial
test-only code flows, diminishing the confidence that the
tests were correctly verifying production behavior, while also
imposing a larger maintenance burden going forward. The
benefit, as we understand it, would be that the WPT could be
shared by other vendors, but the test itself in this case
would only be a few lines. The vast majority of LOC would
be plumbing and internals logic which other vendors would need
to provide for themselves.
Getting a bit off in the weeds relative to this i2s
discussion, but I'll continue on this topic for the sake of
debate:
We can observe that new testdriver APIs are seldom added. I
only see ~4 changes originating from the Chromium project in
the last 5 years. Putting all other reasoning aside, this fact
alone would suggest that testdriver should be touched only in
exceptional cases. I would not argue that testdriver is
without utility. Unlike the one-off actions we'd need for
these buckets WPT, more generic testdriver actions such as
setting permission state can enable a wide range of tests. In
those cases the calculus of cost vs reward is much more
favorable. So I would assume that changes to testdriver should
meet a high bar of broad utility, similar to how we treat core
libraries in //base/.
Hope that addresses your concerns.
-- Evan Stade
On Fri, Dec 1, 2023 at 9:03 AM Philip Jägenstedt
<[email protected]> wrote:
Thanks Evan for listing out the tests, they were more
spread out than I thought. Great to see more of the tests
being upstreamed too!
I wonder if we can do even better and test eviction
however? Have you looked into defining a WebDriver
endpoint for triggering eviction? It could make sense in
https://web-platform-tests.org/writing-tests/testdriver.html#storage
and there are plenty of examples in that documentation
that might be close to the hook/trigger you need.
If you think this is an unreasonable request, please push
back, my interest is only that this can be implemented
interoperably. Tests are our best way of ensuring that,
but it's not the only way, and it's always a tradeoff of
effort vs. utility.
Best regards,
Philip
On Wed, Nov 29, 2023 at 6:32 PM Evan Stade
<[email protected]> wrote:
Hi Philip,
Here is a comprehensive list of web tests that verify
storageBuckets behavior. We do feel the size of this
list aligns well with the spec, which is fairly
concise. As you can see, some are spread out in
directories for the other storage APIs that
storageBuckets brokers.
That said, not all behavior is verifiable from the
web. For example, it's not possible to reliably
trigger an automatic eviction event from the web, so
we can't really know if `persisted` is /actually/
respected. Browser tests provide that kind of coverage.
Some of the WPT are internal but they can/should be
moved
<https://chromium-review.googlesource.com/c/chromium/src/+/5072535>
to the external WPT directory. Thanks for the prompt.
external/wpt/IndexedDB/storage-buckets.https.any.js:
external/wpt/fs/script-tests/FileSystemBaseHandle-buckets.js:
external/wpt/service-workers/cache-storage/cache-storage-buckets.https.any.js:
external/wpt/storage/buckets/bucket-quota-indexeddb.tentative.https.any.js:
external/wpt/storage/buckets/bucket-storage-policy.tentative.https.any.js:
external/wpt/web-locks/storage-buckets.tentative.https.any.js:
http/tests/inspector-protocol/storage/cache-storage-request-cache-names.js:
http/tests/inspector-protocol/storage/cache-storage-storage-key-request-cache-names.js:
http/tests/inspector-protocol/storage/indexed-db-storage-key-clear.js:
http/tests/inspector-protocol/storage/indexed-db-storage-key-delete-db.js:
http/tests/inspector-protocol/storage/indexed-db-storage-key-delete-object-store-entries.js:
http/tests/inspector-protocol/storage/indexed-db-storage-key-get-metadata.js:
http/tests/inspector-protocol/storage/indexed-db-storage-key-request-data.js:
http/tests/inspector-protocol/storage/indexed-db-storage-key-request-database-names.js:
http/tests/inspector-protocol/storage/indexed-db-storage-key-request-database.js:
http/tests/inspector-protocol/storage/storage-bucket-storage-key-track-untrack.js:
http/tests/inspector-protocol/storage/storage-buckets-delete-storage-bucket.js:
wpt_internal/storage/buckets/bucket_names.tentative.https.any.js:
wpt_internal/storage/buckets/buckets_basic.tentative.https.any.js:
wpt_internal/storage/buckets/buckets_storage_policy.tentative.https.any.js:
wpt_internal/storage/buckets/detached-iframe.https.html:
wpt_internal/storage/buckets/idlharness-worker.https.any.js:
wpt_internal/storage/buckets/opaque-origin.https.window.js:
wpt_internal/storage/buckets/resources/opaque-origin-sandbox.html:
wpt_internal/storage/buckets/storage_bucket_object.tentative.https.any.js:
-- Evan Stade
On Wed, Nov 29, 2023 at 9:02 AM Philip Jägenstedt
<[email protected]> wrote:
Hi Evan,
Is
https://wpt.fyi/results/storage/buckets?label=experimental&label=master&aligned
<https://wpt.fyi/results/storage/buckets?label=experimental&label=master&aligned>
the full set of tests? Given the size of the spec,
does that cover the whole feature well?
Best regards,
Philip
On Wed, Nov 29, 2023 at 5:58 PM Evan Stade
<[email protected]> wrote:
[reply-all this time]
Hi Philip,
thanks for pointing out those two oversights.
I have fixed the checkboxes on the
chromestatus entry. It is in fact tested by
WPT and supported on all Blink platforms.
On Wed, Nov 29, 2023 at 8:43 AM Philip
Jägenstedt <[email protected]> wrote:
Hi Evan,
A few questions inline.
On Tue, Nov 14, 2023 at 9:38 PM Evan Stade
<[email protected]> wrote:
Will this feature be supported
on all six Blink platforms
(Windows, Mac, Linux, Chrome
OS, Android, and Android WebView)?
No
Which platform will this not be supported on?
Is this feature fully tested
by web-platform-tests
<https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md>?
No
Can this be fully tested in WPT? If it's
not tested in WPT, how is it currently tested?
Best regards,
Philip
--
You received this message because you are subscribed to the Google
Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it,
send an email to [email protected].
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAARdPYfLUBkQCsJG4RYHSUqxLkN42NdMTRWAK817hofLU6CrPA%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAARdPYfLUBkQCsJG4RYHSUqxLkN42NdMTRWAK817hofLU6CrPA%40mail.gmail.com?utm_medium=email&utm_source=footer>.
--
You received this message because you are subscribed to the Google
Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send
an email to [email protected].
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAFUtAY-7zXke53y9TQ44fBtZBUG%2BvM701WT1FXfTBfeVUjS5cg%40mail.gmail.com
<https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAFUtAY-7zXke53y9TQ44fBtZBUG%2BvM701WT1FXfTBfeVUjS5cg%40mail.gmail.com?utm_medium=email&utm_source=footer>.
--
You received this message because you are subscribed to the Google Groups
"blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/a/chromium.org/d/msgid/blink-dev/eb274beb-e2ec-4669-bbba-1354102bd147%40chromium.org.
