1441792 - browser_net_resend.js fails when converted to async/await instead of Task

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

$ ./mach mochitest devtools/client/netmonitor/test/browser_net_resend.js

fails with this exception:

TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_resend.js | A promise chain failed to handle a rejection: data.requestHeaders is undefined - stack: null

We are not using yield/await on the call to testCustomForm (we should), but it doesn't seem to be just that.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1441792 - Convert browser_net_resend to async/await.

Review commit: https://reviewboard.mozilla.org/r/223830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223830/

Comment 2

[Yulia Startsev [:yulia]]

Attachment: Bug 1441792 - fix browser_net_resend.js and convert to async/await.

Review commit: https://reviewboard.mozilla.org/r/233976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233976/

Comment 3

[Yulia Startsev [:yulia]]

Comment on attachment 8965260 [details]
Bug 1441792 - fix browser_net_resend.js and convert to async/await.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233976/diff/1-2/

Comment 4

[Yulia Startsev [:yulia]]

This issue was the lack of yield in front of testCustomForm -- since generators do not evaluate the body unless they are yielded, this piece of code was never run -- likely there was a refactoring done to make requestHeaders and requestPostData lazy, and this got missed because it wasn't causing the test to fail. 

I did not use fetchNetworkUpdatePacket https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/utils/request-utils.js#80 because it relies on the component update cycle and does not return a promise that can be awaited... I am not sure this level of detail needs a comment, but just in case anyone was wondering, this is why.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8965260 [details]
Bug 1441792 - fix browser_net_resend.js and convert to async/await.

https://reviewboard.mozilla.org/r/233976/#review240026

::: devtools/client/netmonitor/test/browser_net_resend.js:35
(Diff revision 2)
> +
> +  let origItemId = getSortedRequests(store.getState()).get(0).id;
> +
> +  // fetch lazy data so that requestHeaders and requestPostData are available
> +  await connector.requestData(origItemId, "requestHeaders");
> +  await connector.requestData(origItemId, "requestPostData");

I'll let honza have the final word here.

But shouldn't we call some UI action that will do these requestData calls instead of manually doing them from test?
Or are we missing some waiting code that would wait for these requests to complete?
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/HeadersPanel.js#89-93

Comment 6

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

Comment on attachment 8965260 [details]
Bug 1441792 - fix browser_net_resend.js and convert to async/await.

https://reviewboard.mozilla.org/r/233976/#review240096

Thanks Yulia for working on this, it's great to see that this test will be converted as well!

Please see my inline comment.

Honza

::: devtools/client/netmonitor/test/browser_net_resend.js:36
(Diff revision 2)
> +  let origItemId = getSortedRequests(store.getState()).get(0).id;
> +
> +  // fetch lazy data so that requestHeaders and requestPostData are available
> +  await connector.requestData(origItemId, "requestHeaders");
> +  await connector.requestData(origItemId, "requestPostData");
>  

Both "requestHeaders" and "requsetPostData" are automatically fetched in HeadersPanel.componentDidMount(). It happens when a request is selected and Headers panel opens by default.

So, no need to do this explicitelly here.

::: devtools/client/netmonitor/test/browser_net_resend.js:39
(Diff revision 2)
> +  await connector.requestData(origItemId, "requestHeaders");
> +  await connector.requestData(origItemId, "requestPostData");
>  
>    let origItem = getSortedRequests(store.getState()).get(0);
>  
>    store.dispatch(Actions.selectRequest(origItem.id));

This action selects the request and so the Headers panel opens.

The test should wait till both "requestHeaders" and "requestPostData" are fetched from the backend.

You can use similar `waitUntil` code that is used later in the test (a helper could be nice for such construct across tests, but not sure how to do it generically and it might be also out of scope of this bug).

Something like as follows:

await waitUntil(() => {
  let item = getRequestById(store.getState(), origItem.id);
  return item.requestHeaders && item.requestPostData;
});

And of course, after waiting you need to get the request again:

origItem = getSortedRequests(store.getState()).get(0);

or 

origItem = getRequestById(store.getState(), origItem.id);

The state is immutable so, you need to get new `origItem` that points to the fetched headers and post data.

