RESOLVED FIXED 153919
tests fail if display sleeps while run-webkit-tests is running
https://bugs.webkit.org/show_bug.cgi?id=153919
Summary tests fail if display sleeps while run-webkit-tests is running
Darin Adler
Reported 2016-02-05 09:25:57 PST
For a while I’ve been suffering a problem where I see tests fail if the display sleeps while run-webkit-tests runs. While I don’t understand the underlying cause, I see a really simple way to work around this by invoking the OS X command line tool named caffeinate.
Attachments
Proposed patch (2.85 KB, patch)
2016-02-05 19:02 PST, Aakash Jain
no flags
Proposed patch (1.40 KB, patch)
2016-05-28 00:38 PDT, Aakash Jain
no flags
Updated patch (3.28 KB, patch)
2016-06-06 19:50 PDT, Aakash Jain
ap: review+
ap: commit-queue-
Updated patch (3.19 KB, patch)
2016-06-08 11:37 PDT, Aakash Jain
no flags
Darin Adler
Comment 1 2016-02-05 09:26:56 PST
This is a practical problem for me because my home iMac is in a public place where I don’t want the distracting light when I’m not using it, so display sleep is set super-fast, 1 minute I think.
Darin Adler
Comment 2 2016-02-05 09:27:51 PST
Ideally we’d fix the tests so they don’t fail when the display is asleep but I don’t have the slightest idea why they fail so it’s much more practical to do the workaround. Testing it now to be sure it actually works.
Alexey Proskuryakov
Comment 3 2016-02-05 09:29:15 PST
We can easily prevent display sleep from LayoutTestHelper as well. Is preventing display sleep during layout tests an acceptable behavior for you?
Darin Adler
Comment 4 2016-02-05 09:37:16 PST
Yes, it would be better then the failing tests. Most of the time my computer is not running tests. And then perhaps some day we could figure out how to run tests while the display is asleep.
Alexey Proskuryakov
Comment 5 2016-02-05 09:40:59 PST
Yes, agreed. I think that using IOPMAssertionCreateWithDescription would probably be more robust that using caffeinate.
Darin Adler
Comment 6 2016-02-05 09:44:58 PST
I’m not sure what you mean by "robust" but I really don’t prefer one over the other. Just want to make sure that wherever we put the code we explain why we are doing it. Despite the fact that this is assigned to me, if someone else wants to tackle it, please feel free to grab the bug. I have a patch to address this, but currently it’s wrong and I don’t know when I will have time to make it right.
Aakash Jain
Comment 7 2016-02-05 10:31:59 PST
I'll work on it.
Alexey Proskuryakov
Comment 8 2016-02-05 17:00:09 PST
> I’m not sure what you mean by "robust" If we were to spawn the caffeinate process, we would at least need to add it to Tools/BuildSlaveSupport/kill-old-processes, and might need special handling elsewhere. The reason is that when buildbot kills run-webkit-tests, that doesn't happen very cleanly. I'm not sure if that's an issue with buildbot, with our scripts, or even with process group leader support in Darwin. By adding this functionality to LayoutTestHelper, we avoid creating new special cases.
Aakash Jain
Comment 9 2016-02-05 19:02:47 PST
Created attachment 270785 [details] Proposed patch
WebKit Commit Bot
Comment 10 2016-02-05 23:01:00 PST
Comment on attachment 270785 [details] Proposed patch Clearing flags on attachment: 270785 Committed r196212: <http://trac.webkit.org/changeset/196212>
WebKit Commit Bot
Comment 11 2016-02-05 23:01:02 PST
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 12 2016-02-06 08:23:32 PST
Thanks. I’ll give this a try and see if it solves my problem at home.
Darin Adler
Comment 13 2016-05-27 08:30:00 PDT
I have been having the same old problem even with this code checked in. But when I instead do "caffeinate run-webkit-tests" from the command line, it solves the problem just as it always did. Maybe LayoutTestHelper is used only for WebKit 1 testing?
Darin Adler
Comment 14 2016-05-27 08:30:35 PDT
I’m going to reopen this bug because the problem is not fixed.
Aakash Jain
Comment 15 2016-05-27 15:02:52 PDT
I am not able to reproduce it. I tried running run-webkit-tests both for wk1 and wk2 (on mac). In another terminal window I did "pmset -g assertions .” to check the flags and in both cases I found that LayoutTestHelper is correctly setting the flags. [buildbot]$ pmset -g assertions . | grep -i webkit pid 90229(LayoutTestHelper): [0x000a751700058f5b] 00:00:01 PreventUserIdleDisplaySleep named: "WebKit LayoutTestHelper" Details: WebKit layout-test helper tool is preventing sleep.
Darin Adler
Comment 16 2016-05-27 15:33:35 PDT
To test this, did you set a Mac’s Energy Saver setting to turn the display off after 1 minute? We can see from our first unsuccessful attempt that it’s hard to know if we fixed this by just looking at the flag; we’ll need to test our solution by actually reproducing the problem I was seeing. In my testing run-webkit-tests with caffeinate works, and without caffeinate hundreds of tests fail. I am not disputing that the tool sets that flag on your computer. Maybe the flag isn’t set successfully on my computer (I will check tonight)? Maybe we need one of these other flags too? PreventSystemSleep PreventUserIdleSystemSleep
Aakash Jain
Comment 17 2016-05-27 15:48:27 PDT
ok. I was trying on a lab's bot machine. I will try it on my local machine by configuring the Mac’s Energy Saver setting and checking tests results.
Aakash Jain
Comment 18 2016-05-28 00:04:51 PDT
I am trying to reproduce this on my laptop, but not able to reproduce it yet. In both wk1 and wk2, I notice that "PreventUserIdleDisplaySleep" flag is being set properly by "WebKit LayoutTestHelper". Also my laptop's display doesn't turn off while tests are running. If I stop the tests, my laptop display turns off after a minute or two. I will debug it further. Meanwhile it would be great if you can check if the flag is being set on your computer or not.
Aakash Jain
Comment 19 2016-05-28 00:28:26 PDT
I checked caffeinate, it sets another flag "PreventUserIdleSystemSleep" (same as the one you mentioned above), while we were setting "PreventUserIdleDisplaySleep" flag. Since caffeinate works for you, setting this flag should also work. I'll upload a patch soon for you to try.
Aakash Jain
Comment 20 2016-05-28 00:38:01 PDT
Created attachment 280028 [details] Proposed patch Please give it a try.
Darin Adler
Comment 21 2016-05-28 09:56:30 PDT
Comment on attachment 280028 [details] Proposed patch I think we want to prevent both, right? Well I will try this as you posted it.
Darin Adler
Comment 22 2016-05-28 10:52:49 PDT
Comment on attachment 280028 [details] Proposed patch This patch didn’t work. The tests all still timed out.
Darin Adler
Comment 23 2016-05-28 10:54:54 PDT
Sorry, I meant to say "many tests timed out". Not all. New info coming soon.
Darin Adler
Comment 24 2016-05-28 10:55:44 PDT
There may have been something wrong with my experiment. I will try again.
Darin Adler
Comment 25 2016-05-28 11:05:51 PDT
Comment on attachment 280028 [details] Proposed patch My mistake. This seems to be working fine.
WebKit Commit Bot
Comment 26 2016-05-28 11:26:40 PDT
Comment on attachment 280028 [details] Proposed patch Clearing flags on attachment: 280028 Committed r201489: <http://trac.webkit.org/changeset/201489>
WebKit Commit Bot
Comment 27 2016-05-28 11:26:44 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 28 2016-05-29 10:54:23 PDT
Sorry for the mixed message. Many tests are still timing out. This is still not fixed.
Aakash Jain
Comment 29 2016-05-29 12:32:02 PDT
Is the display getting turned off or not? Is the behavior different than when executed with caffeinate?
Darin Adler
Comment 30 2016-05-31 09:46:13 PDT
I think the display does get turned off. And I don’t think caffeinate solves the problem either. I don’t think we are going to get this problem solved indirectly with me repeatedly testing different changes. I think the person who wants to this needs to reproduce the problem. I’ll be happy to test once we have a fix that works for someone else who has reproduced it and sees it fixed.
Aakash Jain
Comment 31 2016-05-31 10:55:38 PDT
ok. I was assuming that caffeinate solves the problem properly, so I was just trying to replicate the behavior. I will reproduce it properly and fix the underlying cause. I am at a conference currently, but should be able to work on it soon.
Darin Adler
Comment 32 2016-06-05 21:06:28 PDT
OK, I just tried this experiment: % caffeinate -dis run-webkit-tests When I did that, all the tests passed.
Darin Adler
Comment 33 2016-06-06 08:40:59 PDT
This also worked: % caffeinate -di run-webkit-tests Next time I will try just "caffeinate -d".
Aakash Jain
Comment 34 2016-06-06 11:00:21 PDT
That indicates that we need both the flags "PreventUserIdleSystemSleep" and "PreventUserIdleDisplaySleep". "-d" flag sets PreventUserIdleDisplaySleep, "-i" flag sets PreventUserIdleSystemSleep. I think running "caffeinate -d", should be equivalent to running just "caffeinate", which you have already tried. I am trying to reproduce the issue today to confirm it. Sorry for the delay, I was out last week.
Darin Adler
Comment 35 2016-06-06 11:25:30 PDT
I’ll keep trying different combinations.
Aakash Jain
Comment 36 2016-06-06 19:50:48 PDT
Created attachment 280663 [details] Updated patch We need both the flags (to make sure that neither display nor system goes to sleep). I was able to reproduce the issue. Settings these flags fixed the issue for me. This is equivalent of "caffeinate -di". Can you please try the patch?
Alexey Proskuryakov
Comment 37 2016-06-08 10:38:31 PDT
Comment on attachment 280663 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=280663&action=review r=me, definitely worth landing since you confirmed that this fixes the problem for you. > Tools/DumpRenderTree/mac/LayoutTestHelper.m:199 > -static void releaseDisplaySleepAssertion() > +static void releaseSleepAssertion() Since there are two assertions here, the function name should be releaseSleepAssertions. > Tools/DumpRenderTree/mac/LayoutTestHelper.m:245 > -void addDisplaySleepAssertion() > +void addSleepAssertion() Ditto.
Aakash Jain
Comment 38 2016-06-08 11:37:27 PDT
Created attachment 280817 [details] Updated patch Incorporated above review comments.
WebKit Commit Bot
Comment 39 2016-06-08 12:16:24 PDT
Comment on attachment 280817 [details] Updated patch Clearing flags on attachment: 280817 Committed r201821: <http://trac.webkit.org/changeset/201821>
WebKit Commit Bot
Comment 40 2016-06-08 12:16:29 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.