1314492 - deconstructing WebRequest test deconstructivism

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect)

Description

[Shane Caraveo (:mixedpuppy)]

test_ext_webrequest.html is large, twisted and gnarly, it needs to be take out to the back 40 and taken care of properly.  From it's ashes multiple tests will grow, may they grow to be small and explicit.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP requestTests

initial reworking of webrequest tests.  still more to do, feedback appreciated at this point.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1314492 refactor webrequest tests,

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

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8807365 [details]
Bug 1314492 refactor webrequest tests,

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

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8807365 [details]
Bug 1314492 refactor webrequest tests,

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

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8807365 [details]
Bug 1314492 refactor webrequest tests,

https://reviewboard.mozilla.org/r/90530/#review90996

This looks like a big improvement, thanks for taking this on.
Clearing review for now since there are several things that appear to have been dropped from the test and I'd like to make sure we don't lose coverage.

::: toolkit/components/extensions/test/mochitest/.eslintrc.js:17
(Diff revision 3)
>      "onmessage": true,
>      "sendAsyncMessage": false,
>  
>      "waitForLoad": true,
>      "promiseConsoleOutput": true,
> +    "addStylesheet": true,

Can you group the things in head_webrequest together and add a comment indicating that that's where those symbols come from?
(it would be great if eslint could have these declarations conditional on tests that use that head file but i don't think that's practical right now)

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:36
(Diff revision 3)
> +    browser.test.assertTrue(!!expected, `expected ${filename}`);
> +    if (!expected) {
> +      return;
> +    }

I don't think you need the `assertTrue()` above since this will hit your "who let the dogs out" error below if its not present.
Also I thought eslint complained about mixing return with and without a return value in the same function, why isn't that tripping here?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:44
(Diff revision 3)
> +      expected.events = [];
> +      for (let name of Object.keys(events)) {
> +        if (name != "onErrorOccurred" && name != "onBeforeRedirect") {
> +          expected.events.push(name);
> +        }
> +      }

