Closed Bug 446281 Opened 13 years ago Closed 13 years ago

automated tests for duplicate ids

Categories

(SeaMonkey :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: mnyromyr, Assigned: mnyromyr)

References

(Blocks 4 open bugs)

Details

Attachments

(5 files, 2 obsolete files)

As bug 446027 showed, it's rather likely to inadvertently add double ids in the preferences. We should have a chrome(?) test for that.
Flags: blocking-seamonkey2?
creating tests is nice but does not block a release.
Sure, those tests are wanted and could help a lot, but we _can_ release without them.
Flags: blocking-seamonkey2? → blocking-seamonkey2-
Assignee: nobody → mnyromyr
Component: Preferences → General
Summary: automated tests for preferences → automated tests for duplicate ids
Assignee: mnyromyr → general
QA Contact: prefs → general
Attached patch duplicate id tests (obsolete) — Splinter Review
Test preferences and some other windows for duplicate ids.
MailNews windows don't work yet, because no accounts are defined.
Addressbook should work, but doesn't because of bug 37919.
Chatzilla/Inspector/Venkman aren't available during chrome tests (yet?).
Assignee: general → mnyromyr
Status: NEW → ASSIGNED
Attachment #331178 - Flags: review?(bugzilla)
Comment on attachment 331178 [details] [diff] [review]
duplicate id tests

+	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)

You now need to change $(DEPTH)/_tests to $(DEPTH)/mozilla/_tests because of the hg migration

+    // Have all loaded windows been closed again?
+    function AllWindowsClosed()
+    {
+      for each (var e in window.gLoadedWindows)
+        return false;
+      return true;
+    }

Either this is wrong, or you could do return window.gLoadedWindows;

preferences, navigator and editor all fail, but I guess you could check this in with just the console test enabled.
Attachment #331178 - Flags: review?(bugzilla) → review+
(In reply to comment #3)
> (From update of attachment 331178 [details] [diff] [review])
> +       $(INSTALL) $(foreach f,$^,"$f")
> $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)
> 
> You now need to change $(DEPTH)/_tests to $(DEPTH)/mozilla/_tests because of
> the hg migration

Actually, pleas use $(MOZDEPTH)/_tests as that variable always points to the mozilla dir even if we might consider renaming it one time.
Before I can check this in, I'll post a couple of patches to avoid turning the tree orange right away... ;-)
Fix up the easy cases (printButton and bundlePreferences), plus collateral cleanup.
Attachment #333469 - Flags: review?(iann_bugzilla)
Attachment #333469 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 333469 [details] [diff] [review]
double id cleanup, part 1: preferences #1 [checked in]

Landed on trunk.
Attachment #333469 - Attachment description: double id cleanup, part 1 → double id cleanup, part 1 (checked in)
The key ids are never actually used.
I checked the sidebar element removal with the sidebar in browser, composer and mail editor.
Attachment #333648 - Flags: review?(iann_bugzilla)
Comment on attachment 333648 [details] [diff] [review]
double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul]

Second pair of knowing sidebar eyes needed. ;-)
Attachment #333648 - Flags: review?(philip.chee)
Comment on attachment 333648 [details] [diff] [review]
double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul]

>-    <popupset id="contentAreaContextSet"/>
>+    <!--popupset id="contentAreaContextSet"/-->

Assume that the + line does not exist, only the - one, so that the element gets removed completely.
Comment on attachment 333648 [details] [diff] [review]
double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul]

