Last Comment Bug 702448 - Create some view source UI mochitests
: Create some view source UI mochitests
Status: RESOLVED FIXED
[qa-]
:
Product: Toolkit
Classification: Components
Component: View Source (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Geoff Lankow (:darktrojan)
:
Mentors:
Depends on: 728628
Blocks: 455888 469434 699356 700042 700260 703965 728655
  Show dependency treegraph
 
Reported: 2011-11-14 15:01 PST by Geoff Lankow (:darktrojan)
Modified: 2012-02-19 01:44 PST (History)
7 users (show)
geoff: in‑testsuite-
geoff: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
tests (9.93 KB, patch)
2011-11-15 13:54 PST, Geoff Lankow (:darktrojan)
no flags Details | Diff | Review
tests v2 (10.54 KB, patch)
2011-11-20 13:54 PST, Geoff Lankow (:darktrojan)
jaws: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Review

Description Geoff Lankow (:darktrojan) 2011-11-14 15:01:31 PST
I'm working on a few regression tests for the view source UI, since there are none, and it got broken a lot with the parser change.
Comment 1 Geoff Lankow (:darktrojan) 2011-11-15 13:54:29 PST
Created attachment 574687 [details] [diff] [review]
tests

This patch has tests for the window title (bug 699356, currently broken so the test is a known-fail), line wrap, syntax highlighting, and tab size prefs.

(I seem to be picking on you for reviews today Gavin, sorry!)
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2011-11-18 13:50:50 PST
Comment on attachment 574687 [details] [diff] [review]
tests

Review of attachment 574687 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks fine. I've pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=99d57a5d87e7

::: toolkit/components/viewsource/test/browser/browser_viewsourceprefs.js
@@ +13,5 @@
> +    mWindow = aWindow;
> +    wrapMenuItem = aWindow.document.getElementById('menu_wrapLongLines');
> +    syntaxMenuItem = aWindow.document.getElementById('menu_highlightSyntax');
> +
> +    isnot(wrapMenuItem.getAttribute("checked"), "true", "Wrap menu item not checked by default");

Is it possible to use |is(wrapMenuItem.hasAttribute("checked"), false, "Wrap menu item not checked by default");| here or do we set the |.checked| attribute to |"false"|, and if so, can we explicitly check for |"false"| instead of just checking for anything but |"true"|? According to the |simulateClick| function, we are removing the attribute, so we should just be able to use |hasAttribute|.

@@ +25,5 @@
> +
> +// Check that the Wrap Long Lines menu item works.
> +function test1() {
> +  simulateClick(wrapMenuItem);
> +  executeSoon(function() {

Is it possible to use an event listener for the |"click"| event here so we don't need the executeSoon?

@@ +35,5 @@
> +}
> +
> +function test2() {
> +  simulateClick(wrapMenuItem);
> +  executeSoon(function() {

Same question here about the possible use of a |"click"| event listener.

@@ +46,5 @@
> +
> +// Check that the Syntax Highlighting menu item works.
> +function test3() {
> +  mWindow.gBrowser.addEventListener("pageshow", function() {
> +    mWindow.gBrowser.removeEventListener("pageshow", arguments.callee, false);

Can you provide a name to the callee function so we don't have to depend on arguments.callee? According to this page, arguments.callee is deprecated in ES5 strict mode: https://developer.mozilla.org/en/JavaScript/Reference/Functions_and_function_scope/arguments/callee

@@ +47,5 @@
> +// Check that the Syntax Highlighting menu item works.
> +function test3() {
> +  mWindow.gBrowser.addEventListener("pageshow", function() {
> +    mWindow.gBrowser.removeEventListener("pageshow", arguments.callee, false);
> +    isnot(syntaxMenuItem.getAttribute("checked"), "true", "Syntax menu item unchecked");

Can we check for |hasAttribute| here as well?

@@ +59,5 @@
> +}
> +
> +function test4() {
> +  mWindow.gBrowser.addEventListener("pageshow", function() {
> +    mWindow.gBrowser.removeEventListener("pageshow", arguments.callee, false);

Same question about arguments.callee here.

@@ +80,5 @@
> +    ["view_source.wrap_long_lines", true],
> +    ["view_source.syntax_highlight", false]
> +  ]}, function() {
> +    openViewSourceWindow(source, function(aWindow) {
> +      isnot(wrapMenuItem.getAttribute("checked"), "true", "Wrap menu item unchecked");

Same question about |hasAttribute| here.

::: toolkit/components/viewsource/test/browser/head.js
@@ +6,5 @@
> +  let viewSourceWindow = openDialog("chrome://global/content/viewSource.xul", null, null, aURI);
> +  let gBrowser;
> +  waitForFocus(function() {
> +    gBrowser = viewSourceWindow.gBrowser;
> +    executeSoon(function() {

Is it possible to use the |"load"| event listener all the time and remove the need for executeSoon here?
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2011-11-18 15:27:52 PST
Comment on attachment 574687 [details] [diff] [review]
tests

Review of attachment 574687 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/viewsource/test/browser/head.js
@@ +4,5 @@
> +
> +function openViewSourceWindow(aURI, aCallback) {
> +  let viewSourceWindow = openDialog("chrome://global/content/viewSource.xul", null, null, aURI);
> +  let gBrowser;
> +  waitForFocus(function() {

Could you add a "load" event listener to viewSourceWindow and when the event fires just check the target to know if the gBrowser.contentWindow has loaded instead of doing the waitForFocus, etc?
Comment 4 Geoff Lankow (:darktrojan) 2011-11-20 13:54:33 PST
Created attachment 575773 [details] [diff] [review]
tests v2

I've managed to clean up most of the stuff I put in as an attempt to just make the damn thing work. The checked attribute is sometimes false for a variety of reasons (one of which I can only explain as "a Mac thing"). I've checked for it and removed it, then used hasAttribute.
Comment 5 Geoff Lankow (:darktrojan) 2011-11-20 14:00:38 PST
Oh, and try is all good. https://tbpl.mozilla.org/?tree=Try&rev=63163396023d
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2011-11-21 14:33:50 PST
Comment on attachment 575773 [details] [diff] [review]
tests v2

Looks good! Thanks for adding more tests :)
Comment 7 Geoff Lankow (:darktrojan) 2011-11-21 15:26:11 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/def93a49ec50
Comment 8 Geoff Lankow (:darktrojan) 2011-11-22 03:03:38 PST
Comment on attachment 575773 [details] [diff] [review]
tests v2

I think we'll want this on Aurora, so that the regressions from bug 482921 can land tests there. I've put it through Try on top of Aurora and there were no problems.
Comment 9 Ed Morley [:emorley] 2011-11-22 09:09:30 PST
https://hg.mozilla.org/mozilla-central/rev/def93a49ec50
Comment 10 Henri Sivonen (:hsivonen) 2011-11-25 06:30:31 PST
I have a fix for bug 699356 and can see visually that the window title updates correctly. However, when openViewSourceWindow() calls the callback, aWindow.document.documentElement.getAttribute("title") returns "Nightly". Did the the window title test pass with the old View Source?
Comment 11 Geoff Lankow (:darktrojan) 2011-11-25 14:14:51 PST
No, the attribute doesn't get updated. Only the first of those tests will pass by fixing bug 699356. I may file and fix the other bug if I can work out how. I should've mentioned it, sorry.
Comment 12 christian 2011-11-28 13:18:51 PST
Comment on attachment 575773 [details] [diff] [review]
tests v2

[triage comment]
Approved for tests.
Comment 13 Geoff Lankow (:darktrojan) 2011-11-28 13:55:09 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/69482d1fd036
Comment 14 Ed Morley [:emorley] 2011-12-03 13:30:57 PST
toolkit/components/viewsource/test/browser/Makefile.in was created in the tree by this bug, but was not listed in toolkit/toolkit-makefiles.sh, so I've included it here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e8e969fa09
Comment 15 Jared Wein [:jaws] (please needinfo? me) 2011-12-03 14:48:47 PST
(In reply to Ed Morley [:edmorley] from comment #14)
> toolkit/components/viewsource/test/browser/Makefile.in was created in the
> tree by this bug, but was not listed in toolkit/toolkit-makefiles.sh, so
> I've included it here:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/17e8e969fa09

Seems like we would want this change also pushed to Aurora, right?
Comment 16 Ed Morley [:emorley] 2011-12-03 15:12:38 PST
That shouldn't be necessary, since it only helps to speed up makefile creation & won't (at least in this case) cause anything to break :-)
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 13:55:58 PST
Marking qa- as this is not something QA needs to verify. Please set to qa+ if this is not the case.

Note You need to log in before you can comment on or make changes to this bug.