Bug 153919 – tests fail if display sleeps while run-webkit-tests is running

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Darin Adler]

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.

[Darin Adler]

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]

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]

We can easily prevent display sleep from LayoutTestHelper as well. Is preventing display sleep during layout tests an acceptable behavior for you?

[Darin Adler]

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]

Yes, agreed.

I think that using IOPMAssertionCreateWithDescription would probably be more robust that using caffeinate.

[Darin Adler]

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]

I'll work on it.

[Alexey Proskuryakov]

> 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]

Created attachment 270785 [details]
Proposed patch

[WebKit Commit Bot]

Comment on attachment 270785 [details]
Proposed patch

Clearing flags on attachment: 270785

Committed r196212: <http://trac.webkit.org/changeset/196212>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Thanks. I’ll give this a try and see if it solves my problem at home.

[Darin Adler]

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]

I’m going to reopen this bug because the problem is not fixed.

[Aakash Jain]

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]

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]

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]

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]

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]

Created attachment 280028 [details]
Proposed patch

Please give it a try.

[Darin Adler]

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 on attachment 280028 [details]
Proposed patch

This patch didn’t work. The tests all still timed out.

[Darin Adler]

Sorry, I meant to say "many tests timed out". Not all. New info coming soon.

[Darin Adler]

There may have been something wrong with my experiment. I will try again.

[Darin Adler]

Comment on attachment 280028 [details]
Proposed patch

My mistake. This seems to be working fine.

[WebKit Commit Bot]

Comment on attachment 280028 [details]
Proposed patch

Clearing flags on attachment: 280028

Committed r201489: <http://trac.webkit.org/changeset/201489>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Sorry for the mixed message. Many tests are still timing out. This is still not fixed.

[Aakash Jain]

Is the display getting turned off or not? Is the behavior different than when executed with caffeinate?

[Darin Adler]

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]

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]

OK, I just tried this experiment:

    % caffeinate -dis run-webkit-tests

When I did that, all the tests passed.

[Darin Adler]

This also worked:

    % caffeinate -di run-webkit-tests

Next time I will try just "caffeinate -d".

[Aakash Jain]

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]

I’ll keep trying different combinations.

[Aakash Jain]

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 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]

Created attachment 280817 [details]
Updated patch

Incorporated above review comments.

[WebKit Commit Bot]

Comment on attachment 280817 [details]
Updated patch

Clearing flags on attachment: 280817

Committed r201821: <http://trac.webkit.org/changeset/201821>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.