Bug 148713

Summary: Import W3C IndexedDB tests
Product: WebKit Reporter: Brady Eidson <beidson>
Component: Tools / TestsAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, youennf
Priority: P2    
Version: Safari 9   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1 thorton: review+

Description Brady Eidson 2015-09-02 09:56:10 PDT
Import W3C IndexedDB tests
Comment 1 Brady Eidson 2015-09-02 12:17:24 PDT
Created attachment 260427 [details]
Patch v1

Checks in all the tests with whatever results they generated, including "failing" results.

Going through the results will be a followup task.

Only a handful crash/assert/timeout, which is great.
Comment 2 Tim Horton 2015-09-02 12:18:43 PDT
Comment on attachment 260427 [details]
Patch v1

rs=me
Comment 3 Brady Eidson 2015-09-02 13:39:35 PDT
https://trac.webkit.org/changeset/189264
Comment 4 Alexey Proskuryakov 2015-09-02 23:33:08 PDT
*** Bug 148712 has been marked as a duplicate of this bug. ***
Comment 5 Alexey Proskuryakov 2015-09-04 12:12:59 PDT
Skipped the tests on ios-simulator-wk1 in r189368.
Comment 6 Alexey Proskuryakov 2015-09-09 09:42:55 PDT
These tests appear super flaky, failing and timing out. Brady, shat should we do?

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=imported%2Fw3c%2Findexeddb
Comment 7 Brady Eidson 2015-09-09 11:21:24 PDT
(In reply to comment #6)
> These tests appear super flaky, failing and timing out. Brady, shat should
> we do?
> 
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#tests=imported%2Fw3c%2Findexeddb

All of them?
Comment 8 Brady Eidson 2015-09-10 09:27:17 PDT
Marked more flaky tests in https://trac.webkit.org/changeset/189569
Comment 9 youenn fablet 2016-03-16 08:13:35 PDT
Is there any reason why these tests were not imported within LayoutTests/imported/w3c/web-platform-tests/IndexedDB?
Should we consider moving them there?
Comment 10 Brady Eidson 2016-03-16 08:38:20 PDT
(In reply to comment #9)
> Is there any reason why these tests were not imported within
> LayoutTests/imported/w3c/web-platform-tests/IndexedDB?

No reason. Thought if they *should* have been imported there, there was no indication.

Why is it preferable that they be there?

> Should we consider moving them there?

We can consider it. What about considering canvas, css, or fonts? Do they belong there as well?

Is there a benefit to moving them there?

If there's no benefit, I don't see why we would bother with the churn involved just to move the,.
Comment 11 youenn fablet 2016-03-16 08:59:51 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > Is there any reason why these tests were not imported within
> > LayoutTests/imported/w3c/web-platform-tests/IndexedDB?
> 
> No reason. Thought if they *should* have been imported there, there was no
> indication.
> Why is it preferable that they be there?

WPT tests are supposed to be served using wptserve which happens only for tests inside LayoutTests/imported/w3c/web-platform-tests/.

Also LayoutTests/imported/w3c/web-platform-tests/ is supposed to be synced regularly with W3C repository. Tooling (import-w3c-tests script) makes reimporting the tests a bit easier if in LayoutTests/imported/w3c/web-platform-tests/.

As a side note, LayoutTests/imported/w3c/web-platform-tests/ is not the place for tests that should remain specific to WebKit, like IDB private tests.

> > Should we consider moving them there?
> 
> We can consider it. What about considering canvas, css, or fonts? Do they
> belong there as well?

I guess it would be good to have canvas and fonts be moved to web-platform-tests folder to keep them in sync with W3C test suites.

As of CSS, it might be good to use the same principle. Note that CSS is in a different repository and should be imported in LayoutTests/imported/w3c/csswg-test. import-w3c-tests script should already be supporting this repository. Also I think CSS tests are scattered in various places.
Comment 12 Brady Eidson 2016-03-16 09:15:53 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Is there any reason why these tests were not imported within
> > > LayoutTests/imported/w3c/web-platform-tests/IndexedDB?
> > 
> > No reason. Thought if they *should* have been imported there, there was no
> > indication.
> > Why is it preferable that they be there?
> 
> WPT tests are supposed to be served using wptserve which happens only for
> tests inside LayoutTests/imported/w3c/web-platform-tests/.
> 
> Also LayoutTests/imported/w3c/web-platform-tests/ is supposed to be synced
> regularly with W3C repository. Tooling (import-w3c-tests script) makes
> reimporting the tests a bit easier if in
> LayoutTests/imported/w3c/web-platform-tests/.
> 
> As a side note, LayoutTests/imported/w3c/web-platform-tests/ is not the
> place for tests that should remain specific to WebKit, like IDB private
> tests.

This is all news to me.

None of the original reviewer for the import or many other reviewers as I've tweaked tests and results in the directory have known about this, either.

Where is this documented?

There is a major hiccup with this plan going forward; There is a manual copy of each test for private browsing.
Comment 13 youenn fablet 2016-03-16 09:27:20 PDT
> None of the original reviewer for the import or many other reviewers as I've
> tweaked tests and results in the directory have known about this, either.
> 
> Where is this documented?

Topic was discussed at 2015 March contributor meeting.
There is also http://trac.webkit.org/wiki/WebKitW3CTesting

> There is a major hiccup with this plan going forward; There is a manual copy
> of each test for private browsing.

Wouldn't it be easier if the private browsing tests were in their own "private" folder, without adding any '-private' prefix?
Comment 14 Brady Eidson 2016-03-16 09:39:56 PDT
(In reply to comment #13)
> > There is a major hiccup with this plan going forward; There is a manual copy
> > of each test for private browsing.
> 
> Wouldn't it be easier if the private browsing tests were in their own
> "private" folder, without adding any '-private' prefix?

The filename/path of the private browsing variant of the tests is not the problem.

The problem is that the private browsing variants of the tests require modifications to the imported w3c test.

In other words, we can't just mechanically reimport the tests without also making new private browsing copies.
Comment 15 Alexey Proskuryakov 2016-03-16 09:56:17 PDT
Can we have the import script make the private copy?
Comment 16 youenn fablet 2016-03-16 10:00:10 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > > There is a major hiccup with this plan going forward; There is a manual copy
> > > of each test for private browsing.
> > 
> > Wouldn't it be easier if the private browsing tests were in their own
> > "private" folder, without adding any '-private' prefix?
> 
> The filename/path of the private browsing variant of the tests is not the
> problem.
> 
> The problem is that the private browsing variants of the tests require
> modifications to the imported w3c test.
> 
> In other words, we can't just mechanically reimport the tests without also
> making new private browsing copies.

Quickly checking the first three IDB tests, private and regular versions are the same. How much are private tests from regular tests? Can these changes be upstreamed to WPT?

Also, why not moving the regular IDB tests to web-platform-tests folder?
The private tests could be synced with the web-platform-tests one from time to time.
Comment 17 youenn fablet 2016-03-16 10:01:08 PDT
(In reply to comment #15)
> Can we have the import script make the private copy?

It seems private copies include some changes that regular ones do not have.
Comment 18 Alexey Proskuryakov 2016-03-16 10:05:55 PDT
I believe that there is special magic enabling private browsing based on file name. We should remove this magic, and have the private copies explicitly enable the mode.
Comment 19 Brady Eidson 2016-03-16 10:16:51 PDT
(In reply to comment #18)
> I believe that there is special magic enabling private browsing based on
> file name. We should remove this magic, and have the private copies
> explicitly enable the mode.

When I was commenting this morning, I thought that the imported tests were different (e.g., private browsing was explicitly enabled)

But, indeed, I can see by inspection that's not the case.

So that's yet another thing that has to be done here if we were to move these tests, a task for which I'm still not quite convinced of the value...
Comment 20 Brady Eidson 2016-03-16 10:18:05 PDT
If it's truly valuable to move these, I'm not going to stand in the way. But I won't be doing it myself - I still have actual test failures to clean up, which is way more important IMHO.
Comment 21 youenn fablet 2016-03-16 13:20:36 PDT
(In reply to comment #20)
> If it's truly valuable to move these, I'm not going to stand in the way. But
> I won't be doing it myself - I still have actual test failures to clean up,
> which is way more important IMHO.

Goal is just about that, getting more/better tests for free and hopefully less test failures over time ;)

I'll take care of moving regular tests.

I am unsure about the best way to handle private tests.
If more and more regular tests are also run as private, there might be a better way than duplicating test files.
Comment 22 Brady Eidson 2016-03-16 13:37:28 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > If it's truly valuable to move these, I'm not going to stand in the way. But
> > I won't be doing it myself - I still have actual test failures to clean up,
> > which is way more important IMHO.
> 
> Goal is just about that, getting more/better tests for free and hopefully
> less test failures over time ;)
> 
> I'll take care of moving regular tests.
> 
> I am unsure about the best way to handle private tests.
> If more and more regular tests are also run as private, there might be a
> better way than duplicating test files.

The "running regular tests as private" concept is not global to all tests; It is specifically about indexed database tests. And this is because whether you're in private browsing or not you get a different backend... both of which need to be tested.
Comment 23 Alexey Proskuryakov 2016-03-16 14:25:58 PDT
Right, I think that the import script should duplicate IndexedDB tests, modifying one copy to enable private browsing.