Add test for bypassing the cache with tabs.reload

RESOLVED FIXED in Firefox 48

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mattw, Assigned: mattw)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

2 years ago
Iteration: 48.1 - Mar 21 → ---

Updated

2 years ago
Assignee: nobody → mwein
(Assignee)

Updated

2 years ago
Iteration: --- → 48.3 - Apr 18
(Assignee)

Comment 1

2 years ago
Created attachment 8737992 [details]
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/
Attachment #8737992 - Flags: review?(kmaglione+bmo)
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.
(Assignee)

Comment 3

2 years ago
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/
Attachment #8737992 - Flags: review?(kmaglione+bmo)
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");
    });
Attachment #8737992 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 5

2 years ago
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/
(Assignee)

Comment 7

2 years ago
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/
Attachment #8737992 - Attachment description: MozReview Request: Bug 1257583 - Add test for bypassing the cache with tabs.reload r?kmag → MozReview Request: Bug 1257583 - Add test for bypassing the cache with tabs.reload.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f6241b9dc0e9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.