Bug 163826 – REGRESSION(r207238): [GTK] Layout test storage/domstorage/events/basic-body-attribute.html is failing

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Layout test storage/domstorage/events/basic-body-attribute.html has been failing since r207238-r207243 (inclusive):

--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/storage/domstorage/events/basic-body-attribute-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/storage/domstorage/events/basic-body-attribute-actual.txt
@@ -44,7 +44,7 @@
 PASS storageEventList[6].newValue is null
 storage.clear()
 PASS storageEventList.length is 8
-PASS storageEventList[7].key is ""
+FAIL storageEventList[7].key should be  (of type string). Was null (of type object).
 PASS storageEventList[7].oldValue is null
 PASS storageEventList[7].newValue is null
 
@@ -90,7 +90,7 @@
 PASS storageEventList[6].newValue is null
 storage.clear()
 PASS storageEventList.length is 8
-PASS storageEventList[7].key is ""
+FAIL storageEventList[7].key should be  (of type string). Was null (of type object).
 PASS storageEventList[7].oldValue is null
 PASS storageEventList[7].newValue is null

Updating expectations accordingly.

[Yusuke Suzuki]

@mcatanzaro,

Hm, this test is flaky before that patch.
It seems that this test is not related to nodeType, right?
https://bugs.webkit.org/show_bug.cgi?id=148435

I'll look into it tonight.

[Michael Catanzaro]

(In reply to comment #1)
> @mcatanzaro,
> 
> Hm, this test is flaky before that patch.

No, from quick visual inspection it looks like <10 failures in the past 30,000 commits. It's been failing consistently since r207243.

> It seems that this test is not related to nodeType, right?
> https://bugs.webkit.org/show_bug.cgi?id=148435

I really don't know, maybe your change is totally unrelated! I had trouble guessing which commit might be to blame. Yours looked most likely since I saw the words "DOM" and "type." :-)

[Michael Catanzaro]

(In reply to comment #2)
> I really don't know, maybe your change is totally unrelated! I had trouble
> guessing which commit might be to blame.

Well looks like I guessed wrong, it was surely r207238 "Update more events to stop using legacy [ConstructorTemplate=Event]". Chris, do the expectations need to be updated, or is it a bug with your change? I guess the type change might be expected, since the value can be null now?

[Chris Dumez]

r207238 made StorageEvent.key nullable, as per the spec.

This particular test just needs to be updated, this is expected.

[Michael Catanzaro]

I wonder why it's not failing on Mac.

[Chris Dumez]

(In reply to comment #5)
> I wonder why it's not failing on Mac.

LayoutTests/platform/mac/TestExpectations:webkit.org/b/148435 storage/domstorage/events/basic-body-attribute.html [ Pass Failure ]

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Created attachment 329913 [details]
Patch

[Daniel Bates]

Comment on attachment 329913 [details]
Patch

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

We are currently skipping this test on Mac (bug #148435), Windows (bug #160447), iOS (bug #155201) and WPE. All of these bugs have been open for a year or more. For your consideration I suggest that we try to unskip the test on all of these ports instead of just unskipping it on GTK.

> LayoutTests/storage/domstorage/events/basic-body-attribute.html:98
> +    shouldBeNull("storageEventList[7].key", "");

The second argument is not necessary.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Created attachment 330029 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 330029 [details]
Patch

Clearing flags on attachment: 330029

Committed r226219: <https://trac.webkit.org/changeset/226219>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

This is still flaky according to https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/11947/steps/layout-test/logs/stdio. Will reopen this ticket and adapt the test expectations.

[Diego Pino]

This test(s) has been consistenly passing in the last 4000 revisions. Closing bug.