Last Comment Bug 380595 - Need unittest for nsIIdleService
: Need unittest for nsIIdleService
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: :Gijs Kruitbosch
:
:
Mentors:
Depends on: 343416 380682 392788
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-13 18:54 PDT by :Gijs Kruitbosch
Modified: 2012-04-29 01:15 PDT (History)
8 users (show)
pavlov: blocking1.9-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Unit test testing 12 thingummywhatsits when given the opportunity. (8.44 KB, patch)
2007-05-13 18:59 PDT, :Gijs Kruitbosch
neil: superreview+
Details | Diff | Splinter Review
Patch with unittest (8.80 KB, patch)
2007-08-26 07:48 PDT, :Gijs Kruitbosch
roc: review+
neil: superreview+
Details | Diff | Splinter Review

Description :Gijs Kruitbosch 2007-05-13 18:54:57 PDT
As in summary. Patch will follow shortly.
Comment 1 :Gijs Kruitbosch 2007-05-13 18:59:05 PDT
Created attachment 264699 [details] [diff] [review]
Unit test testing 12 thingummywhatsits when given the opportunity.

I'm too tired to comment right now. I hope the tests say it all.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-05-14 02:13:35 PDT
This test is going to take one minute to run, every time? That sounds bad.
Comment 3 :Gijs Kruitbosch 2007-05-14 02:18:48 PDT
Yeah. I will take suggestions :-)

I could make the delay for the timeout be 20 seconds or something, but the time it will take until the listener gets called will always vary between 0 and 1 minute. Would that be more acceptable? I can't really think of a better plan if we actually want to test this, except perhaps do away with the observer tests - I don't think I really like that idea a lot. One could WONTFIX, but that will make sayrer unhappy I think. And me too, now that I've put the energy into writing this.
Comment 4 :Gijs Kruitbosch 2007-05-14 02:22:21 PDT
self-nit:

+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=8675309">Mozilla Bug 343416</a>

Forgot to edit that :\
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-05-14 02:40:27 PDT
Can we control the idle timer's rate, and set it to 5 seconds or something during this test? Perhaps via a preference?
Comment 6 :Gijs Kruitbosch 2007-05-14 02:43:30 PDT
"No."

It checks the idle time every 5 seconds, but the granularity of the observer time setting is fixed on 1 minute. :-(
Comment 7 neil@parkwaycc.co.uk 2007-05-14 08:56:12 PDT
Comment on attachment 264699 [details] [diff] [review]
Unit test testing 12 thingummywhatsits when given the opportunity.

I have no objections, except for the extraneous string conversions.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-05-14 15:52:07 PDT
How about changing the granularity of the observer time setting to seconds?
Comment 9 :Gijs Kruitbosch 2007-05-14 17:06:42 PDT
Well... I don't really see the purpose of doing so, as I can't imagine a usecase (other than this test) for having the user idle less than a minute being interesting - but sure, you're the module owner :-)
Comment 10 :Gijs Kruitbosch 2007-08-26 07:48:52 PDT
Created attachment 278304 [details] [diff] [review]
Patch with unittest

This is mostly the same test, but:

- setTimeout takes only 5 seconds
- listener waits for 6 seconds [note that the granularity of nsIIdleService is 5 seconds, so it might take ~11 seconds for the notification to arrive at the JS observer code]
- the test is guaranteed to end after ~12 seconds.


I've removed the extraneous String conversions.


Note that because the granularity of the service is 5 seconds, and the time I ask about is 6 seconds, it is probably not a very good measure of how accurate anything is, but I guess the test would still fail if there was a real mistake made in any of the implementations.
Comment 11 neil@parkwaycc.co.uk 2007-08-26 08:35:51 PDT
Comment on attachment 278304 [details] [diff] [review]
Patch with unittest

>+            alert(data + "  " + newIdleSeconds);
Did you mean dump()?
Comment 12 :Gijs Kruitbosch 2007-08-26 09:04:45 PDT
(In reply to comment #11)
> (From update of attachment 278304 [details] [diff] [review])
> >+            alert(data + "  " + newIdleSeconds);
> Did you mean dump()?
> 

*blush*
No, I forgot to take that out. (including the if statement that precedes it).
Comment 13 :Gijs Kruitbosch 2007-08-28 07:51:35 PDT
I checked this in, and the mac box is very happy, and the linux box is not so happy. It's failing to get the observer notification in a reasonable time, apparently. I find this very odd, given:

1) getting the correct idle time works (because the test checking idleTime after 5 seconds using a setTimeout succeeds, even on the linux box)
2) observer notification works on mac, and is implemented in xpwidgets, meaning the code is identical on linux.
3) the test hasn't timed out entirely yet. The time interval in which the test can fail without triggering the timeout should be 1 second. ("should be" rather than "is" because setTimeout has always been a bit tricky).

