Bug 160529 - Remove old Qt database tests
Summary: Remove old Qt database tests
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-03 15:37 PDT by Jonathan Bedard
Modified: 2016-08-05 13:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.07 KB, patch)
2016-08-03 16:09 PDT, Jonathan Bedard
dbates: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2016-08-03 15:37:22 PDT
There are a number of failing tests which are remnants of QT support, which is no longer relevant.
Comment 1 Jonathan Bedard 2016-08-03 16:09:28 PDT
Created attachment 285287 [details]
Patch
Comment 2 Jonathan Bedard 2016-08-03 16:10:45 PDT
As per <https://bugs.webkit.org/show_bug.cgi?id=103861> and <https://bugs.webkit.org/show_bug.cgi?id=91778>, the removed tests were failing without this pair of functions in the TestRunner, which we are not planning on implementing.
Comment 3 Alex Christensen 2016-08-03 22:00:36 PDT
Comment on attachment 285287 [details]
Patch

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

> LayoutTests/platform/mac-wk2/TestExpectations:-273
> -webkit.org/b/147075 http/tests/cache/disk-cache/disk-cache-disable.html [ Pass Failure Timeout ]

This seems unrelated.

> LayoutTests/storage/resources/storage-close-idle-localstorage-databases-immediately.html:-6
> -            window.testRunner.closeIdleLocalStorageDatabases();

This is implemented in DumpRenderTree.  It was added in https://trac.webkit.org/changeset/136323 
Does this test not pass with WebKit1?
Comment 4 Jonathan Bedard 2016-08-04 08:45:38 PDT
Comment on attachment 285287 [details]
Patch

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

>> LayoutTests/platform/mac-wk2/TestExpectations:-273
>> -webkit.org/b/147075 http/tests/cache/disk-cache/disk-cache-disable.html [ Pass Failure Timeout ]
> 
> This seems unrelated.

This is in connection to r42684 which uncovered the failing tests noted here.  This test is marked as failing but in fact, passes on everything but GTK

>> LayoutTests/storage/resources/storage-close-idle-localstorage-databases-immediately.html:-6
>> -            window.testRunner.closeIdleLocalStorageDatabases();
> 
> This is implemented in DumpRenderTree.  It was added in https://trac.webkit.org/changeset/136323 
> Does this test not pass with WebKit1?

They do pass in WebKit1.  However, the tests were initially added for QT support, so it seems like they are testing irrelevant functionality.  Unless we still intend to support these QT functionalities in WebKit1 and still find it necessary to test for them, it seems appropriate to remove these tests.
Comment 5 Daniel Bates 2016-08-04 16:31:33 PDT
Comment on attachment 285287 [details]
Patch

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

This patch does not make sense. Both the description in this bug (comment #0) and the description in the ChangeLog do not explain how the functions testRunner.{closeIdleLocalStorageDatabases, setStorageDatabaseIdleInterval}(), used to test functionality of local storage, are related to "QT" (sic).

> LayoutTests/ChangeLog:11
> +        Removed tests and references to a number of old tests designed
> +        to support QT.  The two TestRunner functions involved were:
> +             testRunner.closeIdleLocalStorageDatabases
> +             testRunner.setStorageDatabaseIdleInterval

I think when you write "QT" that you are referring to the Qt WebKit port, but I'm unclear these local storage functions relate to that port. As Alex remarked in test LayoutTests/storage/resources/storage-close-idle-localstorage-databases-immediately.html, the function testRunner.closeIdleLocalStorageDatabases() was added in <https://trac.webkit.org/changeset/136323> and only implemented for Mac. The function testRunner.setStorageDatabaseIdleInterval() was added in <https://trac.webkit.org/changeset/122174> and only implemented for Mac.

>>> LayoutTests/platform/mac-wk2/TestExpectations:-273
>>> -webkit.org/b/147075 http/tests/cache/disk-cache/disk-cache-disable.html [ Pass Failure Timeout ]
>> 
>> This seems unrelated.
> 
> This is in connection to r42684 which uncovered the failing tests noted here.  This test is marked as failing but in fact, passes on everything but GTK

Alex is referring to the fact that the change to this line is unrelated to the purpose of this bug to "remove old [QT] database tests" because the test http/tests/cache/disk-cache/disk-cache-disable.html is a disk cache test. That is, it is not a database test. The WebKit OpenSource Project prefers the approach of one fix per bug/patch. So, we should keep this patch focused on removing the Qt database tests and create a new bug/patch to remove the test http/tests/cache/disk-cache/disk-cache-disable.html.

On another note, I'm unclear how r42684 <http://trac.webkit.org/changeset/42684>, which is build fix that was made 7 years ago, relates to this test or any of the other tests removed in this patch.
Comment 6 Jonathan Bedard 2016-08-05 13:02:08 PDT
The blame history for this is a bit of a mess, it is still unclear to me what these tests are supposed to be testing, and it also seems as if the final bug is no longer attached to it's patch in Bugzilla, and the Radars it references don't seem to exist either.

From what I can gather, here are the basics:

https://bugs.webkit.org/show_bug.cgi?id=90713 : Initial proposal of the function
https://bugs.webkit.org/show_bug.cgi?id=103469 : Added the test which uses this funcitonality
https://bugs.webkit.org/show_bug.cgi?id=103876 : Skipped storage/domstorage/localstorage/close-idle-localstorage-databases-immediately.html.

It was this last Bugzilla that connected these tests to the Qt port.  Looking through this more, it seems like actually, this has the opposite problem: Webkit2 needs to implement closeIdleLocalStorageDatabases.
Comment 7 Jonathan Bedard 2016-08-05 13:28:02 PDT
Bugs <https://bugs.webkit.org/show_bug.cgi?id=103861> and <https://bugs.webkit.org/show_bug.cgi?id=91778> remain valid.  Marking resolved as invalid.