Bug 142447

Summary: Add some WorkQueue tests to TestWebKitAPI
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: Tools / TestsAssignee: Brent Fulgham <bfulgham>
Severity: Normal CC: andersca, bfulgham, ddkilzer, gyuyoung.kim, koivisto, mrobinson, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 142432    
Bug Blocks: 142471, 142473    
Description Flags
Patch v2 (Activate on Windows)
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]
Comment 2 Brent Fulgham 2015-03-07 23:17:35 PST
Comment on attachment 248181 [details]

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

Easiest to see where it started failing here:
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!