Comment 7

[Yulia Startsev [:yulia]]

Comment on attachment 8965260 [details]
Bug 1441792 - fix browser_net_resend.js and convert to async/await.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233976/diff/2-3/

Comment 8

[Yulia Startsev [:yulia]]

Thanks for the comments, updated!

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8965260 [details]
Bug 1441792 - fix browser_net_resend.js and convert to async/await.

https://reviewboard.mozilla.org/r/233976/#review241728

Looks good to me, thanks for jumping on this Yulia!

::: devtools/client/netmonitor/test/browser_net_resend.js:90
(Diff revision 3)
> +  function waitForRequestHeaders(id) {
> +    return waitUntil(() => {
> +      const item = getRequestById(store.getState(), id);
> +      return item.requestHeaders && item.requestPostData;
> +    });
> +  }

We may be able to shared this by moving this to head.js:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_simple-request-data.js#107-110
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_resend_cors.js#49-54
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_resend.js#56-63

Comment 10

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

Comment on attachment 8965260 [details]
Bug 1441792 - fix browser_net_resend.js and convert to async/await.

https://reviewboard.mozilla.org/r/233976/#review241734

Looks great for me, thanks Yulia!

R+ 

Honza

::: devtools/client/netmonitor/test/browser_net_resend.js:90
(Diff revision 3)
> +  function waitForRequestHeaders(id) {
> +    return waitUntil(() => {
> +      const item = getRequestById(store.getState(), id);
> +      return item.requestHeaders && item.requestPostData;
> +    });
> +  }

