1257583 - Add test for bypassing the cache with tabs.reload

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Matthew Wein [:mattw]]

We currently don't have any tests to ensure that `tabs.reload(tabId, {bypassCache: true})` is working as intended.

Comment 1

[Matthew Wein [:mattw]]

Attachment: MozReview Request: Bug 1257583 - Add test for bypassing the cache with tabs.reload.

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

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8737992 [details]
MozReview Request: Bug 1257583 - Add test for bypassing the cache with tabs.reload.

https://reviewboard.mozilla.org/r/44239/#review40923

Looks good. Just needs a bit of refactoring and one more test.

::: browser/components/extensions/test/browser/browser_ext_tabs_reload_bypass_cache.js:17
(Diff revision 1)
> +      const BASE = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/";
> +      const URL = BASE + "file_bypass_cache.sjs";
> +
> +      browser.tabs.create({url: URL}).then((tab) => {
> +        return new Promise(resolve => {
> +          browser.tabs.onUpdated.addListener(function listener(tabId, changed, tab_) {

Can you move this into a helper function and use it for both tests?

::: browser/components/extensions/test/browser/browser_ext_tabs_reload_bypass_cache.js:35
(Diff revision 1)
> +          if (changed.url == URL) {
> +            browser.tabs.executeScript(tabId, {
> +              code: "document.body.textContent",
> +            }).then(textContent => {
> +              let [pragma, cacheControl] = textContent.split(":");
> +              browser.test.assertTrue(pragma == "no-cache" && cacheControl == "no-cache", "The correct headers should be set for bypassing the cache");

browser.test.assertEq("no-cache", pragma, ...);
    browser.test.assertEq("no-cache", cacheControl, ...);

::: browser/components/extensions/test/browser/browser_ext_tabs_reload_bypass_cache.js:37
(Diff revision 1)
> +              code: "document.body.textContent",
> +            }).then(textContent => {
> +              let [pragma, cacheControl] = textContent.split(":");
> +              browser.test.assertTrue(pragma == "no-cache" && cacheControl == "no-cache", "The correct headers should be set for bypassing the cache");
> +              browser.tabs.remove(tabId);
> +              browser.test.notifyPass("tabs.reload_bypass_cache");

We should also test without `bypassCache`.

::: browser/components/extensions/test/browser/file_bypass_cache.sjs:6
(Diff revision 1)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80 ft=javascript: */
> +"use strict";
> +
> +function handleRequest(request, response) {
> +  response.processAsync();

This isn't necessary. Neither is the `.finish()`

::: browser/components/extensions/test/browser/file_bypass_cache.sjs:8
(Diff revision 1)
> +"use strict";
> +
> +function handleRequest(request, response) {
> +  response.processAsync();
> +  response.setHeader("Content-Type", "text/plain; charset=UTF-8", false);
> +  response.write(request._headers._headers.pragma);

These are private members. Please use `request.getHeader(...)` instead.

Comment 3

[Matthew Wein [:mattw]]

Comment on attachment 8737992 [details]
MozReview Request: Bug 1257583 - Add test for bypassing the cache with tabs.reload.

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

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8737992 [details]
MozReview Request: Bug 1257583 - Add test for bypassing the cache with tabs.reload.

https://reviewboard.mozilla.org/r/44239/#review41413

Looks good. Thanks

::: browser/components/extensions/test/browser/browser_ext_tabs_reload_bypass_cache.js:49
(Diff revision 2)
> +        let [pragma, cacheControl] = textContent.split(":");
> +        browser.test.assertEq("no-cache", pragma, "`pragma` should be set to `no-cache` when bypassCache is true");
> +        browser.test.assertEq("no-cache", cacheControl, "`cacheControl` should be set to `no-cache` when bypassCache is true");
> +        browser.tabs.remove(tabId);
> +        browser.test.notifyPass("tabs.reload_bypass_cache");
> +      });

Please add:

    )}.catch(e => {
      browser.test.fail(`Error: ${e} :: ${e.stack}`);
      browser.test.notifyFail("tabs.reload_bypass_cache");
    });

Comment 5

[Matthew Wein [:mattw]]

Comment on attachment 8737992 [details]
MozReview Request: Bug 1257583 - Add test for bypassing the cache with tabs.reload.

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

Comment 6

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37898edc758c

Comment 7

[Matthew Wein [:mattw]]

Comment on attachment 8737992 [details]
MozReview Request: Bug 1257583 - Add test for bypassing the cache with tabs.reload.

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

Comment 8

[Matthew Wein [:mattw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9345b16a63c&selectedJob=19167770

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f6241b9dc0e9

Comment 10

[Carsten Book [:Tomcat]]

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