Closed Bug 647881 Opened 9 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: Passwords & Permissions, defect, major)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
seamonkey2.1final

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, Whiteboard: [perma-orange])

Attachments

(1 file, 1 obsolete file)

{
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.
This should be (most of) it:
I couldn't reproduce some of the reported failures, we'll see if/what remains.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #524957 - Flags: review?(iann_bugzilla)
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.
Attachment #524957 - Flags: review?(kairo)
Attachment #524957 - Flags: feedback+
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.
Attachment #524957 - Flags: review?(iann_bugzilla)
(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 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
Attachment #524957 - Flags: review?(kairo) → review-
(In reply to comment #5)

To make it as short as possible: I'm sorry.
Assignee: sgautherie.bz → nobody
Status: ASSIGNED → NEW
Keywords: helpwanted
Whiteboard: [perma-orange] → [patchlove] [perma-orange]
Changed as per review comments (hopefully).
Assignee: nobody → iann_bugzilla
Attachment #524957 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #528020 - Flags: review?(kairo)
Keywords: helpwanted
Whiteboard: [patchlove] [perma-orange] → [perma-orange]
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.
Attachment #528020 - Flags: review?(kairo) → review+
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
Attachment #528020 - Attachment description: Simplified version of serge's patch v1.1 → Simplified version of serge's patch v1.1 [Checked in: Comment 6]
Assignee: iann_bugzilla → sgautherie.bz
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1final
Attachment #528020 - Attachment description: Simplified version of serge's patch v1.1 [Checked in: Comment 6] → Simplified version of serge's patch v1.1 [Checked in: Comment 9]
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.