834498 - Remove usage of setPageTitle and isVisited from Seamonkey's tests

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

These 2 APIs are going away, those uses should be fixed.

I suggest waiting for bug 739217 completion, then it's possible may be matter of just porting a new version of the tests.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

We are quite near removal of the APIs on central, is any SM contributor interested in working on this bug?

Comment 2

[Frank Wein [:mcsmurf]]

Yes, me :) just waited for bug 739217 to be resolved.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Cool, thanks. If you need any help just ping me in #developers.
See also bug 820797.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

any news here? we are about to proceed with the removal in central.

Comment 5

[Frank Wein [:mcsmurf]]

Attachment: First attempt at patch

At first I had problems with the new async API (the test design relied on return codes from functions), then I fixed that, then I noticed a xpcshell test is actually enough. Well, basically I ended up rewriting the test :), but I learned a lot from that. Now the test uses add_task, async APIs, yield and all those modern things. Still need to fix a few things in the test.

Comment 6

[Frank Wein [:mcsmurf]]

Attachment: First attempt at patch

Comment 7

[Frank Wein [:mcsmurf]]

Attachment: Patch

Marco: I need a bit of help with this patch, no full review needed (someone else will have to do that). I think you understand the JS generator thing a bit better than me. In general the test works like this: First it calls the setup() function for each test and after that is done it calls the check() function (similar to the test_preventive_maintenance.js test). Some of the setup() and the check() functions use JS promises (promiseAsyncUpdates and promiseOpenCacheEntry). Now I thought it would be enough to block on the promise with "yield" until the promise is resolved (for example "entry = yield promiseOpenCacheEntry(...)" or "yield promiseAddVisits"). But now I also need to call the setup function with "yield test.setup()" to make the test pass. I suspect I'm missing something here, but I don't know what. I'm fairly new to the JS generator and promise thing..
BTW: The cache change is not related to this bug here, but it also needs to be fixed (switch from sync to async API).

Comment 8

[Frank Wein [:mcsmurf]]

I actually forgot my question: Is my code right or wrong? And why do I need to call "yield test.setup()" and "test.check()" is enough, even though also some check functions use JS promises? Or is something flawed in my code?

Comment 9

[Frank Wein [:mcsmurf]]

To make life easier: The actual test function can be found at the end of test_browser_sanitizer.js, it gets added via add_task

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 722723 [details] [diff] [review]
Patch

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

yes, it should work, with some fixes

