1031261 - Remove fixed timeout in test_profiler_actor.js

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Julian Seward [:jseward]]

toolkit/devtools/server/tests/unit/test_profiler_actor.js starts
the SPS profiler and waits a fixed 200ms before asking for a sample.
This fails miserably in a situation where native unwinding is 
required, since it can take SPS somewhere between 2 and 30 seconds
to load the Dwarf CFI for libxul and other libraries and start
producing samples.

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1031261-1.cset

Use binary exponential backoff, minimum delay 100ms, max
delay 25.6 seconds, so as to tolerate slow configurations
without wasting lots of time in fast configurations.

Comment 2

[Julian Seward [:jseward]]

Possibly related to bug 873855.

Comment 3

[Jim Blandy :jimb]

Comment on attachment 8447121 [details] [diff] [review]
bug1031261-1.cset

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

The duplication here is too much. Let's go around once more on this.

::: toolkit/devtools/server/tests/unit/test_profiler_actor.js
@@ +136,5 @@
> +      do_check_eq(typeof aResponse.profile.meta.platform, "string");
> +      do_check_eq(typeof aResponse.profile.threads, "object");
> +      do_check_eq(typeof aResponse.profile.threads[0], "object");
> +      do_check_eq(typeof aResponse.profile.threads[0].samples, "object");
> +      do_check_neq(aResponse.profile.threads[0].samples.length, 0);

This could be made clearer, with much less duplication of code.

First, every response to a 'getProfile' request should include a profile
property and have the right general shape. The only thing that we want to
wait for is whether we've gotten any samples yet.

Second, we can more clearly distinguish the case where we've given up from
the case where we get a bad response.

So this could be done as follows:


function attempt(aDelayMS)
{
  ... /* spin for the designated time */ ...
  aClient.request({ to: aProfiler, type: "getProfile" }, function (aResponse) {
    // These should be present in any valid getProfile response.
    do_check_eq(typeof aResponse.profile, "object");
    do_check_eq(typeof aResponse.profile.meta, "object");
    do_check_eq(typeof aResponse.profile.meta.platform, "string");
    do_check_eq(typeof aResponse.profile.threads, "object");
    do_check_eq(typeof aResponse.profile.threads[0], "object");
    do_check_eq(typeof aResponse.profile.threads[0].samples, "object");

    // If there are no samples, wait longer.
    if (aResponse.profile.threads[0].samples.length == 0) {
      if (aDelayMS < 20000) {
	do_print("attempt: no samples, going around again");
	return attempt(aDelayMS * 2);
      } else {
        do_check_true(false, "Waited 20000ms but no samples were collected");
        return;
      }
    }

    ... /* check only the aspects not checked above */ ...
}

Comment 4

[Julian Seward [:jseward]]

Attachment: bug1031261-2.cset

Restructured as suggested -- duplication removed, and marginally
improved comments.

Comment 5

[Julian Seward [:jseward]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b29081ee4dcb

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b29081ee4dcb