Closed Bug 1021774 Opened 5 years ago Closed 5 years ago

Add test for background hang monitor / thread hang stats

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
WIP. For some reason the test fails and only fails on OS X 10.6.
Depends on: 1023307
This patch fixes the failure on OS X 10.6.
Attachment #8435899 - Attachment is obsolete: true
Attachment #8439271 - Flags: review?(vdjeric)
Comment on attachment 8439271 [details] [diff] [review]
Add test for background hang monitor / thread hang stats (v2)

Review of attachment 8439271 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_ThreadHangStats.js
@@ +14,5 @@
> +
> +  // We disable hang reporting in several situations (e.g. debug builds,
> +  // official releases). In those cases, we don't have hang stats available
> +  // and should exit the test early.
> +  if (!startHangs) {

This might hide some types of bugs that break hang reporting. Is that ok?

@@ +38,5 @@
> +      let endHangs = getMainThreadHangStats();
> +
> +      // Because hangs are recorded asynchronously, if we don't see new hangs,
> +      // we should wait for pending hangs to be recorded. On the other hand,
> +      // if hang monitoring is broken, this test will time out.

Hmm, is this an acceptable thing to do on our test infrastructure? How long is the timeout?
Attachment #8439271 - Flags: review?(vdjeric)
Comment on attachment 8439271 [details] [diff] [review]
Add test for background hang monitor / thread hang stats (v2)

Review of attachment 8439271 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Vladan Djeric (:vladan) from comment #3)
> Comment on attachment 8439271 [details] [diff] [review]
> Add test for background hang monitor / thread hang stats (v2)
> 
> Review of attachment 8439271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/tests/unit/test_ThreadHangStats.js
> @@ +14,5 @@
> > +
> > +  // We disable hang reporting in several situations (e.g. debug builds,
> > +  // official releases). In those cases, we don't have hang stats available
> > +  // and should exit the test early.
> > +  if (!startHangs) {
> 
> This might hide some types of bugs that break hang reporting. Is that ok?

I think so. Hang reporting is enabled at compile time. We don't expect hang reporting to work in builds that disable it. And for builds that enable it, we do test that it is working.

> @@ +38,5 @@
> > +      let endHangs = getMainThreadHangStats();
> > +
> > +      // Because hangs are recorded asynchronously, if we don't see new hangs,
> > +      // we should wait for pending hangs to be recorded. On the other hand,
> > +      // if hang monitoring is broken, this test will time out.
> 
> Hmm, is this an acceptable thing to do on our test infrastructure? How long
> is the timeout?

The timeout is 5 minutes (as defined by the test harness). If everything is working, we'd never time out. And like any other test, if it does time out, something is broken and we have to fix it.
Attachment #8439271 - Flags: review?(vdjeric)
Comment on attachment 8439271 [details] [diff] [review]
Add test for background hang monitor / thread hang stats (v2)

(In reply to Jim Chen [:jchen :nchen] from comment #4)
> > ::: toolkit/components/telemetry/tests/unit/test_ThreadHangStats.js
> > @@ +14,5 @@
> > > +
> > > +  // We disable hang reporting in several situations (e.g. debug builds,
> > > +  // official releases). In those cases, we don't have hang stats available
> > > +  // and should exit the test early.
> > > +  if (!startHangs) {
> > 
> > This might hide some types of bugs that break hang reporting. Is that ok?
> 
> I think so. Hang reporting is enabled at compile time. We don't expect hang
> reporting to work in builds that disable it. And for builds that enable it,
> we do test that it is working.

No, I meant that we would not be able to detect a bug that breaks hang reporting in such a way that Services.telemetry.threadHangStats does not return an array with an entry for the Gecko thread (on a build with hang reporting).

But I guess that's pretty unlikely.
Attachment #8439271 - Flags: review?(vdjeric) → review+
https://hg.mozilla.org/mozilla-central/rev/bb4422ab5f08
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.