647881 - mochitest-browser-chrome: browser_dataman_basics.js and browser_dataman_callviews.js fail since 2011.04.02

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Serge Gautherie (:sgautherie)]

{
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | The correct number of domains is listed - Got 15, expected 14
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | In search, the correct number of domains is listed - Got 3, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | In search, the correct domains are listed - Got mochi.test,mozilla.com,mozilla.org, expected mochi.test,mozilla.org
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | After search, the correct number of domains is listed - Got 15, expected 14
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | The domain has been removed from the list - Got 14, expected 13
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | The domain for prefs tests has been added from the list - Got 15, expected 14
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | The domain for prefs tests has been removed from the list - Got 14, expected 13
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | For IDN tests, correct domain is selected - Got xn--exmple-cua.test, expected xn--hxajbheg2az3al.xn--jxalpdlp
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | The display title of that domain is correct - Got xn--exmple-cua.test, expected παράδειγμα.δοκιμή
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | Cookies panel is selected - Got permissionsPanel, expected cookiesPanel
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_basics.js | Found a tab after previous test timed out: about:blank

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_callviews.js | Step 7: The correct number of domains is listed - Got 5, expected 2
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_callviews.js | Step 7: The second domain is correct as well - Got example.org, expected getpersonas.com
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/suite/common/dataman/tests/browser_dataman_callviews.js | Step 8: The correct number of domains is listed - Got 13, expected 12
}

Regression timeframe: (See:)
http://build.mozillamessaging.com/tinderboxpushlog/?tree=SeaMonkey2.1&rev=3caccd684365
(m-2.0) Bug 628873 caused this.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Account for mozilla.com domain that bug 628873 added, Minor rewrites

This should be (most of) it:
I couldn't reproduce some of the reported failures, we'll see if/what remains.

Comment 2

[Justin Wood (:Callek)]

Comment on attachment 524957 [details] [diff] [review]
(Av1) Account for mozilla.com domain that bug 628873 added, Minor rewrites

ok, my skim looks like it should be good. I didn't look intent enough for a real review. I also think we should give Kairo a chance to review this as well, but only one review is needed.

Comment 3

[Robert Kaiser]

Comment on attachment 524957 [details] [diff] [review]
(Av1) Account for mozilla.com domain that bug 628873 added, Minor rewrites

I don't think I like the rewrite part of the patch as it is, it seems to move around things without much need. I need to look at this at a quiet hour. I will not r- this, but please wait for my review before doing anything with it.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> it seems to move around things without much need.

About test() line moves, it seems to me either more visible or safer that everything will happen in the expected order...
That said, it should be unrelated to this bug.

Comment 5

[Robert Kaiser]

Comment on attachment 524957 [details] [diff] [review]
(Av1) Account for mozilla.com domain that bug 628873 added, Minor rewrites

Review of attachment 524957 [details] [diff] [review]:

If you'e just fix the bug instead of doing a few other things at the same time, you'd get faster reviews, and they might even go r+ in the end...

::: suite/common/dataman/tests/browser_dataman_basics.js
@@ +34,5 @@
+//
+// Added domains:
+//  2 temporary domains: (See test() below.)
+//   drumbeat.org, geckoisgecko.org.
+//

That comment above overall is more UNclear to me than the short one I have in the original version inside the test() function, so r- for that change.

Just add mozilla.com to the list of things in there and everything is fine.

@@ +36,5 @@
+//  2 temporary domains: (See test() below.)
+//   drumbeat.org, geckoisgecko.org.
+//
+// 15 = 1 + (10 + 2) + 2.
+var gNumberOfDomains = 15;

Bad variable file name, but possibly useful idea.
I'd be more for having a gPreexistingDomains that is set to 10, and where it's being used, use an addition of that and whatever the difference.

