1207702 - Fix a bug in test_promises_object_timetosettle-02.js.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

While refactoring the script actors to use protocol.js in bug 1037992, this test started failing for me. After some investigation, there seem to be a couple of things wrong with this test:

- The test sends an attach request to a promise front. The attach handler of the   
  promise actor eventually calls createSourceActors on TabSources. TabSources   
  needs this._thread to be set to the ThreadActor in order to work properly. 
  This field won't be set until we attach to a Chrome or TabActor. Nowhere in 
  the test do we actually attach to either of these. I'm not sure why the test 
  worked before, but it seems to be by pure chance: making a minor change in one 
  of the script actors causes this to fail. Explicitly attaching to either the 
  Chrome or TabActor makes the problem go away again.
- If that were the only problem, we could simply fix the test. However, the test
  checks that it always takes about 100ms for the promise to settle. I don't
  understand how we can actually guarantee this, and it seems likely to me that
  in fact were not, and again this only happens to work by chance. After fixing
  the above issue, promise settle time occasionally rises to 200-300ms, causing
  the test to fail intermittently.

Given these issues, I'd argue that the test is probably fundamentally broken, and needs to be rethought. Given that this test is blocking me on bug 1037992, I'd like to suggest that we disable it in the meantime.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Disable test_promises_object_timetosettle-02.js

See comment #1.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

What if we just assert time to settle is >= 100ms? I think that is much more sane.

Comment 3

[Eddy Bruel [:ejpbruel]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #2)
> What if we just assert time to settle is >= 100ms? I think that is much more
> sane.

If we can do that, that's fine with me too. In that case, we probably don't have to disable this test.

Comment 4

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8665009 [details] [diff] [review]
Disable test_promises_object_timetosettle-02.js

Canceling review while I come up with a fix instead.

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: Fix a bug in test_promises_object_timetosettle-02.js.

Turns out I was just being stupid and didn't understand how ChromeActor works. Here's a patch that fixes the issues explained in comment #1. I've used your suggestion to check that time to settle is >= 100ms, which is indeed much more sane.

Comment 6

[Eddy Bruel [:ejpbruel]]

Try run for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51380b13972f

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8685994 [details] [diff] [review]
Fix a bug in test_promises_object_timetosettle-02.js.

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

Thanks!

::: devtools/server/tests/unit/test_promises_object_timetosettle-02.js
@@ +24,5 @@
>  
>    let response = yield listTabs(client);
>    let targetTab = findTab(response.tabs, "test-promises-timetosettle");
>    ok(targetTab, "Found our target tab.");
> +  yield attachTab(client, targetTab);

I think you may need to attach to the thread as well, for the tab's sources to show up.

@@ +48,5 @@
>      events.on(front, "promises-settled", promises => {
>        for (let p of promises) {
>          if (p.promiseState.state === "fulfilled" &&
>              p.promiseState.value === resolution) {
> +          let timeToSettle = Math.floor(p.promiseState.timeToSettle / 100) * 100;

I think you can remove the flooring and arithmetic here now.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/dd627c83397a

Comment 9

[Carsten Book [:Tomcat]]

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