Closed Bug 737615 Opened 8 years ago Closed 7 years ago

Remove use of synchronous cache API from unit tests

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: u408661, Assigned: michal)

References

Details

Attachments

(7 files, 6 obsolete files)

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: nobody → michal.novotny
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+
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)
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
Attachment #632079 - Attachment is obsolete: true
Attachment #632166 - Attachment is obsolete: true
Attached patch browser tests v1 (obsolete) — Splinter Review
Attachment #635279 - Flags: review?(bsmith)
Attached patch content tests v1 (obsolete) — Splinter Review
Attachment #635280 - Flags: review?(bsmith)
Attached patch dom tests v1Splinter Review
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
(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.
Attached patch browser tests v2Splinter Review
Attachment #635279 - Attachment is obsolete: true
Attached patch content tests v2 (obsolete) — Splinter Review
Attachment #635280 - Attachment is obsolete: true
(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.
(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).
Depends on: 775482
Attached patch fix for test_bug651100.js (obsolete) — Splinter Review
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)
Attachment #645013 - Attachment is obsolete: true
Attachment #645013 - Flags: review?(bsmith)
Attachment #646766 - Flags: review?(honzab.moz)
Attached patch dom tests part 2Splinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/2147868fa8d5
https://hg.mozilla.org/mozilla-central/rev/a5a0186233f6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Blocks: 792735
You need to log in before you can comment on or make changes to this bug.