Remove usage of setPageTitle and isVisited from Seamonkey's tests

RESOLVED FIXED in seamonkey2.20

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mak, Assigned: mcsmurf)

Tracking

unspecified
seamonkey2.20
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.18 fixed, seamonkey2.19 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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

Updated

6 years ago
Summary: Remove usage of setPageTitle and isVisited from Semonkey's tests → Remove usage of setPageTitle and isVisited from Seamonkey's tests
We are quite near removal of the APIs on central, is any SM contributor interested in working on this bug?
(Assignee)

Comment 2

6 years ago
Yes, me :) just waited for bug 739217 to be resolved.
Cool, thanks. If you need any help just ping me in #developers.
See also bug 820797.
any news here? we are about to proceed with the removal in central.
(Assignee)

Comment 5

6 years ago
Created attachment 722296 [details] [diff] [review]
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.
(Assignee)

Comment 6

6 years ago
Created attachment 722298 [details] [diff] [review]
First attempt at patch
Attachment #722296 - Attachment is obsolete: true
(Assignee)

Comment 7

6 years ago
Created attachment 722723 [details] [diff] [review]
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).
Attachment #722298 - Attachment is obsolete: true
Attachment #722723 - Flags: feedback?(mak77)
(Assignee)

Comment 8

6 years ago
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?
(Assignee)

Comment 9

6 years ago
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
(Reporter)

Updated

6 years ago
Assignee: nobody → bugzilla
OS: Windows 7 → All
Hardware: x86_64 → All
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
Attachment #722723 - Flags: feedback?(mak77) → feedback+
(Reporter)

Updated

6 years ago
Depends on: 838874
(Assignee)

Comment 11

6 years ago
Created attachment 727763 [details] [diff] [review]
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()).
Attachment #722723 - Attachment is obsolete: true
Attachment #727763 - Flags: review?(neil)
(Assignee)

Comment 12

6 years ago
BTW: The promiseAsyncUpdates and promiseAddVisits functions inside head.js have been copied from FF test code.
(Assignee)

Comment 13

6 years ago
Created attachment 727777 [details] [diff] [review]
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).
Attachment #727763 - Attachment is obsolete: true
Attachment #727763 - Flags: review?(neil)
Attachment #727777 - Flags: review?(neil)
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.
(Assignee)

Comment 15

6 years ago
Created attachment 731124 [details] [diff] [review]
Patch

(In reply to neil@parkwaycc.co.uk 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.
Attachment #727777 - Attachment is obsolete: true
Attachment #727777 - Flags: review?(neil)
Attachment #731124 - Flags: review?(neil)
(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.
(Assignee)

Comment 17

6 years ago
Created attachment 731304 [details] [diff] [review]
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).
Attachment #731124 - Attachment is obsolete: true
Attachment #731124 - Flags: review?(neil)
Attachment #731304 - Flags: review?(neil)
(Assignee)

Comment 18

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

Updated

6 years ago
Attachment #731304 - Flags: review?(neil) → review+
I just noticed your new files have DOS line endings. Please fix before checking in!
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.
(Assignee)

Comment 21

5 years ago
Created attachment 733883 [details] [diff] [review]
Patch for checkin
Attachment #731304 - Attachment is obsolete: true
Attachment #733883 - Flags: review+
(Assignee)

Comment 23

5 years ago
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: -
Attachment #733883 - Flags: approval-comm-beta?
Attachment #733883 - Flags: approval-comm-aurora?

Updated

5 years ago
Target Milestone: --- → seamonkey2.20

Updated

5 years ago
Attachment #733883 - Flags: approval-comm-beta?
Attachment #733883 - Flags: approval-comm-beta+
Attachment #733883 - Flags: approval-comm-aurora?
Attachment #733883 - Flags: approval-comm-aurora+
(Assignee)

Comment 24

5 years ago
Pushed to comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/f8116483d7f3
status-seamonkey2.19: --- → fixed
(Assignee)

Comment 25

5 years ago
Pushed to comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/b632ea457b4f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-seamonkey2.18: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.