Last Comment Bug 737615 - Remove use of synchronous cache API from unit tests
: Remove use of synchronous cache API from unit tests
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Michal Novotny (:michal)
:
Mentors:
Depends on: 775482
Blocks: 695399 761228 792735
  Show dependency treegraph
 
Reported: 2012-03-20 13:29 PDT by Nicholas Hurley [:nwgh][:hurley]
Modified: 2013-03-19 06:42 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 - fixed necko unit tests (86.68 KB, patch)
2012-06-11 17:26 PDT, Michal Novotny (:michal)
brian: review+
Details | Diff | Review
Replace 'new OpenCacheEntry(...)' with 'asyncOpenCacheEntry' (19.87 KB, patch)
2012-06-12 01:40 PDT, Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
michal.novotny: review+
Details | Diff | Review
necko tests v2 - merged with Brian's patch #632166 (87.65 KB, patch)
2012-06-21 06:10 PDT, Michal Novotny (:michal)
brian: review+
brian: checkin+
Details | Diff | Review
browser tests v1 (6.49 KB, patch)
2012-06-21 06:11 PDT, Michal Novotny (:michal)
brian: review+
Details | Diff | Review
content tests v1 (2.97 KB, patch)
2012-06-21 06:12 PDT, Michal Novotny (:michal)
brian: review+
Details | Diff | Review
dom tests v1 (44.17 KB, patch)
2012-06-21 06:12 PDT, Michal Novotny (:michal)
honzab.moz: review+
Details | Diff | Review
necko tests - v1-v2 interdiff (5.02 KB, patch)
2012-07-10 17:25 PDT, Michal Novotny (:michal)
brian: review+
Details | Diff | Review
browser tests v2 (6.03 KB, patch)
2012-07-10 17:25 PDT, Michal Novotny (:michal)
no flags Details | Diff | Review
content tests v2 (2.87 KB, patch)
2012-07-10 17:26 PDT, Michal Novotny (:michal)
no flags Details | Diff | Review
content tests v3 - removed is/isnot comparing against null (2.86 KB, patch)
2012-07-11 02:00 PDT, Michal Novotny (:michal)
no flags Details | Diff | Review
fix for test_bug651100.js (9.24 KB, patch)
2012-07-23 11:59 PDT, Michal Novotny (:michal)
no flags Details | Diff | Review
necko tests part2 (15.60 KB, patch)
2012-07-27 17:06 PDT, Michal Novotny (:michal)
honzab.moz: review+
Details | Diff | Review
dom tests part 2 (2.20 KB, patch)
2012-07-27 17:09 PDT, Michal Novotny (:michal)
honzab.moz: review+
Details | Diff | Review

Description Nicholas Hurley [:nwgh][:hurley] 2012-03-20 13:29:07 PDT
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.
Comment 1 Michal Novotny (:michal) 2012-06-11 17:26:28 PDT
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
Comment 2 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-12 01:37:32 PDT
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.
Comment 3 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-12 01:40:50 PDT
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.
Comment 4 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-12 11:36:18 PDT
The tests for bug 761228 use the functionality from head_cache.js. Thanks, Michal, for factoring that out so other tests can use it.
Comment 5 Michal Novotny (:michal) 2012-06-21 06:10:40 PDT
Created attachment 635278 [details] [diff] [review]
necko tests v2 - merged with Brian's patch #632166
Comment 6 Michal Novotny (:michal) 2012-06-21 06:11:27 PDT
Created attachment 635279 [details] [diff] [review]
browser tests v1
Comment 7 Michal Novotny (:michal) 2012-06-21 06:12:05 PDT
Created attachment 635280 [details] [diff] [review]
content tests v1
Comment 8 Michal Novotny (:michal) 2012-06-21 06:12:45 PDT
Created attachment 635281 [details] [diff] [review]
dom tests v1
Comment 9 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-26 18:30:09 PDT
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.
Comment 10 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-26 18:43:32 PDT
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.
Comment 11 Honza Bambas (:mayhemer) 2012-06-27 16:56:39 PDT
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?
Comment 12 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-28 15:33:11 PDT
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.
Comment 13 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-28 20:01:46 PDT
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
Comment 14 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-06-28 20:02:23 PDT
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
Comment 15 Michal Novotny (:michal) 2012-07-10 17:25:07 PDT
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.
Comment 16 Michal Novotny (:michal) 2012-07-10 17:25:50 PDT
Created attachment 640870 [details] [diff] [review]
browser tests v2
Comment 17 Michal Novotny (:michal) 2012-07-10 17:26:24 PDT
Created attachment 640872 [details] [diff] [review]
content tests v2
Comment 18 Michal Novotny (:michal) 2012-07-10 17:27:25 PDT
(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.
Comment 19 Michal Novotny (:michal) 2012-07-10 17:45:13 PDT
(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 20 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-07-10 18:08:54 PDT
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.
Comment 21 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-07-10 18:17:26 PDT
(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).
Comment 22 Michal Novotny (:michal) 2012-07-11 02:00:54 PDT
Created attachment 640981 [details] [diff] [review]
content tests v3 - removed is/isnot comparing against null
Comment 25 Michal Novotny (:michal) 2012-07-23 11:59:26 PDT
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()).
Comment 26 Michal Novotny (:michal) 2012-07-27 17:06:52 PDT
Created attachment 646766 [details] [diff] [review]
necko tests part2
Comment 27 Michal Novotny (:michal) 2012-07-27 17:09:07 PDT
Created attachment 646770 [details] [diff] [review]
dom tests part 2

I forgot to remove one openCacheEntry() in "dom tests v1".

Note You need to log in before you can comment on or make changes to this bug.