Remove use of synchronous cache API from unit tests

RESOLVED FIXED in mozilla17

Status

()

Core
Networking: Cache
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nwgh, Assigned: michal)

Tracking

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 6 obsolete attachments)

87.65 KB, patch
briansmith
: review+
briansmith
: checkin+
Details | Diff | Splinter Review
44.17 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
5.02 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
6.03 KB, patch
Details | Diff | Splinter Review
2.86 KB, patch
Details | Diff | Splinter Review
15.60 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
2.20 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
We have a lot of unit tests that make use of the synchronous cache API. If we ever want to get rid of that API (we do), then we need to stop using it in tests.

Filing this under Networking:Cache since that's where a good portion of those tests live. There are plenty of tests in other components that make use of the synchronous cache API, too.
(Assignee)

Updated

5 years ago
Assignee: nobody → michal.novotny
(Assignee)

Comment 1

5 years ago
Created attachment 632079 [details] [diff] [review]
patch v1 - fixed necko unit tests

This patch removes usage of synchronous openCacheEntry() in all necko unit tests.


I'm not sure what to do with following files. Do we ever run these tests?

  netwerk/test/TestAsyncCache.js
  netwerk/test/TestCacheCollisions.js
  netwerk/test/TestCachePerformance.js
  netwerk/test/TestDiskCache.js
  netwerk/test/TestObjectCache.js
Attachment #632079 - Flags: review?(bsmith)
Comment on attachment 632079 [details] [diff] [review]
patch v1 - fixed necko unit tests

Review of attachment 632079 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

I have one suggestion: it is really unclear that "new OpenCacheEntry(....)" opens a cache entry automatically as part of constructing the object. It would be better to use "asyncOpenCacheEntry(....)" instead. I will attach a patch on top of yours that makes that change, for you to review.
Attachment #632079 - Flags: review?(bsmith) → review+
Created attachment 632166 [details] [diff] [review]
Replace 'new OpenCacheEntry(...)' with 'asyncOpenCacheEntry'

Michal, this patch (to be applied on top of your patch) simply makes the change I suggested in my review comment.
Attachment #632166 - Flags: review?(michal.novotny)
(Assignee)

Updated

5 years ago
Attachment #632166 - Flags: review?(michal.novotny) → review+
The tests for bug 761228 use the functionality from head_cache.js. Thanks, Michal, for factoring that out so other tests can use it.
Blocks: 761228
(Assignee)

Comment 5

5 years ago
Created attachment 635278 [details] [diff] [review]
necko tests v2 - merged with Brian's patch #632166
Attachment #632079 - Attachment is obsolete: true
Attachment #632166 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 635279 [details] [diff] [review]
browser tests v1
Attachment #635279 - Flags: review?(bsmith)
(Assignee)

Comment 7

5 years ago
Created attachment 635280 [details] [diff] [review]
content tests v1
Attachment #635280 - Flags: review?(bsmith)
(Assignee)

Comment 8

5 years ago
Created attachment 635281 [details] [diff] [review]
dom tests v1
Attachment #635281 - Flags: review?(honzab.moz)
Comment on attachment 635280 [details] [diff] [review]
content tests v1

Review of attachment 635280 [details] [diff] [review]:
-----------------------------------------------------------------

r+ but I recommend making the changes I suggested below.

