As in summary. Patch will follow shortly.
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.
This test is going to take one minute to run, every time? That sounds bad.
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.
self-nit: +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=8675309">Mozilla Bug 343416</a> Forgot to edit that :\
Can we control the idle timer's rate, and set it to 5 seconds or something during this test? Perhaps via a preference?
"No." It checks the idle time every 5 seconds, but the granularity of the observer time setting is fixed on 1 minute. :-(
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.
How about changing the granularity of the observer time setting to seconds?
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 :-)
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 on attachment 278304 [details] [diff] [review] Patch with unittest >+ alert(data + " " + newIdleSeconds); Did you mean dump()?
(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).
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.
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.
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.
not blocking but a=me to checkin during code orange
(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 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...
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.
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...
Centos is green. Marking FIXED.
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? :-\
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?
I think we reopen and try to figure out what's wrong.
(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.
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?
(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?
Resolving this again as this test is no longer randomly oranging, as far as I can tell from searching bugzilla.