461710 - Write an automated test to ensure that visited link coloring is turned off in private browsing mode

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[(no longer active)]

Marcia has specified this as one of the pieces for the QA test plan of Private Browsing.  This can, however, be written as a Mochitest, and I think we should do it.  Further Litmus tests may not even be necessary for this, but I'll leave this decision to Marcia.

I'll post the mochitest here.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

The mochitest tries to add a visit in the places DB by a natural page load, and verifies that visited link coloring works outside of the private mode, but doesn't inside of the private mode.

Comment 2

[(no longer active)]

Attachment: Patch (v1.1)

Changed the test to use the keep_current_session pref instead of the private-browsing-enter notification which has been removed in the latest PB patch.

Comment 3

[(no longer active)]

Dietrich: ping?

Comment 4

[(no longer active)]

Attachment: Patch (v1.2)

I found a timing issue in the previous version of this patch which made the test fail if it ran too quickly.  This should fix the issue.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 345955 [details] [diff] [review]
Patch (v1.2)

>diff --git a/toolkit/components/places/tests/Makefile.in b/toolkit/components/places/tests/Makefile.in
>--- a/toolkit/components/places/tests/Makefile.in
>+++ b/toolkit/components/places/tests/Makefile.in
>@@ -53,24 +53,26 @@ XPCSHELL_TESTS = \
>                  bookmarks \
>                  queries \
>                  unit \
>                  $(NULL)
> 
> # Simple MochiTests
> MOCHI_TESTS = mochitest/test_bug_405924.html \
> 							mochitest/test_bug_411966.html \
>+							mochitest/test_bug_461710.html \
> 							$(NULL)
> 
> MOCHI_CONTENT = mochitest/prompt_common.js \
> 								$(NULL)
> 

fix tab indent here

>+
>+  ok(true, "Starting test #" + testNum);

remove

>+var _PBSvc = null;
>+function get_PBSvc() {
>+  if (_PBSvc)
>+    return _PBSvc;
>+
>+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+  try {
>+    _PBSvc = Cc["@mozilla.org/privatebrowsing;1"].
>+             getService(Ci.nsIPrivateBrowsingService);
>+    return _PBSvc;
>+  } catch (e) {}
>+  return null;
>+}

don't you want to test for failure to get the service?

>+
>+
>+var ignoreLoad = false;
>+function handleLoad(aEvent) {
>+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+
>+  ok(true, "Processing subtest #" + testNum);

remove

>+
>+  checkTest();
>+
>+  if (testNum < subtests.length) {
>+    setTimeout(function() {
>+      loadNextTest();
>+    }, 500);
>+  } else {
>+    ok(true, "private browsing visited coloring test finished.");

remove

>+    prefBranch.clearUserPref("browser.privatebrowsing.keep_current_session");
>+    SimpleTest.finish();
>+  }
>+}
>+
>+
>+var pb = get_PBSvc();
>+if (!pb) { // Private Browsing might not be available
>+  ok(true, "Private browsing service is not available");

not a test, and why is it ok? shouldn't we fail in this case?

Comment 6

[(no longer active)]

Attachment: Patch (v1.3)

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 345992 [details] [diff] [review]
Patch (v1.3)

r=me thanks!

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/087fcb57028c

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

When I run the all the mochitests, I get one error in the testcase - reproduced several times. Running only the test or only tests/toolkit/components or similar
doesn't cause the problem.
I don't know which error it is. I guess I could log all the stdout messages and 
search for the error.

Comment 10

[(no longer active)]

(In reply to comment #9)
> When I run the all the mochitests, I get one error in the testcase - reproduced
> several times. Running only the test or only tests/toolkit/components or
> similar
> doesn't cause the problem.
> I don't know which error it is. I guess I could log all the stdout messages and 
> search for the error.

This might be a timing issue I suspect, but it would be great if you can report which test is failing, and also which platform you're on, and any other details you feel might be important.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

After updating I don't get the problem anymore. Something was backed out, I think.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Now I got the problem again :(

Comment 13

[Justin Dolske [:Dolske]]

This is happening on the Tinderboxes too, just had a random failure:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1225940222.1225943866.25049.gz

Comment 14

[(no longer active)]

Attachment: Followup fix

I have thought of another more reliable way to fix the random failure issue.  Instead of loading each page with 500ms delay in the hope that everything happens correctly, let's split the visited and link pages to three separate files.

This way, the test would only fail if what we do in IsVisited is not correct, not because of some random timing issues.  With this patch, I was able to safely get rid of the timeout as well.

Comment 15

[(no longer active)]

REOPENING because the whole purpose of this bug was to write a test which passes.  :-)

Comment 16

[(no longer active)]

Dietrich: ping?

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 346664 [details] [diff] [review]
Followup fix

r=me

Comment 18

[(no longer active)]

Test fix checked in: http://hg.mozilla.org/mozilla-central/rev/1899ea13f9e7

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Add |ok(true, ...);|

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1a) Add |ok(true, ...);| [Checkin: Comment 24 & 25]

Cv1, without the extraneous change :-<

Comment 21

[Dietrich Ayala (:dietrich)]

Comment on attachment 369617 [details] [diff] [review]
(Cv1a) Add |ok(true, ...);|
[Checkin: Comment 24 & 25]

what's the purpose of this?

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> what's the purpose of this?

See bug 483633 comment 3.

Comment 23

[Dietrich Ayala (:dietrich)]

Comment on attachment 369617 [details] [diff] [review]
(Cv1a) Add |ok(true, ...);|
[Checkin: Comment 24 & 25]

r=me

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 369617 [details] [diff] [review]
(Cv1a) Add |ok(true, ...);|
[Checkin: Comment 24 & 25]


http://hg.mozilla.org/mozilla-central/rev/83e98dcd8439

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 369617 [details] [diff] [review]
(Cv1a) Add |ok(true, ...);|
[Checkin: Comment 24 & 25]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/074a174d86dd