u408661 Reporter
	Description • 13 years ago

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.

	Michal Novotny [:michal] Assignee
	Comment 1 • 12 years ago

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

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 2 • 12 years ago

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.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 3 • 12 years ago

Michal, this patch (to be applied on top of your patch) simply makes the change I suggested in my review comment.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 4 • 12 years ago

The tests for bug 761228 use the functionality from head_cache.js. Thanks, Michal, for factoring that out so other tests can use it.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 9 • 12 years ago

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.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 10 • 12 years ago

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.

	Honza Bambas (:mayhemer)
	Comment 11 • 12 years ago

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?

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 12 • 12 years ago

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.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 13 • 12 years ago

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

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 14 • 12 years ago

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

	Michal Novotny [:michal] Assignee
	Comment 15 • 12 years ago

(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.

	Michal Novotny [:michal] Assignee
	Comment 18 • 12 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.

	Michal Novotny [:michal] Assignee
	Comment 19 • 12 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

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 20 • 12 years ago

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.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 21 • 12 years ago

(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).

	Michal Novotny [:michal] Assignee
	Comment 23 • 12 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

	Ed Morley [:emorley]
	Comment 24 • 12 years ago

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

	Michal Novotny [:michal] Assignee
	Comment 25 • 12 years ago

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()).

	Michal Novotny [:michal] Assignee
	Comment 27 • 12 years ago

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

	Michal Novotny [:michal] Assignee
	Comment 28 • 12 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/a5a0186233f6
http://hg.mozilla.org/integration/mozilla-inbound/rev/2147868fa8d5

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/2147868fa8d5
https://hg.mozilla.org/mozilla-central/rev/a5a0186233f6