Summary: | [Qt] QWebPage autotest has three failures | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Hausmann <hausmann> | ||||||||
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cmarcelo, jedrzej.nowacki, kenneth, laszlo.gombos | ||||||||
Priority: | P2 | Keywords: | Qt | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 29803, 29844 | ||||||||||
Bug Blocks: | 29799 | ||||||||||
Attachments: |
|
Description
Simon Hausmann
2009-09-29 06:00:19 PDT
testOptionalJSObjects() is also failing. Jedrzej is looking into it :) 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? I suggest you make that comment to the other bug. 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.
(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. > 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 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 :)
Created attachment 40864 [details]
xfail
xfail patch
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 Committed r49294: <http://trac.webkit.org/changeset/49294> |