Bug 142447

Summary:

Add some WorkQueue tests to TestWebKitAPI

Product:

WebKit

Reporter:

Brent Fulgham <bfulgham>

Component:

Tools / Tests

Assignee:

Brent Fulgham <bfulgham>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, bfulgham, ddkilzer, gyuyoung.kim, koivisto, mrobinson, sam

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

142432

Bug Blocks:

142471, 142473

Attachments:

Description	Flags
Patch	none
Patch v2 (Activate on Windows)	none
Patch v3 (Update per weinig comments)	sam: review+

Description Brent Fulgham 2015-03-07 20:21:43 PST

WorkQueue is used heavily in WebKit2 code, but we don't really have good test coverage for it. We should exercise it as part of our normal TestWebKitAPI tests.

Comment 1 Brent Fulgham 2015-03-07 23:15:49 PST

Created attachment 248181 [details]
Patch

Comment 2 Brent Fulgham 2015-03-07 23:17:35 PST

Comment on attachment 248181 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/WorkQueue.cpp:110
> +    std::this_thread::sleep_for(std::chrono::nanoseconds(1000));

I'm just trying to simulate some lengthy bit of work here. Maybe there's a better way?

> Tools/TestWebKitAPI/Tests/WTF/WorkQueue.cpp:132
> +    queue1->dispatchAfter(std::chrono::milliseconds(100), finishedTests);

I want to verify that the two queues work independently. Not sure if there is a better way to do this? Maybe more condition variables to coordinate the order of things?

Comment 3 Brent Fulgham 2015-03-07 23:34:01 PST

Created attachment 248182 [details]
Patch v2 (Activate on Windows)

Comment 4 Brent Fulgham 2015-03-07 23:51:05 PST

Comment on attachment 248182 [details]
Patch v2 (Activate on Windows)

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

> Tools/TestWebKitAPI/Tests/WTF/WorkQueue.cpp:124
> +    queue1->dispatchAfter(std::chrono::milliseconds(100), finishedTests);

I'm trying to simulate two queues performing different amounts of work. Maybe there's a better way than these delays...

Comment 5 Brent Fulgham 2015-03-08 14:04:02 PDT

Created attachment 248203 [details]
Patch v3 (Update per weinig comments)

Comment 6 Sam Weinig 2015-03-08 14:31:04 PDT

Comment on attachment 248203 [details]
Patch v3 (Update per weinig comments)

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

> Tools/TestWebKitAPI/Tests/WTF/WorkQueue.cpp:191
> +    EXPECT_TRUE(done >= start + std::chrono::milliseconds(500));

I am not sure how guaranteed accurate dispatchAfter is.

Comment 7 Sam Weinig 2015-03-08 14:32:43 PDT

Comment on attachment 248203 [details]
Patch v3 (Update per weinig comments)

The tests look good, though the DispatchAfter worries me that it could be flakey (as all timing depended tests are).  I would land it without testing that the actual timing matches up.

Comment 8 Brent Fulgham 2015-03-08 16:55:35 PDT

Committed r181249: <http://trac.webkit.org/changeset/181249>

Comment 9 Brent Fulgham 2015-03-08 16:58:11 PDT

EFL and GTK ports should probably add this test as well.

Comment 10 Martin Robinson 2015-03-08 21:53:59 PDT

The corresponding change for CMake is here: https://bugs.webkit.org/show_bug.cgi?id=142473

Comment 11 David Kilzer (:ddkilzer) 2015-03-09 05:59:09 PDT

Attempt to fix WTF_WorkQueue.TwoQueues timeout test failure
<http://trac.webkit.org/changeset/181265>

Easiest to see where it started failing here:
<https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK2%20(Tests)?numbuilds=50>

Comment 12 Brent Fulgham 2015-03-09 09:41:05 PDT

(In reply to comment #11)
> Attempt to fix WTF_WorkQueue.TwoQueues timeout test failure
> <http://trac.webkit.org/changeset/181265>
> 
> Easiest to see where it started failing here:
> <https://build.webkit.org/builders/
> Apple%20Mavericks%20Debug%20WK2%20(Tests)?numbuilds=50>

Looks like that worked. Thank you for fixing that!