$ ./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.

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

Thanks for the comments, updated!

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 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 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 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 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 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 on attachment 8967414 [details] Bug 1441792 - add waitForRequestData to netmonitor test head https://reviewboard.mozilla.org/r/236094/#review242596 Thanks for this cleanup!

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

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

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!

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

: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?

(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

Oh im seeing the exact same thing

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

Pushed by ystartsev@mozilla.com: 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

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