Bug 155713 – Refreshing IDB WPT tests

• Component: Tools / Tests
• Status: RESOLVED DUPLICATE

[youenn fablet]

We should update IndexedDB tests to the same revision as other imported tests.

[youenn fablet]

Created attachment 274577 [details]
Patch

[youenn fablet]

(In reply to comment #1)
> Created attachment 274577 [details]
> Patch

Patch is refreshing IDB tests.
It also imports some tests that were not imported previously.
Most of these "new" tests are failing, I am not sure what is the plan here.

[Alex Christensen]

This looks good to me, but I'd like Brady to look at it before landing.  I'm not sure what the plan is for the new failing tests

[Brady Eidson]

(In reply to comment #2)
> (In reply to comment #1)
> > Created attachment 274577 [details]
> > Patch
> 
> Patch is refreshing IDB tests.
> It also imports some tests that were not imported previously.
> Most of these "new" tests are failing, I am not sure what is the plan here.

What are the natures of the failures?

Modern IDB is past the point of landing a huge batch of failures to wade through later. It's surprising to me the new batch would have a ton of failures. It seems more likely to me that one or two systemic issues are are play.

[youenn fablet]

(In reply to comment #4)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Created attachment 274577 [details]
> > > Patch
> > 
> > Patch is refreshing IDB tests.
> > It also imports some tests that were not imported previously.
> > Most of these "new" tests are failing, I am not sure what is the plan here.
> 
> What are the natures of the failures?

Apparently missing methods like index.getAll, indexGetAllKeys, store.openKeyCursor.
More details can be found in the expected.txt new files.

[Brady Eidson]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (In reply to comment #1)
> > > > Created attachment 274577 [details]
> > > > Patch
> > > 
> > > Patch is refreshing IDB tests.
> > > It also imports some tests that were not imported previously.
> > > Most of these "new" tests are failing, I am not sure what is the plan here.
> > 
> > What are the natures of the failures?
> 
> Apparently missing methods like index.getAll, indexGetAllKeys,
> store.openKeyCursor.
> More details can be found in the expected.txt new files.

Those things don't exist in the published IndexedDB spec (http://www.w3.org/TR/IndexedDB/)

Where did these tests come from? Are they for IDB v2? (https://w3c.github.io/IndexedDB/)

[youenn fablet]

Some new failing tests come from  https://github.com/w3c/web-platform-tests/commit/1121cbc8150ab65b716e78c2fe44a8907a872c53

Other new/failing tests (the ones for which methods are missing) have been imported to WPT from Blink and apparently refer to IDB v2.
See https://github.com/w3c/web-platform-tests/commit/6211ac6f68b5eb70af9dd848995c385aa0084be5

[Brady Eidson]

(In reply to comment #7)
> Some new failing tests come from 
> https://github.com/w3c/web-platform-tests/commit/
> 1121cbc8150ab65b716e78c2fe44a8907a872c53
> 
> Other new/failing tests (the ones for which methods are missing) have been
> imported to WPT from Blink and apparently refer to IDB v2.
> See
> https://github.com/w3c/web-platform-tests/commit/
> 6211ac6f68b5eb70af9dd848995c385aa0084be5

Can we try this again just with the non-blink tests?

[youenn fablet]

Created attachment 274685 [details]
Skipping import of some tests

[youenn fablet]

(In reply to comment #9)
> Created attachment 274685 [details]
> Skipping import of some tests

I removed the IDBv2 tests from the patch.
I also marked the IDBv2 tests as skipped in LayoutTests/imported/w3c/resources/ImportExpectations.
If importing them, please make sure to update the ImportExpectations file so that resync all tests does not actually remove these files.

[Darin Adler]

Comment on attachment 274685 [details]
Skipping import of some tests

View in context: https://bugs.webkit.org/attachment.cgi?id=274685&action=review

> LayoutTests/TestExpectations:310
> +imported/w3c/web-platform-tests/IndexedDB//writer-starvation.htm [ Slow ]
> +imported/w3c/web-platform-tests/IndexedDB//writer-starvation.htm [ Slow ]
> +imported/w3c/web-platform-tests/IndexedDB//idbdatabase_createObjectStore10-1000ends.htm [ Slow ]
> +imported/w3c/web-platform-tests/IndexedDB//idbdatabase_createObjectStore8-parameters.htm [ Slow ]
> +imported/w3c/web-platform-tests/IndexedDB//idbobjectstore_createIndex3-usable-right-away.htm [ Slow ]
> +imported/w3c/web-platform-tests/IndexedDB//idbobjectstore_createIndex8-valid_keys.htm [ Slow ]
> +imported/w3c/web-platform-tests/IndexedDB//key_valid.html [ Slow ]
> +imported/w3c/web-platform-tests/IndexedDB//keypath_maxsize.htm [ Slow ]
> +imported/w3c/web-platform-tests/IndexedDB//writer-starvation.htm [ Slow ]

What’s going on with the double slashes in these paths?

> LayoutTests/imported/w3c/web-platform-tests/IndexedDB/abort-in-initial-upgradeneeded.html:5
> -<script src="../../../resources/testharness.js"></script>
> -<script src="../../../resources/testharnessreport.js"></script>
> +<script src=/resources/testharness.js></script>
> +<script src=/resources/testharnessreport.js></script>

So these tests need to be run with a server? I’m a little concerned about this, especially as a trend. Is there no way to run them from file URLs? Typically server-only tests are harder to use to diagnose individual bugs.

[youenn fablet]

(In reply to comment #11)
> Comment on attachment 274685 [details]
> Skipping import of some tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274685&action=review
> 
> > LayoutTests/TestExpectations:310
> > +imported/w3c/web-platform-tests/IndexedDB//writer-starvation.htm [ Slow ]
> > +imported/w3c/web-platform-tests/IndexedDB//writer-starvation.htm [ Slow ]
> > +imported/w3c/web-platform-tests/IndexedDB//idbdatabase_createObjectStore10-1000ends.htm [ Slow ]
> > +imported/w3c/web-platform-tests/IndexedDB//idbdatabase_createObjectStore8-parameters.htm [ Slow ]
> > +imported/w3c/web-platform-tests/IndexedDB//idbobjectstore_createIndex3-usable-right-away.htm [ Slow ]
> > +imported/w3c/web-platform-tests/IndexedDB//idbobjectstore_createIndex8-valid_keys.htm [ Slow ]
> > +imported/w3c/web-platform-tests/IndexedDB//key_valid.html [ Slow ]
> > +imported/w3c/web-platform-tests/IndexedDB//keypath_maxsize.htm [ Slow ]
> > +imported/w3c/web-platform-tests/IndexedDB//writer-starvation.htm [ Slow ]
> 
> What’s going on with the double slashes in these paths?
> 

This is an error, although this causes no harm (tests will not be imported)

> > LayoutTests/imported/w3c/web-platform-tests/IndexedDB/abort-in-initial-upgradeneeded.html:5
> > -<script src="../../../resources/testharness.js"></script>
> > -<script src="../../../resources/testharnessreport.js"></script>
> > +<script src=/resources/testharness.js></script>
> > +<script src=/resources/testharnessreport.js></script>
> 
> So these tests need to be run with a server? I’m a little concerned about
> this, especially as a trend. Is there no way to run them from file URLs?
> Typically server-only tests are harder to use to diagnose individual bugs.

WPT tests are supposed to be run through wptserve. There is no 'http' folder like in WebKit LayoutTests.
Some tests can easily be converted as file tests, some require wptserve.
It is also closer to regular user environment which is good to catch bugs, probably less to debug them.
I am not sure how flakiness is impacted though.

[youenn fablet]

Created attachment 274826 [details]
Fixing double slashes

[Alex Christensen]

(In reply to comment #12)
> > > LayoutTests/imported/w3c/web-platform-tests/IndexedDB/abort-in-initial-upgradeneeded.html:5
> > > -<script src="../../../resources/testharness.js"></script>
> > > -<script src="../../../resources/testharnessreport.js"></script>
> > > +<script src=/resources/testharness.js></script>
> > > +<script src=/resources/testharnessreport.js></script>
> > 
> > So these tests need to be run with a server? I’m a little concerned about
> > this, especially as a trend. Is there no way to run them from file URLs?
> > Typically server-only tests are harder to use to diagnose individual bugs.
> 
> WPT tests are supposed to be run through wptserve. There is no 'http' folder
> like in WebKit LayoutTests.
> Some tests can easily be converted as file tests, some require wptserve.
> It is also closer to regular user environment which is good to catch bugs,
> probably less to debug them.
> I am not sure how flakiness is impacted though.
I think we need many, many more tests that use a server to test browsers' loading code.  This, however, does not appear to test the loading code, and it doesn't seem to benefit from loading from a server vs. from a file.  I don't think this change is an improvement.  Why was this change made in the w3c tests?

[youenn fablet]

(In reply to comment #14)
> (In reply to comment #12)
> > > > LayoutTests/imported/w3c/web-platform-tests/IndexedDB/abort-in-initial-upgradeneeded.html:5
> > > > -<script src="../../../resources/testharness.js"></script>
> > > > -<script src="../../../resources/testharnessreport.js"></script>
> > > > +<script src=/resources/testharness.js></script>
> > > > +<script src=/resources/testharnessreport.js></script>
> > > 
> > > So these tests need to be run with a server? I’m a little concerned about
> > > this, especially as a trend. Is there no way to run them from file URLs?
> > > Typically server-only tests are harder to use to diagnose individual bugs.
> > 
> > WPT tests are supposed to be run through wptserve. There is no 'http' folder
> > like in WebKit LayoutTests.
> > Some tests can easily be converted as file tests, some require wptserve.
> > It is also closer to regular user environment which is good to catch bugs,
> > probably less to debug them.
> > I am not sure how flakiness is impacted though.
> I think we need many, many more tests that use a server to test browsers'
> loading code.  This, however, does not appear to test the loading code, and
> it doesn't seem to benefit from loading from a server vs. from a file.  I
> don't think this change is an improvement.  Why was this change made in the
> w3c tests?

The proposed change is not modifying anything there, ../../../resources/testharnessreport.js and /resources/testharnessreport.js resolve to the same http URL.
All tests located within LayoutTests/imported/w3c/web-platform-tests are served through wptserve.

To run those tests directly as file tests, the simplest approach would be to move these tests out of LayoutTests/imported/w3c/web-platform-tests.
This would mean reverting bug 155581.

Some potential disadvantages of this approach are if that increases substantially flakiness and/or test run time.
AFAIK, this is not the case currently.
I think the benefits of keeping WebKit WPT tests as close to W3C repo currently outweights the disadvantages.

[youenn fablet]

> So these tests need to be run with a server? I’m a little concerned about
> this, especially as a trend. Is there no way to run them from file URLs?
> Typically server-only tests are harder to use to diagnose individual bugs.

FWIW, bug 152486 is about making it easy to launch wptserve.
What is the additional difficulty that we are facing with http-served tests over file-served tests?

[Darin Adler]

(In reply to comment #16)
> What is the additional difficulty that we are facing with http-served tests
> over file-served tests?

The additional difficulty is the need to run a server process just to try the test out singly in a browser or even when running WebKitTestRunner under the debugger. It's really easy to run a file-served test in Safari by just dragging the test file to the Safari window. I do that *all* the time, and I would not want to sacrifice that unless it's necessary. Running the test server is one more step and it's nice to not have to bother with it for most tests.

Perhaps the web-platform-tests are all dependent on the server and thus they all fail when file-served already? I don't have much experience with them. If most work when file-served, it would be really nice to keep that working. It's really convenient when debugging.

[youenn fablet]

(In reply to comment #17)
> (In reply to comment #16)
> > What is the additional difficulty that we are facing with http-served tests
> > over file-served tests?
> 
> The additional difficulty is the need to run a server process just to try
> the test out singly in a browser or even when running WebKitTestRunner under
> the debugger. It's really easy to run a file-served test in Safari by just
> dragging the test file to the Safari window. I do that *all* the time, and I
> would not want to sacrifice that unless it's necessary. Running the test
> server is one more step and it's nice to not have to bother with it for most
> tests.
> 
> Perhaps the web-platform-tests are all dependent on the server and thus they
> all fail when file-served already? I don't have much experience with them.
> If most work when file-served, it would be really nice to keep that working.
> It's really convenient when debugging.

Most of the wpt tests can probably be file-served.

One just needs to add "convert_test_harness_links" in LayoutTests/imported/w3c/resources/TestRepositories import_options and reimport all tests. That wil make all testharness*.js URLs resolvable to LayoutTests/resources in case test is file-served.

The downsides is that it gives a sense that it is ok to file serve the tests while the test behavior might sometimes differ: WPT testharness.js not in sync with LayoutTests/resources one or test requiring wpt server typically.

Also, by narrowing the gap between WebKit tests and WPT tests, we might be able to get rid of the W3C test importer alltogether.

[Brady Eidson]

(In reply to comment #15)
> I think the benefits of keeping WebKit WPT tests as close to W3C repo
> currently outweights the disadvantages.

I'm definitely not convinced and currently agree with Darin.

file: layout tests are, in practice, *way* easier to deal with than http layout tests.

The tests in LayoutTests/http should be about testing http-specific things and not just be "http as a matter of convenience"

[youenn fablet]

(In reply to comment #19)
> (In reply to comment #15)
> > I think the benefits of keeping WebKit WPT tests as close to W3C repo
> > currently outweights the disadvantages.
> 
> I'm definitely not convinced and currently agree with Darin.
> 
> file: layout tests are, in practice, *way* easier to deal with than http
> layout tests.
> 
> The tests in LayoutTests/http should be about testing http-specific things
> and not just be "http as a matter of convenience"

There are several related topics here.
To make progress, it might be good to discuss each one separately.

1. Is this patch an improvement?
Since bug 155581, the testharness* links do not work for IDB tests if file-served.
In addition to other improvements, this patch is making these links consistent with other WPT tests. To me, it would be good to land it as is (except if reverting bug 155581, see point 3).

2. There is the question about testharness links being valid when loaded either through wptserve (bots) or file served (people).
This is possible by using the convert_test_harness_links options.
I am not opposing to it since it is fully automated by W3C test importer.
That said, there will be an uncertainty as to whether a test runs the same if file-served or through wptserve. 

3. There is the question of running WPT tests on bots without wptserve whenever possible.
Doing that for tests within LayoutTests/imported/w3c/web-platform-tests would be too complex.
For IDB tests, that would mean reverting bug 155581, i.e. moving them back to their own separate folder.

This makes it more difficult to keep them in sync and regularly updated.
This also requires checking for the initial import (and each reimport) that a test is not needing wptserve or some non-testharness resources in web-platform-test folder.

From what I saw in WebKit, once initially imported, the WPT tests tend to be never resynced.
Resyncing allows increasing the quality of existing tests.
I saw it several times that flaky tests were fixed in WPT but not in WebKit.
There are some improvements in W3C IDB tests that WebKit would benefit.
Resynced tests sometimes allow identifying new bugs in WebKit.

4. There is the question of modifying WPT tests so that they can be file served except when tests are focused on testing a specific HTTP behavior.
I don't think this is a good idea unless the modification is automated and, ideally, integrated to W3C test importer.

[Alexey Proskuryakov]

Youenn, as part of this investigation, could you compare the performance of running the tests from files vs. running them using wptserve?

[youenn fablet]

(In reply to comment #21)
> Youenn, as part of this investigation, could you compare the performance of
> running the tests from files vs. running them using wptserve?

I can try identifying how many imported WPT tests are passing when file-served with the same expected.txt files. After excluding timing out tests, it should be fair to compare the overall running time. Is it what you have in mind?

[Alexey Proskuryakov]

That would be a useful experiment. Another good thing to check is tests other than IndexedDB - they may have different performance characteristics, so this data can inform overall direction.

[youenn fablet]

I did the following quick experiment:
- Reimport all tests (Tools/Scripts/import-w3c-tests) after adding "convert_test_harness_links" to LayoutTests/imported/w3c/resources/TestRepositories import_options
- Switch between wptserve to file-serve WPT tests by commenting/uncommenting lines 260 & 261 of Tools/Scripts/webkitpy/port/driver.py (test_to_uri method)
- Run the tests with rwt --child-processes=2

There were no test output difference for IDB tests between wpt-served and file-served.
For the 246 selected/not-timing-out IDB tests, the run takes roughly 40 seconds with wptserve and 2 to 3 seconds less when file-served.


I also checked web-platform-tests/html/dom.
In that case, 4 tests out of 250 do not work properly.
9 additional tests have different output and take more time to finish if file-served than wpt-served.
The run takes 25 seconds with wptserve and 4 seconds less when file-served, once all 11 tests are removed.
When adding the 9 tests with different output, the run takes 50 seconds with wptserve and more tan 1:10 minute if file-served.


For the bigger web-platform-tests/html, there are lots of tests that gets broken if file-served.
Many of these tests use URLS like "/common/media.js", "/resources/WebIDLParser.js" or "/images/blue-100x100.png", "/media/movie_5...", either in the mark-up or in scripts.


These are very rough measurements.
In terms of functionality, making testharness* links relative to LayoutTests may help debugging purposes, this is working great for "IDB", not so much for "html" currently.
In terms of performances, for fast running tests, there seems to be a small (10%?) performance decrease caused by wptserve.
But overall run time heavily depends on longer-running or timing-out tests (like xhr timeouts or idl based tests).

[Alexey Proskuryakov]

> In terms of performances, for fast running tests, there seems to be a small (10%?) performance decrease caused by wptserve.

This seems like a lot to me.

> But overall run time heavily depends on longer-running or timing-out tests (like xhr timeouts or idl based tests).

That should be largely mitigated when running the whole test suite. We run more parallel processes than there are physical cores, so having a few sit idle for 30 seconds is OK, and it doesn't affect overall time.

[youenn fablet]

As part of bug 158872, I plan to refresh WPT tests.
I do not plan right now to refresh IDB tests but can also do that for them.
This would correct a few bugs, such as the ones in this patch.
Please let me know.

[youenn fablet]

As part of bug 163205, I plan to refresh IDB WPT tests like done for other imported WPT tests. Please let me know if there is any objection.

[Brady Eidson]

(In reply to comment #27)
> As part of bug 163205, I plan to refresh IDB WPT tests like done for other
> imported WPT tests. Please let me know if there is any objection.

Are you planning on including the tests for 2.0 features we don't support?

[youenn fablet]

(In reply to comment #28)
> (In reply to comment #27)
> > As part of bug 163205, I plan to refresh IDB WPT tests like done for other
> > imported WPT tests. Please let me know if there is any objection.
> 
> Are you planning on including the tests for 2.0 features we don't support?

All IDB tests have been imported.

It is possible to mark the 2.0 tests as skipped in LayoutTests/imported/w3c/resources/ImportExpectations.
But I haven't seen a specific 2.0 directory, so they should be marked individually.

That way, next time I do a full resync, these tests will be removed from WebKit repository.

[youenn fablet]

*** This bug has been marked as a duplicate of bug 163205 ***