Bug 45294 – Web Inspector: add a sanity test for storage panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Yury Semikhatsky]

Web Inspector: add a sanity test for storage panel

[Yury Semikhatsky]

Created attachment 66718 [details]
Patch

[Yury Semikhatsky]

Created attachment 66720 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 66718 [details]
Patch

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

> LayoutTests/inspector/storage-panel-show.html:27
> +    function printStorageEntries(storage, entries) {
Looks like this function is not used.

[Yury Semikhatsky]

Created attachment 66721 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #3)
> (From update of attachment 66718 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66718&action=prettypatch
> 
> > LayoutTests/inspector/storage-panel-show.html:27
> > +    function printStorageEntries(storage, entries) {
> Looks like this function is not used.

Thanks. Removed the function.

[Joseph Pecoraro]

Comment on attachment 66721 [details]
Patch

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

> LayoutTests/inspector/storage-panel-show-expected.txt:1
> +Test that storage panel is present and that it contains correct datat for local and session DOM storages.

Typo: "datat" => "data"


> LayoutTests/inspector/storage-panel-show.html

This tests only the DOM Storage aspect of the Storage panel. Maybe
this file name is a bit too general, unless there are immediate
plans to test other parts of the panel (Databases, Cookies,
Application Caches).


> LayoutTests/inspector/storage-panel-show.html:12
> +        localStorage["_a" + i] = i;
> +        sessionStorage["_b" + i] = i+10;

These lines are a bit misleading, since they are actually stored as strings. I think
eventually they will be stored as Structured Clones. Should be fine, but something
to keep in mind.


> WebKit/chromium/src/js/Tests.js:-224
> -TestSuite.prototype.testHostIsPresent = function()
> -TestSuite.prototype.testElementsTreeRoot = function()
> -TestSuite.prototype.testMainResource = function()

A bit weird removing these tests, as they have nothing to do with this patch.
Probably would have been better to have its own patch.


However, there is nothing really wrong with this patch, and its a good
test =). Do you agree with any of the comments above?

[Yury Semikhatsky]

Created attachment 66849 [details]
Patch

[Yury Semikhatsky]

Thanks for reviewing this!

(In reply to comment #6)
> (From update of attachment 66721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66721&action=prettypatch
> 
> > LayoutTests/inspector/storage-panel-show-expected.txt:1
> > +Test that storage panel is present and that it contains correct datat for local and session DOM storages.
> 
> Typo: "datat" => "data"
> 
Done.

 
> > LayoutTests/inspector/storage-panel-show.html
> 
> This tests only the DOM Storage aspect of the Storage panel. Maybe
> this file name is a bit too general, unless there are immediate
> plans to test other parts of the panel (Databases, Cookies,
> Application Caches).
>
Renamed the test to storage-panel-dom-storage.html
 

> > LayoutTests/inspector/storage-panel-show.html:12
> > +        localStorage["_a" + i] = i;
> > +        sessionStorage["_b" + i] = i+10;
> 
> These lines are a bit misleading, since they are actually stored as strings. I think
> eventually they will be stored as Structured Clones. Should be fine, but something
> to keep in mind.
>
Changed the values to strings. This way ability to store  JS objects in the dom storage shouldn't affect this test.

 
> 
> > WebKit/chromium/src/js/Tests.js:-224
> > -TestSuite.prototype.testHostIsPresent = function()
> > -TestSuite.prototype.testElementsTreeRoot = function()
> > -TestSuite.prototype.testMainResource = function()
> 
> A bit weird removing these tests, as they have nothing to do with this patch.
> Probably would have been better to have its own patch.
> 
Ok, excluded this change from this patch. I was too lazy to file a new bug just too remove obsolete code.

[Joseph Pecoraro]

Comment on attachment 66849 [details]
Patch

Thanks for those making those changes.

r=me as long as those key/value pair orderings will always happen.
It would be bad if the order changed in some iterations causing
a flakey test. You could probably just check using --iterations.

[Yury Semikhatsky]

(In reply to comment #9)
> (From update of attachment 66849 [details])
> Thanks for those making those changes.
> 
> r=me as long as those key/value pair orderings will always happen.
> It would be bad if the order changed in some iterations causing
> a flakey test. You could probably just check using --iterations.

The order was steady for 100 iterations.

[WebKit Commit Bot]

Comment on attachment 66849 [details]
Patch

Rejecting patch 66849 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20939 test cases.
animations/combo-transform-translate+scale.html -> failed

Exiting early after 1 failures. 99 tests run.
20.27s total testing time

98 test cases (98%) succeeded
1 test case (1%) had incorrect layout

Full output: http://queues.webkit.org/results/3914322

[Yury Semikhatsky]

Comment on attachment 66849 [details]
Patch

Clearing flags on attachment: 66849

Committed r67067: <http://trac.webkit.org/changeset/67067>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.