::: content/html/content/test/browser_bug649778.js
@@ +22,5 @@
> +  _shouldExist : false,
> +
> +  onCacheEntryAvailable: function oCEA(entry, access, status) {
> +    if (this._shouldExist) {
> +      isnot(entry, null, "Entry not found");

I think this should just be:

is(entry, "Entry not found");

since null isn't "truthy".

@@ +25,5 @@
> +    if (this._shouldExist) {
> +      isnot(entry, null, "Entry not found");
> +      is(status, Components.results.NS_OK, "Entry not found");
> +    } else {
> +      is(entry, null, "Entry found");

do_check_null(entry, "Entry found");

@@ +30,5 @@
> +      is(status, Components.results.NS_ERROR_CACHE_KEY_NOT_FOUND,
> +         "Invalid error code");
> +    }
> +
> +    setTimeout(this._cb, 0);

I think you can write this as:

var session = cache.createSession(...); // as you had it

// local variable 
var checkCacheListener = {
  onCacheEntryAvailable: function oCEA(entry, access, status) {
    if (shouldExist) {
      ...
    } else {
      ...
    }

    setTimeout(cb, 0);
  }
};

session.asyncOpenCacheEntry(url, Ci.nsICache.ACCESS_READ,
                            checkCacheListener);

Since JavaScript has closures.

@@ +49,4 @@
>    is(wyciwygURL.substring(0, 10), "wyciwyg://", "Unexpected URL.");
>    popup.close()
>  
> +  checkCache(wyciwygURL, Components.interfaces.nsICache.STORE_ON_DISK, false, testContinue2);

Similarly, this can be written:

function testContinue() {
  var wyciwygURL = getPopupURL();
  is(wyciwygURL.substring(0, 10), "wyciwyg://",
     "Unexpected URL.");
  popup.close();

  checkCache(wyciwygURL, Ci.nsICache.STORE_ON_DISK, false,
    function() {
      checkCache(wyciwygURL, Ci.nsICache.STORE_IN_MEMORY, 
                 true, finish);
    });
}

Again, because JavaScript has closures.
Attachment #635280 - Flags: review?(bsmith) → review+
Comment on attachment 635279 [details] [diff] [review]
browser tests v1

Review of attachment 635279 [details] [diff] [review]:
-----------------------------------------------------------------

I guess we can't use the asyncOpenCacheEntry function in cache_head.js because cache_head.js isn't available here, right?

::: browser/base/content/test/browser_sanitizeDialog.js
@@ +504,5 @@
> +    function CacheListener(windowHelper)
> +    {
> +      this._wh = windowHelper;
> +    }
> +    CacheListener.prototype = {

Seems like this can be written like the other tests I reviewed; no need for "prototype" and QueryInterface goop, right?

@@ +507,5 @@
> +    }
> +    CacheListener.prototype = {
> +      _wh: null,
> +
> +      QueryInterface: function(iid) {

I heard that xpconnect will automatically do the QueryInterface magically for you so you don't have to implement QueryInterface yourself. Is that not true?

@@ +520,5 @@
> +        var content = "content";
> +        stream.write(content, content.length);
> +        stream.close();
> +        entry.close();
> +        this._wh.open();

windowHelper.open() and no _wh member needed?

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js
@@ +136,3 @@
>  }
>  
> +var storeCacheListener = {

Move this to a local variable like I suggested for the other tests, so you can just use a closure. This makes it clearer that storeCacheListener isn't used by any other code.

@@ +149,5 @@
> +      do_throw("oStream.write has not written all data!\n" +
> +               "  Expected: " + written  + "\n" +
> +               "  Actual: " + this._content.length + "\n");
> +    }
> +    os.close();

Will the data actually be written here? Don't we need to sync with the cache thread, since the data is actually written on the cache thread?

@@ +165,5 @@
> +                              Ci.nsICache.ACCESS_READ,
> +                              checkCacheListener);
> +}
> +
> +var checkCacheListener = {

Also move this to a local variable, to limit its scope to the scope it is used in.
Attachment #635279 - Flags: review?(bsmith) → review+
Comment on attachment 635281 [details] [diff] [review]
dom tests v1

Review of attachment 635281 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

I haven't checked this manually my self.

::: dom/tests/mochitest/ajax/offline/offlineTests.js
@@ +220,5 @@
>    var waitFunc = function() {
>      var cacheSession = OfflineTest.getActiveSession();
> +    cacheSession.asyncOpenCacheEntry(url,
> +                                     Ci.nsICache.ACCESS_READ,
> +                                     waitForAddListener);

Can this throw?

@@ +313,5 @@
>    }
>  
> +  var _checkCacheListener = {
> +    onCacheEntryAvailable: function(entry, access, status) {
> +      netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

Is this really needed?
Attachment #635281 - Flags: review?(honzab.moz) → review+
Comment on attachment 635278 [details] [diff] [review]
necko tests v2 - merged with Brian's patch #632166

I checked in just this part because I needed head_cache.js for another checkin and it was already r+d.
Attachment #635278 - Flags: review+
Attachment #635278 - Flags: checkin+
I backed out the change to netwerk/test/unit/test_bug651100.js from attachment 635278 [details] [diff] [review] because it caused a test failure (seems like it would become random orange):

https://hg.mozilla.org/mozilla-central/rev/081d8578beb1
Here is the relevant part of log for the test failure on mozilla-central:

TEST-PASS | c:/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_bug651100.js | [write_big_datafile : 26] 0 == 0

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST-PASS | c:/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_bug651100.js | [null : 65] 0 == 0

TEST-PASS | c:/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_bug651100.js | [check_cache_size : 77] true == true

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-PASS | c:/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_bug651100.js | [write_and_doom_big_metafile : 42] 0 == 0

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST-UNEXPECTED-FAIL | c:/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_bug651100.js | 1048576 == 0 - See following stack:
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: do_throw :: line 440
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: _do_check_eq :: line 534
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: do_check_eq :: line 555
JS frame :: c:/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_bug651100.js :: <TOP_LEVEL> :: line 65
JS frame :: c:/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_bug651100.js :: check_cache_size :: line 76
JS frame :: c:/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_bug651100.js :: run_test_3 :: line 114
JS frame :: c:\talos-slave\test\build\xpcshell\head.js :: <TOP_LEVEL> :: line 407

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "'Abort' when calling method: [nsICacheVisitor::visitDevice]"  nsresult: "0x80004004 (NS_ERROR_ABORT)"  location: "JS frame :: c:/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_bug651100.js :: check_cache_size :: line 76"  data: no]

TEST-INFO | (xpcshell/head.js) | exiting test
(Assignee)

Comment 15

5 years ago
Created attachment 640868 [details] [diff] [review]
necko tests - v1-v2 interdiff

(In reply to Brian Smith (:bsmith) from comment #12)
> Comment on attachment 635278 [details] [diff] [review]
> necko tests v2 - merged with Brian's patch #632166
> 
> I checked in just this part because I needed head_cache.js for another
> checkin and it was already r+d.

You've checked in an older version of this patch. This is a diff against it.
(Assignee)

Comment 16

5 years ago
Created attachment 640870 [details] [diff] [review]
browser tests v2
Attachment #635279 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 640872 [details] [diff] [review]
content tests v2
Attachment #635280 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
(In reply to Brian Smith (:bsmith) from comment #9)
> ::: content/html/content/test/browser_bug649778.js
> @@ +22,5 @@
> > +  _shouldExist : false,
> > +
> > +  onCacheEntryAvailable: function oCEA(entry, access, status) {
> > +    if (this._shouldExist) {
> > +      isnot(entry, null, "Entry not found");
> 
> I think this should just be:
> 
> is(entry, "Entry not found");
> 
> since null isn't "truthy".

Function is() needs 2 arguments to compare. What's wrong with comparing entry with null?


> @@ +25,5 @@
> > +    if (this._shouldExist) {
> > +      isnot(entry, null, "Entry not found");
> > +      is(status, Components.results.NS_OK, "Entry not found");
> > +    } else {
> > +      is(entry, null, "Entry found");
> 
> do_check_null(entry, "Entry found");

do_check_null() is available only in xpcshell tests


(In reply to Brian Smith (:bsmith) from comment #10)
> I guess we can't use the asyncOpenCacheEntry function in cache_head.js
> because cache_head.js isn't available here, right?

Right.


> ::: browser/base/content/test/browser_sanitizeDialog.js
> @@ +504,5 @@
> > +    function CacheListener(windowHelper)
> > +    {
> > +      this._wh = windowHelper;
> > +    }
> > +    CacheListener.prototype = {
> 
> Seems like this can be written like the other tests I reviewed; no need for
> "prototype" and QueryInterface goop, right?

Right, fixed.


> @@ +149,5 @@
> > +      do_throw("oStream.write has not written all data!\n" +
> > +               "  Expected: " + written  + "\n" +
> > +               "  Actual: " + this._content.length + "\n");
> > +    }
> > +    os.close();
> 
> Will the data actually be written here? Don't we need to sync with the cache
> thread, since the data is actually written on the cache thread?

This write is synchronous.
(Assignee)

Comment 19

5 years ago
(In reply to Honza Bambas (:mayhemer) from comment #11)
> ::: dom/tests/mochitest/ajax/offline/offlineTests.js
> @@ +220,5 @@
> >    var waitFunc = function() {
> >      var cacheSession = OfflineTest.getActiveSession();
> > +    cacheSession.asyncOpenCacheEntry(url,
> > +                                     Ci.nsICache.ACCESS_READ,
> > +                                     waitForAddListener);
> 
> Can this throw?

The request is dispatched to the cacheIO thread because we call here asyncOpenCacheEntry() on the main thread. So it can throw only in case the dispatch fails, which shouldn't happen.


> @@ +313,5 @@
> >    }
> >  
> > +  var _checkCacheListener = {
> > +    onCacheEntryAvailable: function(entry, access, status) {
> > +      netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> 
> Is this really needed?

Test test_identicalManifest.html fails without it:

TEST-UNEXPECTED-FAIL | unknown test url | an unexpected uncaught JS exception reported through window.onerror - Error: Permission denied for <http://mochi.test:8888> to call method UnnamedClass.close at http://mochi.test:8888/tests/dom/tests/mochitest/ajax/offline/offlineTests.js:324
JavaScript error: http://mochi.test:8888/tests/dom/tests/mochitest/ajax/offline/offlineTests.js, line 324: Permission denied for <http://mochi.test:8888> to call method UnnamedClass.close
Comment on attachment 640868 [details] [diff] [review]
necko tests - v1-v2 interdiff

Review of attachment 640868 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know if you need the r+ but I'm giving you one anyway.
Attachment #640868 - Flags: review+
(In reply to Michal Novotny (:michal) from comment #18)
> (In reply to Brian Smith (:bsmith) from comment #9)
> > ::: content/html/content/test/browser_bug649778.js
> > @@ +22,5 @@
> > > +  _shouldExist : false,
> > > +
> > > +  onCacheEntryAvailable: function oCEA(entry, access, status) {
> > > +    if (this._shouldExist) {
> > > +      isnot(entry, null, "Entry not found");
> > 
> > I think this should just be:
> > 
> > is(entry, "Entry not found");
> > 
> > since null isn't "truthy".
> 
> Function is() needs 2 arguments to compare.

Sorry, I meant ok(entry, "Entry not found");

> What's wrong with comparing entry with null?

This is just my misunderstanding. (In general, I do not like comparing against null in Javascript because I find the rules confusing).
(Assignee)

Comment 22

5 years ago
Created attachment 640981 [details] [diff] [review]
content tests v3 - removed is/isnot comparing against null
Attachment #640872 - Attachment is obsolete: true
(Assignee)

Comment 23

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/3437e9d38623
http://hg.mozilla.org/integration/mozilla-inbound/rev/72019f633129
http://hg.mozilla.org/integration/mozilla-inbound/rev/ad66250ace28
http://hg.mozilla.org/integration/mozilla-inbound/rev/7776db42d372
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/3437e9d38623
https://hg.mozilla.org/mozilla-central/rev/72019f633129
https://hg.mozilla.org/mozilla-central/rev/ad66250ace28
https://hg.mozilla.org/mozilla-central/rev/7776db42d372

Updated

5 years ago
Depends on: 775482
(Assignee)

Comment 25

5 years ago
Created attachment 645013 [details] [diff] [review]
fix for test_bug651100.js

syncWithCacheIOThread is still needed in test_bug651100.js. Also I've found out that writing of big metafile didn't work properly. Metadata wasn't written while closing the entry because the entry was doomed (see comment in write_big_metafile()).
Attachment #645013 - Flags: review?(bsmith)
(Assignee)

Comment 26

5 years ago
Created attachment 646766 [details] [diff] [review]
necko tests part2
Attachment #645013 - Attachment is obsolete: true
Attachment #645013 - Flags: review?(bsmith)
Attachment #646766 - Flags: review?(honzab.moz)
(Assignee)

Comment 27

5 years ago
Created attachment 646770 [details] [diff] [review]
dom tests part 2

I forgot to remove one openCacheEntry() in "dom tests v1".
Attachment #646770 - Flags: review?(honzab.moz)
Attachment #646766 - Flags: review?(honzab.moz) → review+
Attachment #646770 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 28

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/a5a0186233f6
http://hg.mozilla.org/integration/mozilla-inbound/rev/2147868fa8d5
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/2147868fa8d5
https://hg.mozilla.org/mozilla-central/rev/a5a0186233f6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Blocks: 792735
You need to log in before you can comment on or make changes to this bug.