me=r+ (first time, hope I've done this correctly)

>>-    <popupset id="contentAreaContextSet"/>
>>+    <!--popupset id="contentAreaContextSet"/-->
>
>Assume that the + line does not exist, only the - one, so that the element gets
>removed completely.

while grepping through comm-central I noticed that debugQATextEditorShell.xul pulls in contentAreaContextOverlay.xul for no apparent reason, but that's grist for another bug.
Attachment #333648 - Flags: review?(philip.chee) → review+
This fixes the doubled IDs in EditorContextMenuOverlay.xul and editor, accidentally also fixing the wrong application of acceltexts to the "Select Row" context menu instead of to the menu menu (see <http://mxr.mozilla.org/seamonkey/source/editor/ui/composer/content/editor.js#638>).
Attachment #333813 - Flags: superreview?(neil)
Attachment #333813 - Flags: review?(stefanh)
Attachment #333469 - Attachment description: double id cleanup, part 1 (checked in) → double id cleanup, part 1: preferences #1 (checked in)
Comment on attachment 333648 [details] [diff] [review]
double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul]

>-    <popupset id="contentAreaContextSet"/>
>+    <!--popupset id="contentAreaContextSet"/-->
The problem here as I recall is that not all windows with a sidebar have a content area; specifically Editor and Address Book don't normally require the contentAreaContextSet so for the sidebar to be able to use the content area context menu it needed to provide an overlay point for it.
Comment on attachment 333813 [details] [diff] [review]
double id cleanup, part 3: editor window [checked in]

I wonder why these are still observes= instead of command= (I didn't try to see whether command= works or not).
Attachment #333813 - Flags: superreview?(neil) → superreview+
Attached patch duplicate id tests, v2 (obsolete) — Splinter Review
- switched to $(MOZDEPTH)
- fixed a reentrancy problem in the paneload mechanism, causing certain overlays to be applied twice (they were too fast for the core to bark!)
- had specialcase the character_encoding_pane, because (only?) RDF templates create fancy ids
- typo

Hence rerequesting review.
Attachment #331178 - Attachment is obsolete: true
Attachment #334048 - Flags: review?
Attachment #334048 - Flags: review? → review?(bugzilla)
One last double id in preferences.
Attachment #334051 - Flags: review?(iann_bugzilla)
(In reply to comment #13)
> The problem here as I recall is that not all windows with a sidebar have a
> content area; specifically Editor

Neither mail editor nor Composer show any regressions as far as I can tell. What am I missing?

> and Address Book

But Address Book doesn't even have a sidebar at all?!
Attachment #333648 - Flags: review?(iann_bugzilla) → review+
Attachment #334051 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 334051 [details] [diff] [review]
double id cleanup, part 4: preferences #2 [checked in]

Landed on trunk.
Attachment #334051 - Attachment description: double id cleanup, part 4: preferences #2 → double id cleanup, part 4: preferences #2 [checked in]
Attachment #333469 - Attachment description: double id cleanup, part 1: preferences #1 (checked in) → double id cleanup, part 1: preferences #1 [checked in]
Comment on attachment 333648 [details] [diff] [review]
double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul]

>-    <popupset id="contentAreaContextSet"/>
>+    <!--popupset id="contentAreaContextSet"/-->

This breaks the context menu in sidebar's webpanels, e.g. the Tinderbox panel.

Since there's no easy way to say "only add the element with this id if the id isn't used elsewhere in this window already"¹, I'll specialcase this for the browser window in the test itself. :-|

I landed this on trunk, but *WITHOUT* the changes to suite/common/sidebar/sidebarOverlay.xul!


¹ Using the removeelement attribute doesn't help; killing the second id instance via JS either doesn't see the outer element yet or may run after the test...
Attachment #333648 - Attachment description: double id cleanup, part 2: browser window → double id cleanup, part 2: browser window [checked in without sidebarOverlay.xul]
Allow ignoring certain ids, and do so for contentAreaContextSet when checking navigator.xul.
Attachment #334048 - Attachment is obsolete: true
Attachment #334185 - Flags: review?(bugzilla)
Attachment #334048 - Flags: review?(bugzilla)
Comment on attachment 333813 [details] [diff] [review]
double id cleanup, part 3: editor window [checked in]

diff --git a/editor/ui/composer/content/editor.xul b/editor/ui/composer/content/editor.xul
--- a/editor/ui/composer/content/editor.xul
+++ b/editor/ui/composer/content/editor.xul
@@ -154,7 +154,7 @@
       <menuitem id="viewAllTagsMode"/>
       <menuitem id="viewSourceMode"/>
       <menuitem id="viewPreviewMode"/>
-      <menuseparator  id="viewSep1"/>
+      <menuseparator id="viewSep2"/>
       <menu id = "composerCharsetMenu" />
     </menupopup>
     </menu>

I haven't tested this, but it looks like you'll need to update http://mxr.mozilla.org/comm-central/source/mozilla/editor/ui/composer/content/editor.js#453
(In reply to comment #21)
> I haven't tested this, but it looks like you'll need to update
> http://mxr.mozilla.org/comm-central/source/mozilla/editor/ui/composer/content/editor.js#453

No, I don't need to do that.
But I ghad to kill my mozilla-central clone and accidently killed my diff defaults, hence the -U3. The patch touches this:

152       <menuseparator id="viewSep1"/>
153       <menuitem id="viewNormalMode" checked="true"/>
154       <menuitem id="viewAllTagsMode"/>
155       <menuitem id="viewSourceMode"/>
156       <menuitem id="viewPreviewMode"/>
157       <menuseparator  id="viewSep1"/>
158       <menu id = "composerCharsetMenu" />
Comment on attachment 333813 [details] [diff] [review]
double id cleanup, part 3: editor window [checked in]

OK, looks good then.
Attachment #333813 - Flags: review?(stefanh) → review+
Comment on attachment 333813 [details] [diff] [review]
double id cleanup, part 3: editor window [checked in]

Landed on trunk.
Attachment #333813 - Attachment description: double id cleanup, part 3: editor window → double id cleanup, part 3: editor window [checked in]
Attachment #334185 - Flags: review?(bugzilla) → review+
Comment on attachment 334185 [details] [diff] [review]
duplicate id tests, v3 [checked in]

Landed on trunk.
Attachment #334185 - Attachment description: duplicate id tests, v3 → duplicate id tests, v3 [checked in]
Blocks: 451952
I filed bug 451952 for the mailnews main window tests I had to comment out.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → seamonkey2.0alpha
(In reply to comment #11)
> I noticed that debugQATextEditorShell.xul pulls in contentAreaContextOverlay.xul for no apparent reason

I filed bug 451959.

(In reply to comment #14)
> I wonder why these are still observes= instead of command=

I filed bug 451960.
Blocks: 455678
Blocks: 474716
You need to log in before you can comment on or make changes to this bug.