Closed Bug 1673934 Opened 1 year ago Closed 1 year ago

Remove defer usage in client/debugger/src/actions/tests/preview.spec.js

Categories

(DevTools :: Debugger, task, P3)

task

Tracking

(firefox84 fixed)

RESOLVED FIXED
84 Branch
Tracking Status
firefox84 --- fixed

People

(Reporter: hexagonrecursion, Assigned: hexagonrecursion)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

We should use new Promise instead.

I'm claiming the bug now to warn others that I started working on the file. I'll add a patch when I'm done writing it.

Blocks: 1283869
Assignee: nobody → hexagonrecursion

I am sorry, but I could not find a better solution. I searched on the internet. I searched on stackoverflow. I looked at several other patches that remove defer. The only one that was helpful in this case was https://bugzilla.mozilla.org/show_bug.cgi?id=1288947.

Here is why I do not see a better solution than to save a reference to resolve in a local variable:

  • The code that uses defer is two tests for setPreview()
  • setPreview() is an asynchronous redux action that first does some async stuff and then dispatches SET_PREVIEW
  • Both tests try to reproduce a race condition: if a second preview begins dispatching while one is already in progress, the first preview should not dispatch SET_PREVIEW
  • To reproduce the race condition the tests need a way to reliably pause setPreview(). The pausing is accomplished by injecting a fake client.loadObjectProperties() that waits until the test resolves a promise

While manually testing the patch (intentionally injecting a fault into setPreview() and confirming that both tests detect it) I noticed that the following assertion in the second test does not fail:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/src/actions/tests/preview.spec.js#232

await waitATick(() => expect(fail).toEqual(false));

The test still fails because of another assertion
https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/src/actions/tests/preview.spec.js#236

expect(preview && preview.expression).toEqual("secondSetPreview");

I do not understand why the first assertion does not fail.

I think I may have found a much simpler solution, but I need to check https://tc39.es/ecma262/#sec-promise-objects to see if I'm right.

Never mind. I was wrong.

Attachment #9184738 - Attachment description: Bug 1673934 Remove defer usage in client/debugger/src/actions/tests/preview.spec.js r=honza! → Bug 1673934 - [devtools] Remove defer usage in client/debugger/src/actions/tests/preview.spec.js r=honza!
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b39d14d9fe26
[devtools] Remove defer usage in client/debugger/src/actions/tests/preview.spec.js r=Honza

Backed out for debugger failures

backout: https://hg.mozilla.org/integration/autoland/rev/62d253dbdd941117843b421f1a22a589e0353fa5

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&searchStr=linux%2C18.04%2Cx64%2Copt%2Cnode%2Ctests%2Csource-test-node-debugger-tests%2Cdebugger&revision=b39d14d9fe26af1099f58fa3cfa7bf9c4b05476f&selectedTaskRun=RxSrRjYySpW64TCFFfupMQ.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=320697092&repo=autoland&lineNumber=382

[task 2020-11-04T10:56:50.874Z] Error -------------------------------------------------------------------------- src/actions/tests/preview.spec.js:199:5
[task 2020-11-04T10:56:50.874Z]
[task 2020-11-04T10:56:50.874Z] Cannot call resolveFirst because uninitialized variable [1] is not a function.
[task 2020-11-04T10:56:50.874Z]
[task 2020-11-04T10:56:50.874Z] src/actions/tests/preview.spec.js:199:5
[task 2020-11-04T10:56:50.874Z] 199| resolveFirst();
[task 2020-11-04T10:56:50.874Z] ^^^^^^^^^^^^^^
[task 2020-11-04T10:56:50.874Z]
[task 2020-11-04T10:56:50.874Z] References:
[task 2020-11-04T10:56:50.874Z] src/actions/tests/preview.spec.js:164:9
[task 2020-11-04T10:56:50.874Z] 164| let resolveFirst, resolveSecond;
[task 2020-11-04T10:56:50.874Z] ^^^^^^^^^^^^ [1]
[task 2020-11-04T10:56:50.875Z]
[task 2020-11-04T10:56:50.875Z]
[task 2020-11-04T10:56:50.875Z]
[task 2020-11-04T10:56:50.875Z] Found 4 errors
[task 2020-11-04T10:56:50.875Z]
[task 2020-11-04T10:57:06.251Z] TEST-UNEXPECTED-FAIL flow | Cannot call resolveFirst because uninitialized variable [1] is not a function.
[task 2020-11-04T10:57:06.251Z] TEST-UNEXPECTED-FAIL flow | Cannot call resolveSecond because uninitialized variable [1] is not a function.
[task 2020-11-04T10:57:06.251Z] TEST-UNEXPECTED-FAIL flow | Cannot call resolveSecond because uninitialized variable [1] is not a function.
[task 2020-11-04T10:57:06.251Z] TEST-UNEXPECTED-FAIL flow | Cannot call resolveFirst because uninitialized variable [1] is not a function.

Flags: needinfo?(hexagonrecursion)

The logs are a bit confusing, but I figured out how to reproduce this locally.

cd ~/mozilla-central/devtools/client/debugger/
yarn flow

Here is the situation:

  • the variable is guarantied to be initialized before the call because DOM Promise calls the executor synchronously
  • flow is not smart enough to understand that
    I have found a way to stop flow from complaining, but I wonder if there is a better way.
let resolveFirst = () => {}, resolveSecond = () => {};
  1. This is a false positive generated by the flow static type checker.
  2. I have failed to come up with a productive way to avoid the error so I just silenced it (see the above patch).
Flags: needinfo?(hexagonrecursion)

Another option is this:

let resolveFirst = false;
...
(async () => {
        while (!resolveFirst) await waitATick(() => {});
      })()
...
resolveFirst = true;

this makes flow happy without $FlowIgnore

Severity: -- → S3
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
Attachment #9185710 - Attachment description: Bug 1673934 Remove defer usage in client/debugger/src/actions/tests/preview.spec.js r=Honza,nataliaCs → Bug 1673934 - [devtools] Remove defer usage in client/debugger/src/actions/tests/preview.spec.js r=Honza,nataliaCs

hi can i work on this issue ?

(In reply to mayankkoshta2000 from comment #13)

hi can i work on this issue ?

Sorry, this bug is already assigned.
(see the status at the top of this page)

Honza

(In reply to mayankkoshta2000 from comment #13)

hi can i work on this issue ?

There are plenty more files that need to be converted. See this comment for instructions on how to find them https://bugzilla.mozilla.org/show_bug.cgi?id=1283869#c31

Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac2626bc6ae4
[devtools] Remove defer usage in client/debugger/src/actions/tests/preview.spec.js r=Honza
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch
Attachment #9184738 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.