This could be shortened to `expected.events = Object.keys(events).filter(name => name != "onErrorOccurred" && name != "onBeforeRedirect");`

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:59
(Diff revision 3)
> +
> +  function processHeaders(phase, expected, details) {
> +    let headers = details[`${phase}Headers`];
> +    browser.test.assertTrue(Array.isArray(headers), `${phase}Headers array present`);
> +
> +    let {added, modified, deleted} = expected.headers[phase];

The original copy had some checking for a "Webrequest-processed" header here, why is it removed?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:59
(Diff revision 3)
> +
> +  function processHeaders(phase, expected, details) {
> +    let headers = details[`${phase}Headers`];
> +    browser.test.assertTrue(Array.isArray(headers), `${phase}Headers array present`);
> +
> +    let {added, modified, deleted} = expected.headers[phase];

This is all from the existing test but:
I was slightly by confused by this since `added, modified, deleted` are all past-tense but you're actually applying those actions here.  In any case, I think this would be cleaner if you pass those actions in and let the caller worry about grabbing them from the right place in `expected`.  That makes the logic here a lot simpler and more self-contained.

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:74
(Diff revision 3)
> +      headers.push(header);
> +    }
> +
> +    let modifiedAny = false;
> +    for (let header of headers.filter(h => h.name in modified)) {
> +      browser.test.log(`header ${header.name} ${header.value}`);

I imagine this is going to get noisy, if its just for debugging can you remove it before landing

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:120
(Diff revision 3)
> +      browser.test.assertFalse(headers.some(h => h.name === name), `deleted header ${name} still found in ${phase}Headers`);
> +    }
> +  }
> +
> +  let lastRequestId;
> +  function listener(name, details) {

This confused me on first reading, seems like listener should take `name` and return a callable that takes `details` as a parameter.
Having them both as parameters here but then partially applying name below feels awkward.

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:124
(Diff revision 3)
> +  let lastRequestId;
> +  function listener(name, details) {
> +    browser.test.log(`${name} ${details.requestId} ${details.url}`);
> +    let expected = getExpected(details);
> +    if (!expected) {
> +      browser.test.fail("who let the dogs out?");

I like this but can we make it more descriptive?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:132
(Diff revision 3)
> +    let expectedEvent = expected.events[0] == name;
> +    browser.test.assertTrue(expectedEvent, `recieved ${name}`);
> +    if (expectedEvent) {
> +      expected.events.shift();
> +    }
> +    browser.test.assertEq(expected.type, details.type, "resource type is correct");

Checks on originUrl from the originaly test are now gone?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:142
(Diff revision 3)
> +      browser.test.assertTrue(details.tabId !== undefined, `tabId ${details.tabId}`);
> +      expected.tabId = details.tabId;
> +      browser.test.assertTrue(details.requestId !== undefined, `requestId ${details.requestId}`);
> +      // validate requestId
> +      expected.requestId = details.requestId;
> +      if (lastRequestId !== undefined) {

It looks like nothing ever sets `lastRequestId`?
The logic here is also different from the original (the url is ignored now), why?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:151
(Diff revision 3)
> +    } else {
> +      // on subsequent request events check the previous values
> +      browser.test.assertEq(expected.requestId, details.requestId, "correct requestId");
> +      browser.test.assertEq(expected.tabId, details.tabId, "correct tabId");

This looks like it might be meant to be the else branch of the `lastRequestId` test and not `if (onBeforeRequest)`?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:171
(Diff revision 3)
> +      if (expected.headers && expected.headers.request) {
> +        checkHeaders("request", expected, details);
> +      }
> +    }
> +    if (name == "onHeadersReceived") {
> +      browser.test.assertEq(expected.status || 200, details.statusCode, `HTTP status code ${expected.status || 200} for ${details.url} (found ${details.statusCode})`);

This used to check the `.ip` property but no longer does?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:177
(Diff revision 3)
> +      if (expected.headers && expected.headers.response) {
> +        result.responseHeaders = processHeaders("response", expected, details);
> +      }
> +    }
> +    if (name == "onBeforeRedirect") {
> +      // reset id checks on redirect

This used to have logic for checking that `details.url` is getting redirected as expected?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:183
(Diff revision 3)
> +      lastRequestId = undefined;
> +    }
> +    if (name == "onCompleted") {
> +      // If we have already completed a GET request for this url,
> +      // and it was found, we expect for the response to come fromCache.
> +      let expectCached = !!(expected.cached && details.method === "GET" && details.statusCode != 404);

I don't think the !! is needed here

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:213
(Diff revision 3)
> +        listener.bind(null, name), ...args);
> +    } catch (e) {
> +      browser.test.assertTrue(/\brequestBody\b/.test(e.message),
> +                              "Request body is unsupported");
> +
> +      // requestBody is disabled in release builds

Can we just explicitly check AppConstants.RELEASE_OR_BETA here and not ask for requestBody in the first place if we don't expect it to be supported?
Actually maybe it doesn't matter since nothing appears to actually look at `details.requestBody`?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:64
(Diff revision 3)
> +  yield extension.awaitMessage("continue");
> +  addStylesheet("file_style_bad.css");
> +  yield extension.awaitMessage("cancelled");
> +  // we redirect to style_good which completes the test
> +  addStylesheet("file_style_redirect.css");
> +  yield extension.awaitMessage("done");

This test used to verify that the good stylesheet was loaded and that the bad one was not, that's now gone?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:118
(Diff revision 3)
> +  yield extension.awaitMessage("continue");
> +  addScript("file_script_bad.js");
> +  yield extension.awaitMessage("cancelled");
> +  // we redirect to script_good which completes the test
> +  addScript("file_script_redirect.js");
> +  yield extension.awaitMessage("done");

This test used to verify that the good script ran and that the bad script did not run, that's now gone.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8807365 [details]
Bug 1314492 refactor webrequest tests,

https://reviewboard.mozilla.org/r/90530/#review90996

> The original copy had some checking for a "Webrequest-processed" header here, why is it removed?

That was a control structure for the test itself that make sure processHeaders was only executed once on a url.  It's not necessary to keep, we already test headers otherwise.  However, I added a test to make sure that processheaders can only be run once per request and response.

> This is all from the existing test but:
> I was slightly by confused by this since `added, modified, deleted` are all past-tense but you're actually applying those actions here.  In any case, I think this would be cleaner if you pass those actions in and let the caller worry about grabbing them from the right place in `expected`.  That makes the logic here a lot simpler and more self-contained.

I'm making some adjustment here, but am going to push back a little as well.  I prefer to handle the extraction in one place rather than multiple places in the caller.

> This looks like it might be meant to be the else branch of the `lastRequestId` test and not `if (onBeforeRequest)`?

No, this is an area meant to run for each event, validating the id's remain the same.

> This used to have logic for checking that `details.url` is getting redirected as expected?

That's not necessary.  In the tests themselves, this is defined by the expect values.  The file being redirected has a limited list of events to expect (those which should happen).  Any additional events received will cause a test failure.  The file being redirected to is in the expected list as well, and is not otherwise initiated from the test code.  A failure in redirect will cause a failure since that expected file would not be hit.

> I don't think the !! is needed here

expected.cache may be undefined which would fail in the following assertEq.  Adjusting code to make it clearer.

> Can we just explicitly check AppConstants.RELEASE_OR_BETA here and not ask for requestBody in the first place if we don't expect it to be supported?
> Actually maybe it doesn't matter since nothing appears to actually look at `details.requestBody`?

The upload tests were moved to their own test in a different bug, prior to refactoring these tests.  Removing that from the default config, but am expecting to grow this a bit with some other tests that will be added after this bug.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Additionally, I ran into an intermittent issue with the cache test, so have moved to using promises to ensure that all the expected test requests run to the expected completion.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8807365 [details]
Bug 1314492 refactor webrequest tests,

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

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8807365 [details]
Bug 1314492 refactor webrequest tests,

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

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8807365 [details]
Bug 1314492 refactor webrequest tests,

https://reviewboard.mozilla.org/r/90530/#review92096

So much better. Thank you!

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:16
(Diff revision 5)
> +  "onErrorOccurred":     [{urls: ["<all_urls>"]}],
> +};
> +
> +function background(events) {
> +  let expect;
> +  let promises;

This should be declared inside the onMessage listener callback.

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:55
(Diff revision 5)
> +    let filename;
> +    if (url.protocol == "data:") {
> +      // pathname is everything after protocol.
> +      filename = url.pathname;
> +    } else {
> +      filename = url.pathname.substring(url.pathname.lastIndexOf("/") + 1);

`url.pathname.split("/").pop()`?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:133
(Diff revision 5)
> +    let headers = details[`${phase}Headers`];
> +    browser.test.assertTrue(Array.isArray(headers), `valid ${phase}Headers array`);
> +
> +    let {add, modify, remove} = expected.headers[phase];
> +    for (let name in add) {
> +      browser.test.assertTrue(headers.some(h => h.name.toLowerCase() === name.toLowerCase() && h.value === add[name]), `header ${name} correctly injected in ${phase}Headers`);

Can you break out the `headers.some()` thing to a separate line/variable? This gives me a headache.

And, for that matter, can we just replace the `.some()` with a `.find()` for the header name, and then mame this a `.assertEq()` for the expected value?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:137
(Diff revision 5)
> +    for (let name in add) {
> +      browser.test.assertTrue(headers.some(h => h.name.toLowerCase() === name.toLowerCase() && h.value === add[name]), `header ${name} correctly injected in ${phase}Headers`);
> +    }
> +
> +    let modifiedAny = false;
> +    for (let header of headers.filter(h => h.name in modify)) {

Can we skip the `.filter` and just add a nested if?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:145
(Diff revision 5)
> +      modifiedAny = true;
> +    }
> +    browser.test.assertTrue(modifiedAny, `at least one modified ${phase}Headers element`);
> +
> +    for (let name of remove) {
> +      browser.test.assertFalse(headers.some(h => h.name === name), `deleted header ${name} still found in ${phase}Headers`);

Again, can we move the `.some()` to a separate line?

::: toolkit/components/extensions/test/mochitest/head_webrequest.js:174
(Diff revision 5)
> +        // Validate requestId if it's already set, this happens with redirects.
> +        if (expected.test.requestId !== undefined) {
> +          browser.test.assertEq("string", typeof expected.test.requestId, `requestid ${expected.test.requestId} is string`);
> +          browser.test.assertEq("string", typeof details.requestId, `requestid ${details.requestId} is string`);
> +          browser.test.assertEq("number", typeof parseInt(details.requestId, 10), "parsed requestid is number");
> +          browser.test.assertTrue(parseInt(expected.test.requestId, 10) !== parseInt(details.requestId, 10),

`assertNotEq`

And the parseInt shouldn't be necessary.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:78
(Diff revision 5)
> +    },
> +    "file_image_redirect.png": {
> +      status: 302,
> +      type: "image",
> +      events: ["onBeforeRequest", "onBeforeSendHeaders", "onBeforeRedirect"],
> +      redirect:  "file_image_good.png",

Nit: Extra space.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:104
(Diff revision 5)
> +    },
> +    "file_script_redirect.js": {
> +      status: 302,
> +      type: "script",
> +      events: ["onBeforeRequest", "onBeforeSendHeaders", "onBeforeRedirect"],
> +      redirect:  "file_script_good.js",

Nit: Extra space.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8807365 [details]
Bug 1314492 refactor webrequest tests,

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df489872d561
refactor webrequest tests, r=kmag

Comment 14

[Carsten Book [:Tomcat]]

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