Furthermore there have recently been other problems with this tinderbox (bug 392788), which were suspected to have been caused by slow VM performance (see comments 4 and 5 on that bug). So all in all I'm not very much inclined to back the test out, but on the other hand I'm not sure what else we could do, either. Suggestions are welcome.
Comment 14 :Gijs Kruitbosch 2007-08-28 08:28:24 PDT
After talking to robcee from QA/tinderbox land, I've backed out the test. We'll have to wait until the box is a bit more sane. Marking dep bug.
Comment 15 :Gijs Kruitbosch 2007-09-05 04:23:01 PDT
Asking for blocking? as I don't want to end up with a Fx 3 release which has a new interface without tests being used on it.
Comment 16 Stuart Parmenter 2007-09-24 16:44:31 PDT
not blocking but a=me to checkin during code orange
Comment 17 :Gijs Kruitbosch 2007-09-25 15:04:13 PDT
(In reply to comment #16)
> not blocking but a=me to checkin during code orange
> 

The issue is not that the box is orange all the time, but that it turns orange due to this test because the box is sucky when it comes to being fast and/or doing timeouts accurately.
Comment 18 Mike Connor [:mconnor] 2007-10-17 12:24:14 PDT
Comment on attachment 278304 [details] [diff] [review]
Patch with unittest

clearing approval for now, we need to fix the box issue before this can be safely landed...
Comment 19 :Gijs Kruitbosch 2007-11-01 13:14:54 PDT
So centos5 is better these days, but holding off on landing this until after the freeze so that b1 can ship without being marred by possible traffic-light behaviour from the tinderboxen.
Comment 20 :Gijs Kruitbosch 2007-11-15 12:52:18 PST
Checking in mozilla/widget/Makefile.in;
/cvsroot/mozilla/widget/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Checking in mozilla/widget/tests/Makefile.in;
/cvsroot/mozilla/widget/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.11; previous revision: 1.10
done
Checking in mozilla/widget/tests/test_bug343416.xul;
/cvsroot/mozilla/widget/tests/test_bug343416.xul,v  <--  test_bug343416.xul
new revision: 1.3; previous revision: 1.2
done


And now we wait...
Comment 21 :Gijs Kruitbosch 2007-11-15 13:53:40 PST
Centos is green. Marking FIXED.
Comment 22 :Gijs Kruitbosch 2007-12-02 14:38:44 PST
So centos just turned orange randomly, as far as I know that's the first time since this was checked in, but in any case, if it happens again I'm thinking maybe we're better off wontfixing this or removing the (most useful) test? :-\
Comment 23 :Gijs Kruitbosch 2007-12-04 14:27:12 PST
centos just failed again. We shouldn't have random box-induced failures in a freeze period, so I've commented out the one offending test. I'm not sure what should happen to it though. Do we reopen this bug and try to figure out why the box is making an idiot out of itself, or just leave everything as it is and stop worrying?
Comment 24 Jeff Walden [:Waldo] (remove +bmo to email) 2007-12-09 14:38:34 PST
I think we reopen and try to figure out what's wrong.
Comment 25 :Gijs Kruitbosch 2008-04-05 02:10:12 PDT
(In reply to bug 343416 comment #36)
> When running the following mochitest which tests this feature I get one or more
> failed passes out of 10.
> 
> http://mxr.mozilla.org/seamonkey/source/widget/tests/test_bug343416.xul
> 
> "not ok - The idle time should have increased by roughly the amount of time it
> took for the timeout to fire"
> 
> "not ok - We added a listener and it should have been called by now"
> 
> How should I proceed? Filing a new bug which depends on that one?
> 

What OS is this on? And are you sure you're not providing keyboard/mouse input at the time? Idle time is the time since the last user interaction - if you interact with the machine in any way during this test, it will reset that timer and break the test... Also, it could be that setTimeout calls fail to fire accurately, particularly if your machine is under heavy CPU load or something. All of which does not really pertain to bug 343416, it just shows that the tests are very fragile. 

I don't really see any way to make them less fragile, as by its very nature this service is about determining user idle time, which is always going to be "iffy". See also eg. bug 413035 (which afaict is a bug in x/gnome-screensaver or Ubuntu). If you really are seeing an issue with the idle service, I'm rather biased towards blaming upstream, as basically we're just wrapping an OS API, and if that breaks, we break. 

So I've replied on the bug about the unittest... if you find that there is indeed an issue with the implementation on some OS, you should probably file a new bug depending on the bug which added the implementation - there are separate bugs for Mac OS X and Win32, and Linux is on the original bug 343416.
Comment 26 Henrik Skupin (:whimboo) 2008-04-05 02:43:29 PDT
It happened with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008040509 Minefield/3.0pre

Now after reading your statement how this test has to be run it is working and doesn't fail anymore. It would be great to add some information about not doing any keyboard/mouse input while this test is running. Can this be covered by this bug?
Comment 27 :Gijs Kruitbosch 2009-11-02 09:02:37 PST
(In reply to comment #26)
> It happened with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US;
> rv:1.9pre) Gecko/2008040509 Minefield/3.0pre
> 
> Now after reading your statement how this test has to be run it is working and
> doesn't fail anymore. It would be great to add some information about not doing
> any keyboard/mouse input while this test is running. Can this be covered by
> this bug?

Reviving this, bit late I know... where would you want this info?
Comment 28 :Gijs Kruitbosch 2012-04-29 01:15:40 PDT
Resolving this again as this test is no longer randomly oranging, as far as I can tell from searching bugzilla.

Note You need to log in before you can comment on or make changes to this bug.