Bug 29867 - [Qt] QWebPage autotest has three failures
Summary: [Qt] QWebPage autotest has three failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on: 29803 29844
Blocks: Qt46
  Show dependency treegraph
 
Reported: 2009-09-29 06:00 PDT by Simon Hausmann
Modified: 2011-04-28 17:25 PDT (History)
4 users (show)

See Also:


Attachments
idea of patch (584 bytes, patch)
2009-09-30 05:35 PDT, Jędrzej Nowacki
jedrzej.nowacki: commit-queue-
Details | Formatted Diff | Diff
patch - test change (2.35 KB, patch)
2009-10-01 01:39 PDT, Jędrzej Nowacki
hausmann: review-
Details | Formatted Diff | Diff
xfail (2.22 KB, patch)
2009-10-08 04:23 PDT, Jędrzej Nowacki
hausmann: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2009-09-29 06:00:19 PDT
Current two tests are failing in tst_qwebpage:

    * testEnablePersistentStorage
    * testOptionalJSObjects
Comment 1 Simon Hausmann 2009-09-29 07:09:32 PDT
testOptionalJSObjects() is also failing. Jedrzej is looking into it :)
Comment 2 Jędrzej Nowacki 2009-09-30 05:35:50 PDT
Created attachment 40363 [details]
idea of patch

Ok, results of "looking"...

testEnablePersistentStorage works after fix for 29844 bug.

test testOptionalJSObjects is broken because of DOMWindow.cpp. I attached patch. 
The patch SHOULD _NOT_ be committed, only reviewed. I think it is good, but it was reverted in bug 28359
(https://bugs.webkit.org/show_bug.cgi?id=28359) because:
"made a change to return 'null' for window.applicationCache when the feature is disabled in the preferences."

According to w3 documentation (http://www.whatwg.org/specs/web-apps/current-work/#application-cache-api) window.applicationCache should return an object to AplicationCache. So I think it is pretty normal to return null if cache is disabled (there is no object).

so, why this patch is bad?
Comment 3 Kenneth Rohde Christiansen 2009-09-30 05:49:01 PDT
I suggest you make that comment to the other bug.
Comment 4 Jędrzej Nowacki 2009-10-01 01:39:12 PDT
Created attachment 40428 [details]
patch - test change

After short discussion with Simon, we decided that test was buggy. It checked undocumented feature. Application cache is tested in Layout Tests, so we should check only Qt API.
Comment 5 Laszlo Gombos 2009-10-02 05:51:01 PDT
(In reply to comment #4)
> Created an attachment (id=40428) [details]
> patch - test change
> 
> After short discussion with Simon, we decided that test was buggy. It checked
> undocumented feature. Application cache is tested in Layout Tests, so we should
> check only Qt API.

Not a reviewer myself but as the author of the test here are some comments:

1./ I'm not sure if there is a need to test interference between QWebPage settings instances. My intention was to test the interference on the JS engine level (as the JS engine is shared to some degree); that is that evaluateJavaScript() able to detect the presence/lack of the particular JS object. This of course only possible if there is a case where evaluateJavaScript() detects that the JS object is not present, which does not seems to be the case for App cache - which brings to my second point.

2./ Layoutests only tests if App cache (when enabled) functions properly. If there is a setting to turn App cache off, than there is to be a definition/spec/test for the case when it is turned off. 

If the 

-QCOMPARE(testFlag(webPage1, QWebSettings::OfflineWebApplicationCacheEnabled, "applicationCache", false), false);

test is removed than there is no behavior test for what happens if App Cache is turned off.

3./ In general JS should be able to detect if a particular JS API is enabled or not; this was also a goal of this test. The proposed patch does not demonstrates this.

----

This bug relates to a recent webkit-dev discussion - https://lists.webkit.org/pipermail/webkit-dev/2009-September/009982.html

For now we should maybe just comment out the failing line as I do agree that it is a priority that all autotests are passing.
Comment 6 Jędrzej Nowacki 2009-10-05 09:43:08 PDT
> Not a reviewer myself but as the author of the test here are some comments:
> 
> 1./ I'm not sure if there is a need to test interference between QWebPage
> settings instances. My intention was to test the interference on the JS engine
> level (as the JS engine is shared to some degree); that is that
> evaluateJavaScript() able to detect the presence/lack of the particular JS
> object. This of course only possible if there is a case where
> evaluateJavaScript() detects that the JS object is not present, which does not
> seems to be the case for App cache - which brings to my second point.

I suppose that testing interference between instances of JSC on QWebPage is enough and it do the work.

> 2./ Layoutests only tests if App cache (when enabled) functions properly. If
> there is a setting to turn App cache off, than there is to be a
> definition/spec/test for the case when it is turned off. 
> 
> If the 
> 
> -QCOMPARE(testFlag(webPage1, QWebSettings::OfflineWebApplicationCacheEnabled,
> "applicationCache", false), false);
> 
> test is removed than there is no behavior test for what happens if App Cache is
> turned off.
> 3./ In general JS should be able to detect if a particular JS API is enabled or
> not; this was also a goal of this test. The proposed patch does not
> demonstrates this.
> 

I agree it is bad.

> 
> This bug relates to a recent webkit-dev discussion -
> https://lists.webkit.org/pipermail/webkit-dev/2009-September/009982.html
> 

Thanks for link, it was really useful.
------------------------------------------------------------

Ok. First I will try to divide the test to simpler cases and than fight the problem.
Comment 7 Simon Hausmann 2009-10-07 07:51:47 PDT
Comment on attachment 40428 [details]
patch - test change

We should be using an XFAIL instead, as also suggested by Laszlo (implicitly :-) and agreed with in the office :)
Comment 8 Jędrzej Nowacki 2009-10-08 04:23:56 PDT
Created attachment 40864 [details]
xfail

xfail patch
Comment 9 WebKit Commit Bot 2009-10-08 06:02:55 PDT
Comment on attachment 40864 [details]
xfail

Rejecting patch 40864 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
mitting to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/qt/ChangeLog
	M	WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebKit/qt/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/libexec/git-core//git-svn line 469
Comment 10 Simon Hausmann 2009-10-08 06:10:07 PDT
Committed r49294: <http://trac.webkit.org/changeset/49294>