@@ +42,5 @@
 function test() {
+  waitForExplicitFinish();
+
+  // Services.prefs.setBoolPref("data_manager.debug", true);
+

Don't move those around unless you have a good reason to state why you're doing that.

@@ -71,5 @@
-
-  gBrowser.addTab();
-  // Open the Data Manager, testing the menu item.
-  document.getElementById("tasksDataman").click();
-

Please leave those unchanged and where they are unless you have a good argument and/or it fixes this bug.

@@ -76,3 @@
   var testIndex = 0;
   var win;
-

Please leave that empty line in.

@@ +120,5 @@
 var testFuncs = [
 function test_open_state(aWin) {
   is(aWin.document.documentElement.id, "dataman-page",
      "The active tab is the Data Manager");
+  is(aWin.gDomains.tree.view.rowCount, gNumberOfDomains,

gPreexistingDomains + 5

@@ +130,5 @@
   aWin.document.getElementById("domainSearch").doCommand();
   is(aWin.gDomains.tree.view.selection.count, 0,
      "In search, non-matching selection is lost");
+  is(aWin.gDomains.tree.view.rowCount, 3,
+     "In search, the correct number of '*mo*' domains is listed");

Don't change the string, anyone who sees a failure there needs to look for what exactly is being searched anyhow and '*mo*' looks ugly.

@@ +135,4 @@
   is(aWin.gDomains.displayedDomains.map(function(aDom) { return aDom.title; })
                                    .join(","),
+     "mochi.test,mozilla.com,mozilla.org",
+     "In search, the correct '*mo*' domains are listed");

Same here, leave the error string unchanged.

@@ +140,4 @@
   aWin.gDomains.tree.view.selection.select(0);
   aWin.document.getElementById("domainSearch").value = "";
   aWin.document.getElementById("domainSearch").doCommand();
+  is(aWin.gDomains.tree.view.rowCount, gNumberOfDomains,

Again, gPreexistingDomains + 5

@@ +279,5 @@
      "After selecting, the select all context menu item is disabled");
   is(aWin.document.getElementById("cookies-context-remove").disabled, false,
      "After selecting, the remove context menu item is enabled");
 
+  // Remove drumbeat.org domain.

No need for a comment on that, it's clear by what the name of the clicked element is.

@@ +284,2 @@
   aWin.document.getElementById("cookies-context-remove").click();
+  gNumberOfDomains -= 1;

Gah, now _that_ is ugly.

@@ +284,3 @@
   aWin.document.getElementById("cookies-context-remove").click();
+  gNumberOfDomains -= 1;
+  is(aWin.gDomains.tree.view.rowCount, gNumberOfDomains,

gPreexistingDomains + 4

@@ +476,4 @@
   Services.contentPrefs.setPref("my.drumbeat.org", "data_manager.test", "foo");
   Services.contentPrefs.setPref("drumbeat.org", "data_manager.test", "bar");
+  gNumberOfDomains += 1;
+  is(aWin.gDomains.tree.view.rowCount, gNumberOfDomains,

Again, no need for adding a comment, and ugliness of changing that global var.
Just again go for gPreexistingDomains + 5 :)

@@ +533,2 @@
   aWin.document.getElementById("forgetButton").click();
+  gNumberOfDomains -= 1;

Again, leave out the comment and var manipulation.

@@ +542,5 @@
      "Forget tab is hidden again");
   is(aWin.document.getElementById("forgetTab").disabled, true,
      "Forget panel is disabled again");
 
+  is(aWin.gDomains.tree.view.rowCount, gNumberOfDomains,

gPreexistingDomains + 4

@@ +652,5 @@
   aWin.document.getElementById("domainSearch").value = "";
   aWin.document.getElementById("domainSearch").doCommand();
 
+  // Select the valid IDN domain: it is assumed to be listed last.
+  aWin.gDomains.tree.view.selection.select(gNumberOfDomains - 1);

No need to add the comment, but the general idea is good. Use
gPreexistingDomains + 3

::: suite/common/dataman/tests/browser_dataman_callviews.js
@@ +15,5 @@
 const DATAMAN_LOADED = "dataman-loaded";
+
+// See browser_dataman_basics.js.
+// 13 = 1 + (10 + 2).
+const kNumberOfDomains = 13;

Again, use preexitingDomains = 10 - though you're right, using a const instead of a var is probably a good idea.

@@ +18,5 @@
+// 13 = 1 + (10 + 2).
+const kNumberOfDomains = 13;
+// See test() below: drumbeat.org, getpersonas.com.
+const kNumberOfDomainsWithCookies = 2;
+

No need to do an own const for that, we're not changing it anyhow and no comment is needed, it's obvious.

@@ -27,5 @@
   var win;
-
-  gBrowser.addTab();
-  toDataManager("example.org");
-

Again, please don't change those around unless you have really good arguments and/or it fixes this bug.

@@ +133,5 @@
 
 function test_load_datatype(aWin) {
   is(aWin.gDomains.selectfield.value, "Cookies",
     "Step " + testIndex + ": The correct menulist item is selected");
+  is(aWin.gDomains.tree.view.rowCount, kNumberOfDomainsWithCookies,

Just leave that hardcoded, no need to touch that.

@@ +150,5 @@
 
 function test_switch_datatype(aWin) {
   is(aWin.gDomains.selectfield.value, "Permissions",
     "Step " + testIndex + ": The correct menulist item is selected");
+  is(aWin.gDomains.tree.view.rowCount, kNumberOfDomains,

preexitingDomains + 3

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)

To make it as short as possible: I'm sorry.

Comment 7

[Ian Neal]

Attachment: Simplified version of serge's patch v1.1 [Checked in: Comment 9]

Changed as per review comments (hopefully).

Comment 8

[Robert Kaiser]

Comment on attachment 528020 [details] [diff] [review]
Simplified version of serge's patch v1.1 [Checked in: Comment 9]

Review of attachment 528020 [details] [diff] [review]:

Yes, look right on the money to fix this bug here.

We can talk about any other improvements, but I'd love to have them in separate bugs/patches if possible. :)

::: suite/common/dataman/tests/browser_dataman_basics.js
@@ +29,5 @@
   // getpersonas.com and addons.mozilla.org to install addons as well as
   // permissions for a number of sites used in mochitest to load XUL/XBL.
   // For the latter, those domains are used/listed: 172.0.0.1, bank1.com,
   // bank2.com, example.com, example.org, mochi.test, test,
   // xn--exmple-cua.test, xn--hxajbheg2az3al.xn--jxalpdlp

Nit: please add mozilla.com to this list.

Comment 9

[Ian Neal]

Comment on attachment 528020 [details] [diff] [review]
Simplified version of serge's patch v1.1 [Checked in: Comment 9]

http://hg.mozilla.org/comm-central/rev/454703a53588
http://hg.mozilla.org/releases/comm-2.0/rev/ada1d86214e0

Comment 10

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey2.1/1303948443.1303951705.23223.gz
OS X 10.6 comm-2.0 debug test mochitest-other on 2011/04/27 16:54:03

V.Fixed