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.
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.
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.
We can easily prevent display sleep from LayoutTestHelper as well. Is preventing display sleep during layout tests an acceptable behavior for you?
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.
Yes, agreed. I think that using IOPMAssertionCreateWithDescription would probably be more robust that using caffeinate.
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.
I'll work on it.
> 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.
Created attachment 270785 [details] Proposed patch
Comment on attachment 270785 [details] Proposed patch Clearing flags on attachment: 270785 Committed r196212: <http://trac.webkit.org/changeset/196212>
All reviewed patches have been landed. Closing bug.
Thanks. I’ll give this a try and see if it solves my problem at home.
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?
I’m going to reopen this bug because the problem is not fixed.
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.
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
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.
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.
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.
Created attachment 280028 [details] Proposed patch Please give it a try.
Comment on attachment 280028 [details] Proposed patch I think we want to prevent both, right? Well I will try this as you posted it.
Comment on attachment 280028 [details] Proposed patch This patch didn’t work. The tests all still timed out.
Sorry, I meant to say "many tests timed out". Not all. New info coming soon.
There may have been something wrong with my experiment. I will try again.
Comment on attachment 280028 [details] Proposed patch My mistake. This seems to be working fine.
Comment on attachment 280028 [details] Proposed patch Clearing flags on attachment: 280028 Committed r201489: <http://trac.webkit.org/changeset/201489>
Sorry for the mixed message. Many tests are still timing out. This is still not fixed.
Is the display getting turned off or not? Is the behavior different than when executed with caffeinate?
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.
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.
OK, I just tried this experiment: % caffeinate -dis run-webkit-tests When I did that, all the tests passed.
This also worked: % caffeinate -di run-webkit-tests Next time I will try just "caffeinate -d".
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.
I’ll keep trying different combinations.
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?
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.
Created attachment 280817 [details] Updated patch Incorporated above review comments.
Comment on attachment 280817 [details] Updated patch Clearing flags on attachment: 280817 Committed r201821: <http://trac.webkit.org/changeset/201821>