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

Status: RESOLVED FIXED

(DevTools :: Debugger, task, P3)

Description

[Andrey Bienkowski]

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.

Comment 1

[Andrey Bienkowski]

Attachment: Bug 1673934 - [devtools] Remove defer usage in client/debugger/src/actions/tests/preview.spec.js r=honza!

Comment 2

[Andrey Bienkowski]

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

Comment 3

[Andrey Bienkowski]

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.

Comment 4

[Andrey Bienkowski]

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.

Comment 5

[Andrey Bienkowski]

Never mind. I was wrong.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b39d14d9fe26
[devtools] Remove defer usage in client/debugger/src/actions/tests/preview.spec.js r=Honza

Comment 7

[Natalia Csoregi [:nataliaCs]]

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.

Comment 8

[Andrey Bienkowski]

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

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

Comment 9

[Andrey Bienkowski]

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 = () => {};

Comment 10

[Andrey Bienkowski]

Attachment: Bug 1673934 - [devtools] Remove defer usage in client/debugger/src/actions/tests/preview.spec.js r=Honza,nataliaCs

Comment 11

[Andrey Bienkowski]

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).

Comment 12

[Andrey Bienkowski]

Another option is this:

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

this makes flow happy without $FlowIgnore

Comment 13

[mayankkoshta2000]

hi can i work on this issue ?

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 15

[Andrey Bienkowski]

(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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac2626bc6ae4
[devtools] Remove defer usage in client/debugger/src/actions/tests/preview.spec.js r=Honza

Comment 17

[Narcis Beleuzu [:NarcisB]]

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