Agree, this would be great (it's ok for me if this is a follow up).

Comment 11

[Yulia Startsev [:yulia]]

Comment on attachment 8965260 [details]
Bug 1441792 - fix browser_net_resend.js and convert to async/await.

https://reviewboard.mozilla.org/r/233976/#review241728

> We may be able to shared this by moving this to head.js:
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_simple-request-data.js#107-110
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_resend_cors.js#49-54
> https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_resend.js#56-63

made a follow up pr for this!

Comment 12

[Yulia Startsev [:yulia]]

Attachment: Bug 1441792 - add waitForRequestData to netmonitor test head

Review commit: https://reviewboard.mozilla.org/r/236094/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236094/

Comment 13

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

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

https://reviewboard.mozilla.org/r/236094/#review242120

I really like the new API, thanks for working on that Yulia!

Please see my inline comments.

Honza

::: devtools/client/netmonitor/test/browser_net_headers_sorted.js:44
(Diff revision 1)
>    let wait = waitForDOM(document, ".headers-overview");
>    EventUtils.sendMouseEvent({ type: "mousedown" },
>      document.querySelectorAll(".request-list-item")[0]);
>    await wait;
>  
> -  await waitUntil(() => {
> +  await waitForRequestData(store, ["requestHeaders", "requestPostData"]);

Shouldn't the second arg be: ["requestHeaders", "responseHeaders"]?

::: devtools/client/netmonitor/test/head.js:768
(Diff revision 1)
>      content.wrappedJSObject.performRequests(requestCount);
>    });
>    await wait;
>  }
> +
> +function waitForRequestData(store, fields) {

It would be nice to have short js doc comment for the method.

Comment 14

[Yulia Startsev [:yulia]]

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236094/diff/1-2/

Comment 15

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

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

https://reviewboard.mozilla.org/r/236094/#review242158

::: devtools/client/netmonitor/test/browser_net_headers_filter.js:27
(Diff revision 2)
>    wait = waitUntil(() => document.querySelector(".headers-overview"));
>    EventUtils.sendMouseEvent({ type: "mousedown" },
>      document.querySelectorAll(".request-list-item")[0]);
>    await wait;
>  
> -  await waitUntil(() => {
> +  await waitForRequestData(store, ["requestHeaders", "requestPostData"]);

Yet this one should be:
["requestHeaders", "responseHeaders"]

Comment 16

[Yulia Startsev [:yulia]]

Attachment: Bug 1441792 - fix browser_net_resend.js and convert to async/await.

Review commit: https://reviewboard.mozilla.org/r/236400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236400/

Comment 17

[Yulia Startsev [:yulia]]

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236094/diff/2-3/

Comment 18

[Yulia Startsev [:yulia]]

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236094/diff/3-4/

Comment 19

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

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

https://reviewboard.mozilla.org/r/236094/#review242186

Looks great to me, thanks Yulia!

R+

Honza

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

https://reviewboard.mozilla.org/r/236094/#review242596

Thanks for this cleanup!

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

https://reviewboard.mozilla.org/r/236094/#review242684

::: devtools/client/netmonitor/test/browser_net_resend.js
(Diff revision 4)
>      is(item.url, orig.url, "item is showing the same URL as original request");
>    }
>  
> -  function waitForRequestHeaders(id) {
> -    return waitUntil(() => {
> -      const item = getRequestById(store.getState(), id);

You test failure may be related to this line.
This test used to check for the state of one particular request, specified by `id` argument.
Whereas `waitForRequestData` assert the state of the first request (`get(0)`).

You may add an optional `id` parameter to waitForRequestData...

Comment 22

[Yulia Startsev [:yulia]]

Attachment: Bug 1441792 - fix browser_net_resend.js and convert to async/await.

Review commit: https://reviewboard.mozilla.org/r/241120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241120/

Comment 23

[Yulia Startsev [:yulia]]

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236094/diff/4-5/

Comment 24

[Yulia Startsev [:yulia]]

Comment on attachment 8972529 [details]
Bug 1441792 - fix browser_net_resend.js and convert to async/await.

Review commit: https://reviewboard.mozilla.org/r/241120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241120/

Comment 25

[Yulia Startsev [:yulia]]

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236094/diff/4-5/

Comment 26

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

@Yulia, I can't apply the patches (there is a conflict).
Btw. The first one isn't needed anymore?

Honza

Comment 27

[Yulia Startsev [:yulia]]

We cleared this up in chat: looks like the conflict was due to this patch being in two parts, and there was some ambiguity about which patch was the right one. Once we got the right ones, the patches applied correctly!

Tests look green!

Comment 28

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

Attachment: review-error.png

Looks good to me and the test passes locally on my machine.
But, I can't R+ this, I am seeing the following error.
(see the screenshot)
Is it only me?

Yulia, you can just R+ if it works for you...

Honza

Comment 29

[Yulia Startsev [:yulia]]

:Honza I don't see that error when I run browser_net_resend, is it this test that you are having trouble with or another one?

Comment 30

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

(In reply to Yulia Startsev [:yulia] from comment #29)
> :Honza I don't see that error when I run browser_net_resend, is it this test
> that you are having trouble with or another one?
It's the publish button in Review-board that shows me the error instead of publishing my R+

Honza

Comment 31

[Yulia Startsev [:yulia]]

Oh im seeing the exact same thing

Comment 32

[Yulia Startsev [:yulia]]

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

https://reviewboard.mozilla.org/r/236094/#review247268

remove reviews for already reviewed pr

Comment 33

[Yulia Startsev [:yulia]]

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

https://reviewboard.mozilla.org/r/236094/#review247272

remove request to review for already reviewed commit

Comment 34

[Yulia Startsev [:yulia]]

Attachment: Bug 1441792 - fix browser_net_resend.js and convert to async/await

Review commit: https://reviewboard.mozilla.org/r/241642/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241642/

Comment 35

[Yulia Startsev [:yulia]]

Comment on attachment 8967414 [details]
Bug 1441792 - add waitForRequestData to netmonitor test head

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236094/diff/5-6/

Comment 36

[Yulia Startsev [:yulia]]

Comment on attachment 8973140 [details]
Bug 1441792 - fix browser_net_resend.js and convert to async/await

https://reviewboard.mozilla.org/r/241642/#review247462

R+ing for honza, since there were issues with reviewboard

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7b95c137dba
fix browser_net_resend.js and convert to async/await r=yulia
https://hg.mozilla.org/integration/autoland/rev/1a810001d12e
add waitForRequestData to netmonitor test head r=Honza,ochameau

Comment 38

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f7b95c137dba
https://hg.mozilla.org/mozilla-central/rev/1a810001d12e