Need unittest for nsIIdleService

RESOLVED FIXED

Status

()

Core
Widget
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
As in summary. Patch will follow shortly.
(Assignee)

Comment 1

10 years ago
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.
Assignee: general → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #264699 - Flags: superreview?(neil)
Attachment #264699 - Flags: review?(roc)
This test is going to take one minute to run, every time? That sounds bad.
(Assignee)

Comment 3

10 years ago
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.
(Assignee)

Comment 4

10 years ago
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?
(Assignee)

Comment 6

10 years ago
"No."

It checks the idle time every 5 seconds, but the granularity of the observer time setting is fixed on 1 minute. :-(

Comment 7

10 years ago
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.
Attachment #264699 - Flags: superreview?(neil) → superreview+
How about changing the granularity of the observer time setting to seconds?
(Assignee)

Comment 9

10 years ago
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 :-)
(Assignee)

Updated

10 years ago
Depends on: 380682
(Assignee)

Updated

10 years ago
Attachment #264699 - Attachment is obsolete: true
Attachment #264699 - Flags: review?(roc)
(Assignee)

Comment 10

10 years ago
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.
Attachment #278304 - Flags: superreview?(neil)
Attachment #278304 - Flags: review?(roc)

Comment 11

10 years ago
Comment on attachment 278304 [details] [diff] [review]
Patch with unittest

>+            alert(data + "  " + newIdleSeconds);
Did you mean dump()?
Attachment #278304 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 12

10 years ago
(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).
Attachment #278304 - Flags: review?(roc) → review+
(Assignee)

Comment 13

10 years ago
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.
(Assignee)

Comment 14

10 years ago
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.
Depends on: 392788
(Assignee)

Comment 15

10 years ago
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.
Flags: blocking1.9?

Updated

10 years ago
Attachment #278304 - Flags: approval1.9+

Comment 16

10 years ago
not blocking but a=me to checkin during code orange
Flags: blocking1.9? → blocking1.9-
(Assignee)

Comment 17

10 years ago
(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...
Attachment #278304 - Flags: approval1.9+
(Assignee)

Comment 19

10 years ago
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.
(Assignee)

Comment 20

10 years ago
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...
(Assignee)

Comment 21

10 years ago
Centos is green. Marking FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 22

10 years ago
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? :-\
(Assignee)

Comment 23

10 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

9 years ago
(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?
QA Contact: general
(Assignee)

Comment 27

8 years ago
(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?
(Assignee)

Comment 28

5 years ago
Resolving this again as this test is no longer randomly oranging, as far as I can tell from searching bugzilla.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.