::: suite/modules/test/unit/head.js
@@ +11,5 @@
> +  "resource://gre/modules/commonjs/sdk/core/promise.js");
> +
> +
> +/*
> + * @return {Promise}

missing description (not a valid javadoc)

@@ +19,5 @@
> +{
> +  let deferred = Promise.defer();
> +
> +  let cacheService = Components.classes["@mozilla.org/network/cache-service;1"]
> +                               .getService(Components.interfaces.nsICacheService);

Services.cache... though it is unused since you pass a cache session as argument...

@@ +20,5 @@
> +  let deferred = Promise.defer();
> +
> +  let cacheService = Components.classes["@mozilla.org/network/cache-service;1"]
> +                               .getService(Components.interfaces.nsICacheService);
> +  

trailing spaces

@@ +22,5 @@
> +  let cacheService = Components.classes["@mozilla.org/network/cache-service;1"]
> +                               .getService(Components.interfaces.nsICacheService);
> +  
> +  let cacheListener = {
> +    onCacheEntryAvailable: function (entry, access, status) {

is onCacheEntryAvailable invoked even if it's not available?

@@ +23,5 @@
> +                               .getService(Components.interfaces.nsICacheService);
> +  
> +  let cacheListener = {
> +    onCacheEntryAvailable: function (entry, access, status) {
> +//      is(status, Cr.NS_OK);

leftover?

@@ +25,5 @@
> +  let cacheListener = {
> +    onCacheEntryAvailable: function (entry, access, status) {
> +//      is(status, Cr.NS_OK);
> +      deferred.resolve(entry);
> +    }

should also provide empty onCacheEntryDoomed method per idl

::: suite/modules/test/browser_sanitizer.js
@@ +1,4 @@
> +// Need to explicitly load profile for Places
> +do_get_profile();
> +
> +Cu.import("resource://gre/modules/Services.jsm");

you could move these to head.js, they make sense for any test probably.

@@ +14,5 @@
>      window = wm.getEnumerator(aType);
>    return window;
>  }
>  
> +let cs = null;

this looks a leftover, otherwise it's unlikely to work.

@@ +22,5 @@
>      desc: "Cache",
>      setup: function() {
>        var entry = null;
>        var cacheService = Components.classes["@mozilla.org/network/cache-service;1"]
>                                     .getService(Components.interfaces.nsICacheService);

directly use Services.cache

@@ +25,5 @@
>        var cacheService = Components.classes["@mozilla.org/network/cache-service;1"]
>                                     .getService(Components.interfaces.nsICacheService);
>        try {
>          this.cs = cacheService.createSession("SanitizerTest", Components.interfaces.nsICache.STORE_ANYWHERE, true);
> +        entry = yield promiseOpenCacheEntry("http://santizer.test", Components.interfaces.nsICache.ACCESS_READ_WRITE);

should pass this.cs as third arg

may use Ci. basically everywhere thanks to head.js

@@ +40,1 @@
>        } catch(ex) {}

I'm not sure what this try/catch is protecting now, at first glance I'd say "bugs". I think it's worth to remove it

@@ +71,1 @@
>        } catch(ex) {}

ditto on try/catch

@@ +72,5 @@
>  
>        if (entry) {
>          entry.close();
> +        rv = true;
> +      } 

trailing space

@@ +74,5 @@
>          entry.close();
> +        rv = true;
> +      } 
> +
> +      do_check_eq(rv, !aShouldBeCleared);

why not checking entry like you did in previous test?

@@ +102,4 @@
>    history: {
>      desc: "History",
>      setup: function() {
> +      var ret = false;

unused?

@@ +122,5 @@
>        var results = history.executeQuery(query, options).root;
>        results.containerOpen = true;
>        for (var i = 0; i < results.childCount; i++) {
> +        if (results.getChild(i).uri == "http://sanitizer.test/") {
> +          rv = true;

and break?

@@ +128,3 @@
>        }
> +
> +      do_check_eq(rv, !aShouldBeCleared);

please close the results container

@@ +131,4 @@
>      }
>    },
>  
> +  urlbar: {  

trailing spaces

@@ +176,1 @@
>          return true;

this test looks still wip

@@ +253,5 @@
>  
>    passwords: {
>      desc: "Login manager",
>      setup: function() {
> +      var succ = false;

unused?

@@ +269,2 @@
>        for (var i = 0; i < logins.length; i++) {
> +        // XXX - return early on success!

and do_throw if you exit the loop.

@@ +303,2 @@
>    var prefs = Services.prefs.getBranch("privacy.item.");
> +  gCurrTest = 0;

unused?

@@ +314,5 @@
>    Sanitizer.sanitize();
>  
>    for (var testName in sanTests) {
>      var test = sanTests[testName];
> +    test.check(true);

should yield for check as well

@@ +336,5 @@
> +  for(var testName in sanTests) {
> +    let test = sanTests[testName];
> +    dump("\nExecuting test: " + testName + "\n" + "*** " + test.desc + "\n");
> +    yield test.setup();
> +    test.check(false);

should yield for check

@@ +346,3 @@
>    }
>  
> +  yield fullSanitize();

add_task(test_fullSanitize) as a separate test

Comment 11

[Frank Wein [:mcsmurf]]

Attachment: Patch

Neil: This patch converts the test from mochitest chrome to a xpcshell unit test and removes deprecated APIs. It also fixes the cache usage from sync to async. Mak already reviewed most of the patch, so work for you should be easier.
Test design is: Call [test].setup, then call [test].check(false) if data is setup correctly, then sanitize data, then call [test].check(true) if data has been cleared. At the end a full sanitize run will be done.
I removed the file.exists(...) check for urlbarhistory.sqlite. I'm not sure if this check made sense once, currently this test fails (not related to the deprecated APIs). After clearing/sanitizing urlbar history, I find it reasonable for the file to be gone.
I don't return early inside the passwords check() function as there is only one login anyway. If there would be another login, the test would fail.
The test runs inside a JS task, see the comment at the beginning of http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/Task.jsm for more information on how JS tasks work. I need to "yield" the calls to setup() and check() so that the JS task will block on the "yield" inside the functions. Otherwise it would just continue and not execute the tests at all (inside check(...)) or the tests would fail (when omitting the yield before setup()).

Comment 12

[Frank Wein [:mcsmurf]]

BTW: The promiseAsyncUpdates and promiseAddVisits functions inside head.js have been copied from FF test code.

Comment 13

[Frank Wein [:mcsmurf]]

Attachment: Patch

Actually we do need to check the password/login thing :) as it could happen that the login does not get stored at all. In this test it will now throw when the username does not have the expected value and also throw when it does not find a login at all (after setup). When it does find a good login, it returns early (as in the original test).

Comment 14

[[e-mail]]

Comment on attachment 727777 [details] [diff] [review]
Patch

>-      try {
Why remove the try/catch? (Or should you have removed the other try/catch above?)

>+    check: function(aShouldBeCleared) {
>+      if (aShouldBeCleared)
>+        do_check_neq(this.cs.getCookieString(this.uri, null), "Sanitizer!");
>+      else
>+        do_check_eq(this.cs.getCookieString(this.uri, null), "Sanitizer!");
>     }
I think this would be much clearer if you passed do_check_eq or do_check_neq as the parameter i.e.
check: function(do_check) {
  do_check(this.cs.getCookieString(this.uri, null), "Sanitizer!");
}

Bonus points for reproducing the original test description string (test.desc + " data cleared [by full sanitize]").

>-    check: function() {
>-      var logins = this.pm.findLogins({}, "http://sanitizer.test", null, "Rick Astley Fan Club");
>+    check: function(aShouldBeCleared) {
>+      let logins = this.pm.findLogins({}, "http://sanitizer.test", null, "Rick Astley Fan Club");
>       for (var i = 0; i < logins.length; i++) {
>-        if (logins[i].username == "dolske")
>-          return true;
>+        if ((logins[i].username == "dolske") == !aShouldBeCleared)
>+          return;
>+        else
>+          do_throw("Username does not have the expected value");
>       }
>-      return false;
>+
>+      if (!aShouldBeCleared)
>+        do_throw("We should not have reached this line");
>     }
This test appears to be incapable of passing?

>-    check: function() {
>-      var domain = {};
>-      var user = {};
>-      var password = {};
>+      let domain = {};
>+      let user = {};
>+      let password = {};
...

>+  for(var testName in sanTests) {
[Space before (]

>+add_task(function test_browser_fullSanitize()
>+{
>+  yield fullSanitize();
>+});
add_task(fullSanitize); should suffice.

Comment 15

[Frank Wein [:mcsmurf]]

Attachment: Patch

(In reply to [e-mail] from comment #14)
> Comment on attachment 727777 [details] [diff] [review]
> Patch
> 
> >-      try {
> Why remove the try/catch? (Or should you have removed the other try/catch
> above?)

Mak thought a try/catch might just cover up bugs and it's not really needed here. And actually I wanted to remove both try/catch :) the tests work totally fine without try/catch and in case an exception gets thrown we want to see it.

> >+    check: function(aShouldBeCleared) {
> >+      if (aShouldBeCleared)
> >+        do_check_neq(this.cs.getCookieString(this.uri, null), "Sanitizer!");
> >+      else
> >+        do_check_eq(this.cs.getCookieString(this.uri, null), "Sanitizer!");
> >     }
> I think this would be much clearer if you passed do_check_eq or do_check_neq
> as the parameter i.e.
> check: function(do_check) {
>   do_check(this.cs.getCookieString(this.uri, null), "Sanitizer!");
> }

But then fixing the code logic for some check() tests gets more complicated (some check tests would then need to compare the do_check value).

> Bonus points for reproducing the original test description string (test.desc
> + " data cleared [by full sanitize]").
> 
> >-    check: function() {
> >-      var logins = this.pm.findLogins({}, "http://sanitizer.test", null, "Rick Astley Fan Club");
> >+    check: function(aShouldBeCleared) {
> >+      let logins = this.pm.findLogins({}, "http://sanitizer.test", null, "Rick Astley Fan Club");
> >       for (var i = 0; i < logins.length; i++) {
> >-        if (logins[i].username == "dolske")
> >-          return true;
> >+        if ((logins[i].username == "dolske") == !aShouldBeCleared)
> >+          return;
> >+        else
> >+          do_throw("Username does not have the expected value");
> >       }
> >-      return false;
> >+
> >+      if (!aShouldBeCleared)
> >+        do_throw("We should not have reached this line");
> >     }
> This test appears to be incapable of passing?

This test passes fine. The first time (aShouldBeCleared=false) it does find a username and the functions returns. The second time (aShouldBeCleared=true) the number of logins is zero and the "if (!aShouldBeCleared)" line prevents throwing an exception.

> >-    check: function() {
> >-      var domain = {};
> >-      var user = {};
> >-      var password = {};
> >+      let domain = {};
> >+      let user = {};
> >+      let password = {};
> ...

Changed back.

> >+  for(var testName in sanTests) {
> [Space before (]

Fixed.

> >+add_task(function test_browser_fullSanitize()
> >+{
> >+  yield fullSanitize();
> >+});
> add_task(fullSanitize); should suffice.

Yes, works fine.

I also added another yield inside fullSanitize which I forgot before.

Comment 16

[[e-mail]]

(In reply to Frank Wein from comment #15)
> (In reply to comment #14)
> > (From update of  attachment 727777 [details] [diff] [review])
> > >+    check: function(aShouldBeCleared) {
> > >+      if (aShouldBeCleared)
> > >+        do_check_neq(this.cs.getCookieString(this.uri, null), "Sanitizer!");
> > >+      else
> > >+        do_check_eq(this.cs.getCookieString(this.uri, null), "Sanitizer!");
> > >     }
> > I think this would be much clearer if you passed do_check_eq or do_check_neq
> > as the parameter i.e.
> > check: function(do_check) {
> >   do_check(this.cs.getCookieString(this.uri, null), "Sanitizer!");
> > }
> 
> But then fixing the code logic for some check() tests gets more complicated
> (some check tests would then need to compare the do_check value).
Only the passwords check ;-)

> > >+    check: function(aShouldBeCleared) {
> > >+      let logins = this.pm.findLogins({}, "http://sanitizer.test", null, "Rick Astley Fan Club");
> > >       for (var i = 0; i < logins.length; i++) {
> > >+        if ((logins[i].username == "dolske") == !aShouldBeCleared)
> > >+          return;
> > >+        else
> > >+          do_throw("Username does not have the expected value");
> > >       }
> > >+
> > >+      if (!aShouldBeCleared)
> > >+        do_throw("We should not have reached this line");
> > >     }
> > This test appears to be incapable of passing?
> This test passes fine. The first time (aShouldBeCleared=false) it does find
> a username and the functions returns. The second time
> (aShouldBeCleared=true) the number of logins is zero and the "if
> (!aShouldBeCleared)" line prevents throwing an exception.
Well, the new code doesn't do the same check as the old code. In fact in the aShouldBeCleared == false case it doesn't even call any of the do_check methods.

One alternative could be to look for the dolske login, and then do_check whether you found it (compare the history check).
Or maybe you could just check the number of logins.

Comment 17

[Frank Wein [:mcsmurf]]

Attachment: Patch 2

I fixed the password check now.

I'm still against the do_check thing, modifying all tests again is a bit of work (I've already spent too much work on this bug here).

Comment 18

[Frank Wein [:mcsmurf]]

BTW: Please note the test currently fails due to Bug 856208.

Comment 19

[[e-mail]]

I just noticed your new files have DOS line endings. Please fix before checking in!

Comment 20

[[e-mail]]

Comment on attachment 731304 [details] [diff] [review]
Patch 2

>+        do_check_eq(dl.displayName,"Sanitizer!");
Nit: space after comma

Don't worry about that do_check thing, it sounded good at the time.

Comment 21

[Frank Wein [:mcsmurf]]

Attachment: Patch for checkin

Comment 22

[Frank Wein [:mcsmurf]]

Pushed to comm-central: https://hg.mozilla.org/comm-central/rev/cddaf61b0b4b

Comment 23

[Frank Wein [:mcsmurf]]

Comment on attachment 733883 [details] [diff] [review]
Patch for checkin

[Approval Request Comment]
Regression caused by (bug #): Removal/Changes of Places and network API
User impact if declined: -
Testing completed (on m-c, etc.): Works on comm-central
Risk to taking this patch (and alternatives if risky): Test-only fix
String changes made by this patch: -

Comment 24

[Frank Wein [:mcsmurf]]

Pushed to comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/f8116483d7f3

Comment 25

[Frank Wein [:mcsmurf]]

Pushed to comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/b632ea457b4f