1037416 - Add mozmill test for Developer Tools keyboard shortcuts

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Add automated test for shortcuts related to Developer Tools interactions:

* Ctrl + Shift + Y - Downloads
* Ctrl + Shift + A - Add-ons
* F12, Ctrl + Shift + I - Toggle Developer Tools
* Ctrl + Shift + K - Open Web Console
* Ctrl + Shift + C - Open Inspector
* Ctrl + Shift + S - Toggle Debugger
* Shift + F7 - Toggle Style Editor 	
* Shift + F5 - Toggle Profiler 	
* Ctrl + Shift + Q - Toggle Network tools
* Shift + F2 - Toggle Developer Toolbar 
* Ctrl + Shift + M - Toggle Responsive Design View
* Shift + F4 - Open Scratchpad
* Ctrl + U - Open Page Source
* Ctrl + Shift + J - Open Browser Console
* Ctrl + I - Open Page Info

(Source: https://support.mozilla.org/en-US/kb/keyboard-shortcuts-perform-firefox-tasks-quickly#w_tools)

Comment 1

[Teodor Druta]

Attachment: b1037416.patch

This patch will add a new test for testing developer tools shortcut keys.

Comment 2

[Andrei Eftimie]

Comment on attachment 8507755 [details] [diff] [review]
b1037416.patch

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

Please check the test across all paltforms (win, osx, linux).
At least on OSX I see several failures (I think there are differences in the keyboard shortcuts used across different platforms).

Also check a couple locales to see if there are any obvious problems (check some locales we run against Aurora).

::: firefox/tests/functional/testKeyboardShortcuts/testDeveloperTools.js
@@ +78,5 @@
> +                                      "main-window");
> +  aModule.mainWindow.waitForElement();
> +
> +  // store close window key value to a variable for global scope use
> +  aModule.closeWindowKey = utils.getEntity(DTDS, "closeWindow.key");

You can store this in setupModule since it won't change between tests.

@@ +100,5 @@
> +  // trigger key press [ctrl/cmd + shift + y/j] for downloads
> +  mainWindow.keypress(downloadsTabKey, {accelKey: true, shiftKey: true});
> +
> +  // handle library window with the Downloads tab selected
> +  windows.handleWindow("type", PROPERTIES.library.type, function(aController) {

We might be able to make use of the places-organizer library that will land soon in bug 996530

@@ +123,5 @@
> +
> +/*
> + * Test Add-ons page shortcut
> + */
> +function testAddonsShortcut() {

For this we can actually use the addons library. Use the open method with type: "shortcut"

@@ +153,5 @@
> +  // [ctrl/cmd + shift + i] test case
> +  var devToolsKey = utils.getEntity(DTDS, "devToolboxMenuItem.keytext");
> +
> +  // trigger key press [ctrl/cmd + shift + i]
> +  mainWindow.keypress(devToolsKey, {accelKey: true, shiftKey: true});

cmd + shift +i opens DOM Inspector not DeveloperTools. F12 works.
The original requirements might be wrong, please check if there's another shortcut.

@@ +164,5 @@
> +  mainWindow.keypress(devToolsKey, {accelKey: true, shiftKey: true});
> +  // END [ctrl/cmd + shift + i] test case
> +
> +  // [F12] test case
> +  // there is a conflicting issue between the closing/opening dev tools

What kind of conflict? What happens here?
F12 should work on all pages.

@@ +248,5 @@
> +
> +/*
> + * Test Style Editor shortcut
> + */
> +function testStyleEditorShortcut() {

We can have all these Developer Toolbar Tabs as 1 function that iterates through them as all checks are identical.
Just specify all you need in the PROPERTIES constant (also group them in an array or similar).

@@ +450,5 @@
> +    expect.waitFor(() => utils.isDisplayed(aController, browserConsoleWin),
> +                   "Browser console window root element is displayed.");
> +
> +    // close window
> +    browserConsoleWin.keypress(closeWindowKey, {accelKey: true, shiftKey: true});

This should be only accel + key (cmd/ctrl + w)
-- [cmd/ctrl + shift + w] doesn't work (at least on OSX)

@@ +506,5 @@
> +  devToolsWebConsole.waitForElement();
> +
> +  // check if dev tools is displayed
> +  expect.waitFor(() => utils.isDisplayed(aController, devToolsWebConsole),
> +                 "Developer tools is displayed with the web console selected.");

I don't think you need both waitForElement and this waitFor call. You can probably remove the first one.

Comment 3

[Teodor Druta]

Attachment: devkeystest.patch

Since patches for Places Organizer and Page Info have landed on default, I refactored the patch for this new test, all the functions have been rewritten.

Comment 4

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8537702 [details] [diff] [review]
devkeystest.patch

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

This test contains a lot of duplicate or similar code. I suggest you create a more general function(s) which can be used for all shortcuts and use it in an iteration across all shortcuts. This way the test would be shorter and easier to follow.

::: firefox/tests/functional/testKeyboardShortcuts/testDeveloperTools.js
@@ +58,5 @@
> +                                       "chrome://browser/locale/devtools/webconsole.properties"
> +                                      ];
> +}
> +
> +function setupTest(aModule) {

I'm not sure why you need to do this foe every test, isn't it enough if you do it in the setupModule?

@@ +90,5 @@
> +
> +  win.close();
> +}
> +
> +/*

nit: /**

@@ +102,5 @@
> +
> +  addonsManager.open({type: "shortcut"});
> +
> +  // Check if the number of opened tabs is correct
> +  expect.equal((browserWindow.tabs.length - 1), initTabsLength,

I'd suggest to compare browser.tabs.length with initTabsLength + 1 to be clearer

@@ +185,5 @@
> +                                        "debuggerPopupset");
> +  debuggerPopupset.waitForElement();
> +  // Check for matching base URIs
> +  expect.equal(PROPERTIES.debugger.URI, debuggerPopupset.getNode().baseURI,
> +               "Inspector URI match");

nit: "Debugger URI matches"

Comment 5

[Teodor Druta]

Attachment: devkeystestv3.patch

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #4)
> Comment on attachment 8537702 [details] [diff] [review]
> devkeystest.patch
> 
> Review of attachment 8537702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This test contains a lot of duplicate or similar code. I suggest you create
> a more general function(s) which can be used for all shortcuts and use it in
> an iteration across all shortcuts. This way the test would be shorter and
> easier to follow.
> 

I don't see any duplicate code, except for the loadDevTools and closeDevTools functions, added a new function for that code.

> ::: firefox/tests/functional/testKeyboardShortcuts/testDeveloperTools.js
> @@ +58,5 @@
> > +                                       "chrome://browser/locale/devtools/webconsole.properties"
> > +                                      ];
> > +}
> > +
> > +function setupTest(aModule) {
> 
> I'm not sure why you need to do this foe every test, isn't it enough if you
> do it in the setupModule?
> 

Good point. I moved the mainWindow element declaration to the setupModule, and left the waitForElement in the setupTest, it should reside there.

> @@ +90,5 @@
> > +
> > +  win.close();
> > +}
> > +
> > +/*
> 
> nit: /**
> 

Updated everywhere it applied

> @@ +102,5 @@
> > +
> > +  addonsManager.open({type: "shortcut"});
> > +
> > +  // Check if the number of opened tabs is correct
> > +  expect.equal((browserWindow.tabs.length - 1), initTabsLength,
> 
> I'd suggest to compare browser.tabs.length with initTabsLength + 1 to be
> clearer
> 

Updated.

Thanks for the review, Mihaela !

Comment 6

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8545895 [details] [diff] [review]
devkeystestv3.patch

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

Please replace let with var.

I'd go with separate tests for each tool instead of this big one. Thinking of readability, of failure in one function which will make us skip the whole test for just one tool that has a regression or something. Henrik, what do you think?

Also, elements are being taken in the test itself, instead of a lib, but I don't think we'll have more tests for this to be worth writing a library. At the moment there are not duplicated. Still, if we separate in multiple tests, some helper methods like open and close could be moved.

::: firefox/tests/functional/testKeyboardShortcuts/testDeveloperTools.js
@@ +115,5 @@
> +
> +/**
> + * Test Developer tools shortcut
> + */
> +function testDeveloperToolsShortcut() {

This function in particular is hard to read, maybe we could change the comments.. see below.

@@ +121,5 @@
> +  var helperKey = mozmill.isMac ? "altKey" : "shiftKey";
> +  var keyShortcut = {accelKey: true};
> +  keyShortcut[helperKey] = true;
> +
> +  // [ctrl/cmd + shift + i] test case

So here it starts. Maybe have the comments with /* to pop better?

@@ +135,5 @@
> +  handleDevToolsWebConsole(browserWindow.controller);
> +
> +  closeDevTools(browserWindow.controller);
> +  browserWindow.tabs.closeAllTabs();
> +  // END [ctrl/cmd + shift + i] test case

That will get removed with the suggestion above, I think with the comment for the next test case it will be more visible that this one ended.

@@ +379,5 @@
> + *
> + * @param {MozMillController} aController
> + *        MozMillController of the window to operate on
> + */
> +function closeDevTools(aController) {

If we separate this in tests, it will need to be moved in a lib.

@@ +395,5 @@
> + *
> + * @param {MozMillController} aController
> + *        MozMillController of the window to operate on
> + */
> +function handleDevToolsWebConsole(aController) {

Same here.

@@ +418,5 @@
> + *        MozMillController of the window to operate on
> + * @param {function} aCallback
> + *        Callback to use for loading/closing the developer tools
> + */
> +function handleWindowResize(aController, aCallback) {

I would move this in utils.js

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8545895 [details] [diff] [review]
devkeystestv3.patch

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

For each different dev tool we should have a separate test method, yes. It should open the tool, perform all tests, and close it again.

Otherwise there are a couple of obvious things which I noted down inline.

::: firefox/tests/functional/testKeyboardShortcuts/testDeveloperTools.js
@@ +52,5 @@
> +  aModule.browserWindow = new browser.BrowserWindow();
> +
> +  // Store main window object for test scope use
> +  aModule.mainWindow = findElement.ID(aModule.browserWindow.controller.window.document,
> +                                      "main-window");

Make this a property of the base window and modify it so it uses Selector for `:root`, see https://github.com/mozilla/firefox-ui-tests/blob/master/firefoxtests/lib/api/windows.py#L33.

@@ +63,5 @@
> +                      "chrome://browser/locale/devtools/netmonitor.properties",
> +                      "chrome://browser/locale/devtools/profiler.properties",
> +                      "chrome://browser/locale/devtools/styleeditor.properties",
> +                      "chrome://browser/locale/devtools/webconsole.properties"
> +                     ]);

How are the dev tools adding those property files? If they are around since the initializatino of the browser window, you have to add this to the browser window class.

@@ +67,5 @@
> +                     ]);
> +}
> +
> +function setupTest(aModule) {
> +  aModule.mainWindow.waitForElement();

For what is that? This will always be present when Mozmill starts the test.

@@ +77,5 @@
> +}
> +
> +function teardownModule(aModule) {
> +  aModule.browserWindow.tabs.closeAllTabs();
> +  aModule.windows.closeAllWindows(aModule.browserWindow.controller);

You can pass the browser window in directly. Also you already have both in teardownTest. So just duplication.

@@ +91,5 @@
> +  expect.equal(widgets.getSelectedCell(win.tree).title, title,
> +               "Library has been opened on the correct location");
> +
> +  win.close();
> +}

I don't see the relation to dev tools here.

@@ +110,5 @@
> +               "Tab has been opened");
> +
> +  expect.equal(browserWindow.controller.tabs.activeTab.location.toString(),
> +               PROPERTIES.addons.URL, "The Add-ons Manager has been opened");
> +}

Same here. What has this to do with dev tools?

@@ +418,5 @@
> + *        MozMillController of the window to operate on
> + * @param {function} aCallback
> + *        Callback to use for loading/closing the developer tools
> + */
> +function handleWindowResize(aController, aCallback) {

Why? This is windows related and we have a windows library.

Comment 8

[Teodor Druta]

Attachment: devshortcuts_new.patch

I have rewritten this, again :(

So now everything is separated into separate test files.

The DevTools test was rewritten, so there are new events which it uses, these events are placed inside the test file, I would suggest that we should leave those there, at least for now, since we don't have any other test(s) that uses those events.
Henrik, Andreea, can you please provide some feedback on this ?

Comment 9

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8551249 [details] [diff] [review]
devshortcuts_new.patch

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

Way better this way to have separate tests! On the good track here.

::: firefox/tests/functional/testKeyboardShortcuts/testBrowserConsole.js
@@ +32,5 @@
> +  }, {state: "open", type: "devtools:webconsole"});
> +
> +  // Get Browser Console window root element
> +  var browserConsoleWin = findElement.ID(controller.window.document, "devtools-webconsole");
> +  // Check Browser Console window root element for visibility

Please combine the comments into one.

@@ +36,5 @@
> +  // Check Browser Console window root element for visibility
> +  expect.waitFor(() => utils.isDisplayed(controller, browserConsoleWin),
> +                 "Browser console window root element is displayed");
> +
> +  // Close Browser Console window

And this can be removed, it's clear from the method name.

::: firefox/tests/functional/testKeyboardShortcuts/testDevTools.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +/*var {devtools} = */

Please remove this.

@@ +52,5 @@
> +      shiftKey : true
> +    },
> +    {
> +      name : "netmonitor",
> +      // [ctrl + shift + q] / [cmd + opt + q]

If we really want the comments here, let's add them before each {} block.

@@ +108,5 @@
> +  TEST_DATA.tools.forEach(aTool => {
> +    key = browserWindow.getProperty(aTool.shortcut);
> +
> +    waitForTool(aTool.name, () => {
> +      // mainWindow.keypress(key, keyShortcut);

Remove this.

@@ +130,5 @@
> + * @param {string} aTool
> + *        Name of the tool we should check for
> + */
> +function checkCurrentTool(aTool) {
> +  // Check aTool parameter against the current selected tool id

No need for this.

@@ +154,5 @@
> + *        Callback to use for closing the developer tools
> + */
> +function closeDevTools(aCallback) {
> +  var callback = () => {
> +    // Get developer tools close button element

And here, it's pretty obvious from the code.

@@ +206,5 @@
> + * @param {function} aCallback
> + *        Callback to use for loading the developer tool
> + */
> +function waitForTool(aTool, aCallback) {
> +  var toolReady = false

semicolon

Comment 10

[Teodor Druta]

Attachment: devshortcuts_newv2.patch

Updated nits

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8557874 [details] [diff] [review]
devshortcuts_newv2.patch

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

A few nits, otherwise it looks good.

::: firefox/tests/functional/testKeyboardShortcuts/testDevTools.js
@@ +7,5 @@
> +Cu.import("resource:///modules/devtools/gDevTools.jsm");
> +Cu.import("resource://gre/modules/devtools/Loader.jsm");
> +
> +// Include the required modules
> +var addons = require("../../../../lib/addons");

This lib is not used in this test

@@ +12,5 @@
> +var utils = require("../../../../lib/utils");
> +var windows = require("../../../../lib/windows");
> +
> +var browser = require("../../../lib/ui/browser");
> +var widgets = require("../../../../lib/ui/widgets");

This is not used here, please remove

@@ +135,5 @@
> +
> +/**
> + * Checks if developer toolbox is open
> + */
> +function checkToolbox() {

nit: I think checkToolboxOpen() would be a more suggestive name

@@ +146,5 @@
> +
> +/**
> + * Closes developer tools and waits for it to unload
> + *
> + * @param {function} [aCallback]

This parameter can be removed (see bellow)

@@ +149,5 @@
> + *
> + * @param {function} [aCallback]
> + *        Callback to use for closing the developer tools
> + */
> +function closeDevTools(aCallback) {

You never use this function with a callback, please remove the parameter

@@ +156,5 @@
> +                                          "toolbox-close");
> +    devToolsCloseBtn.waitThenClick();
> +  };
> +
> +  callback = aCallback || callback;

Considering previous comment, you can remove this line

@@ +194,5 @@
> +  }
> +}
> +
> +/**
> + * Waits for a certain developer tools to load

nit: ... a certain developer tool ...

Comment 12

[Teodor Druta]

Attachment: devshortcuts_newv3.patch

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #11)
> ::: firefox/tests/functional/testKeyboardShortcuts/testDevTools.js
> @@ +135,5 @@
> > +
> > +/**
> > + * Checks if developer toolbox is open
> > + */
> > +function checkToolbox() {
> 
> nit: I think checkToolboxOpen() would be a more suggestive name
> 

Actually I think the comment here is not correct it checks for the toolbox element rather than toolbox open state, updated comment.

> @@ +149,5 @@
> > + *
> > + * @param {function} [aCallback]
> > + *        Callback to use for closing the developer tools
> > + */
> > +function closeDevTools(aCallback) {
> 
> You never use this function with a callback, please remove the parameter
> 

:(

Fixed all the nits.

Comment 13

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8559099 [details] [diff] [review]
devshortcuts_newv3.patch

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

Thanks, Daniel!

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #13)
> Comment on attachment 8559099 [details] [diff] [review]
> devshortcuts_newv3.patch
> 
> Review of attachment 8559099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, Daniel!

I meant "Thanks, Teodor!". Sorry...

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8559099 [details] [diff] [review]
devshortcuts_newv3.patch

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

Looks good from last reviewed and this version. Tests well on OSX.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8559099 [details] [diff] [review]
devshortcuts_newv3.patch

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

For me this is a hacky implementation of something which should be landed quickly. I do not feel comfortable to take this code at all. Given the amount of work which is still necessary here, it would probably make more sense to directly implement it via Marionette.

::: firefox/tests/functional/testKeyboardShortcuts/testBrowserConsole.js
@@ +14,5 @@
> +  aModule.browserWindow = new browser.BrowserWindow();
> +}
> +
> +function teardownModule(aModule) {
> +  aModule.browserWindow.tabs.closeAllTabs();

We don't open any tabs, so nothing to close here.

@@ +15,5 @@
> +}
> +
> +function teardownModule(aModule) {
> +  aModule.browserWindow.tabs.closeAllTabs();
> +  aModule.windows.closeAllWindows(aModule.browserWindow);

The windows module has  been imported outside of the aModule scope.

@@ +26,5 @@
> +  // Get Browser Console shortcut key code
> +  var browserConsoleKeyCode = browserWindow.getEntity("browserConsoleCmd.commandkey");
> +
> +  var controller = windows.waitForWindowState(() => {
> +    // Trigger keypress [ctrl/cmd + shift + J]

I would not list the current key combination. It can be invalid in the future.

@@ +28,5 @@
> +
> +  var controller = windows.waitForWindowState(() => {
> +    // Trigger keypress [ctrl/cmd + shift + J]
> +    browserWindow.windowElement.keypress(browserConsoleKeyCode, {accelKey: true, shiftKey: true});
> +  }, {state: "open", type: "devtools:webconsole"});

Lets create a minimal BrowserConsoleWindow class, and update the browser class for a browser.openConsole() method. That should be easily doable.

@@ +33,5 @@
> +
> +  // Check Browser Console window root element for visibility
> +  var browserConsoleWin = findElement.ID(controller.window.document, "devtools-webconsole");
> +  expect.waitFor(() => utils.isDisplayed(controller, browserConsoleWin),
> +                 "Browser console window root element is displayed");

Why do we have to check for all that? If the correct window gets opened I don't see that we have to further check elements in that window.

::: firefox/tests/functional/testKeyboardShortcuts/testDevTools.js
@@ +17,5 @@
> +  toolBox : [
> +    // [ctrl + shift + i] / [cmd + opt + i]
> +    "devToolboxMenuItem.keytext",
> +    // [F12]
> +    "devToolsCmd.keycode"

Please never list shortcuts. Those can all change.

@@ +28,5 @@
> +    },
> +    // [ctrl + shift + k] / [cmd + opt + k]
> +    {
> +      name : "webconsole",
> +      shortcut : "cmd.commandkey"

This looks suspicious. Do we really have such a non-meaningful name for the key?

@@ +39,5 @@
> +    // [shift + F7]
> +    {
> +      name : "styleeditor",
> +      shortcut : "open.commandkey",
> +      shiftKey : true

shift is hard-coded and not listed in a property file?

@@ +45,5 @@
> +    // [shift + F5]
> +    {
> +      name : "jsprofiler",
> +      shortcut : "profiler.commandkey2",
> +      shiftKey : true

Same here.

@@ +76,5 @@
> +  var key;
> +  // Handle differences between key shortcuts on OSX
> +  var helperKey = mozmill.isMac ? "altKey" : "shiftKey";
> +  var keyShortcut = {accelKey: true};
> +  keyShortcut[helperKey] = true;

Call this modifier.

@@ +98,5 @@
> +    // Close developer tools for first instance
> +    if (aIndex === 0) {
> +      closeDevTools();
> +    }
> +  });

I do not see why we need a loop here. With parts moved to a DeveloperToolbox class it will become way simpler.

@@ +128,5 @@
> + */
> +function checkCurrentTool(aTool) {
> +  assert.equal(toolbox.currentToolId, aTool,
> +               "Expected current tool - ", toolbox.currentToolId);
> +}

I don't see a reason for this extra method. It makes the test way harder to follow and there are only two places. The first instance might even want to have a different message.

@@ +138,5 @@
> +  var toolBox = findElement.ID(browserWindow.controller.window.document,
> +                               "toolbox-notificationbox");
> +  assert.waitFor(() => utils.isDisplayed(browserWindow.controller, toolBox) &&
> +                       toolBox.getNode().localName === "notificationbox",
> +                 "Toolbox element is displayed");

No element definitions in the test.

@@ +162,5 @@
> +    assert.waitFor(() => toolboxDestroyed, "Developer tools have been loaded");
> +  }
> +  finally {
> +    gDevTools.off("toolbox-destroyed", onToolboxDestroyed);
> +  }

Same as above applies here too.

::: firefox/tests/functional/testKeyboardShortcuts/testDeveloperToolbar.js
@@ +15,5 @@
> +}
> +
> +function teardownModule(aModule) {
> +  aModule.browserWindow.tabs.closeAllTabs();
> +  aModule.windows.closeAllWindows(aModule.browserWindow);

No tabs or windows are getting opened here, but you want to force close the toolbar.

@@ +23,5 @@
> + * Test Developer toolbar shortcut
> + */
> +function testDeveloperToolbarShortcut() {
> +  // Get developer toolbar shortcut key code
> +  var devToolbarKeyCode = browserWindow.getEntity("devToolbar.keycode");

Just key

@@ +30,5 @@
> +  browserWindow.windowElement.keypress(devToolbarKeyCode, {shiftKey: true});
> +
> +  // Check if developer toolbar is visible
> +  var devToolbar = findElement.ID(browserWindow.controller.window.document,
> +                                  "developer-toolbar");

No element definition in the test!

@@ +36,5 @@
> +                 "Developer Toolbar is visible");
> +
> +  // Get developer toolbar close button
> +  var devToolbarCloseBtn = findElement.ID(browserWindow.controller.window.document,
> +                                          "developer-toolbar-closebutton");

Same here. Not sure how this passed earlier review.

::: firefox/tests/functional/testKeyboardShortcuts/testOpenPageSource.js
@@ +33,5 @@
> +
> +  // Check Page Source window root element for visibility
> +  var viewSourceWin = findElement.ID(controller.window.document, "viewSource");
> +  expect.waitFor(() => utils.isDisplayed(controller, viewSourceWin),
> +                 "View source window root element is displayed");

All comments as given to the browser console apply here too.

::: firefox/tests/functional/testKeyboardShortcuts/testResponsiveDesignView.js
@@ +15,5 @@
> +}
> +
> +function teardownModule(aModule) {
> +  aModule.browserWindow.tabs.closeAllTabs();
> +  aModule.windows.closeAllWindows(aModule.browserWindow);

I don't see that any additional window or tab is getting opened. But you want to force close this tool.

@@ +23,5 @@
> + * Test Responsive Design View shortcut
> + */
> +function testResponsiveDesignViewShortcut() {
> +  // Get responsive design view shortcut key code
> +  var resDesignKeyCode = browserWindow.getEntity("responsiveDesignTool.commandkey");

Just key

@@ +30,5 @@
> +  browserWindow.windowElement.keypress(resDesignKeyCode, {accelKey: true, shiftKey: true});
> +
> +  // Check for responsive design view specific element visibility
> +  var tabViewDeck = findElement.ID(browserWindow.controller.window.document,
> +                                   "tab-view-deck");

No element definitions in the test! This has been set as requirement a long time ago.

::: firefox/tests/functional/testKeyboardShortcuts/testScratchpad.js
@@ +37,5 @@
> +                 "Scratchpad window root element is displayed");
> +
> +  // Close Scratchpad window
> +  controller.window.close();
> +}

All comments as given to the browser console apply here too.

::: lib/ui/base-window.js
@@ +51,5 @@
> +   * @returns {ElemBase} Window root element
> +   */
> +  get windowElement() {
> +    return findElement.Selector(this._controller.window.document, ":root");
> +  },

This has to be added to getEntities() and maybe you can name this 'inner'? That's what we will use in Marionette tests.

Comment 17

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #16)
> Given the amount of work which is still necessary here, it would probably make more
> sense to directly implement it via Marionette.

I'll be sure to check later to see how it will take around 1-3 years for a contributor to implement this test in marionette/in anything else, judging by the complexity of the developer tools implementation.

> For me this is a hacky implementation of something which should be landed
> quickly. I do not feel comfortable to take this code at all.

Ok, then if you feel like functional (as in they work) test(s) which could help in catching developer tools bugs should be left out of production, it's fine with me. I don't even understand why you took your time to review this patch then.

I don't think I should consume any more of my limited work time on this bug.
I'll mark this bug as WONTFIX. 
A LOT of work done here in the end resulted in being useless 
Please reopen if you have anything else to add, who am I kidding you will reopen it anyway and will reassign it to nobody.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Teodor Druta from comment #17)
> Ok, then if you feel like functional (as in they work) test(s) which could
> help in catching developer tools bugs should be left out of production, it's
> fine with me. I don't even understand why you took your time to review this
> patch then.

I reviewed the patch first but as more as I looked at it as more I was scared. Sorry, but it's not about getting code in just that it works. We still keep our level of quality, and this patch doesn't meet it clearly.

> I don't think I should consume any more of my limited work time on this bug.
> I'll mark this bug as WONTFIX. 
> A LOT of work done here in the end resulted in being useless 

I would suggest that you are more careful in the words you are using here. You all know how Mozmill tests are written and what we expect quality wise. And as I stated before lots of things have not been obeyed and should have caused a r- way earlier. There was indeed a delay in reviewing this code, but please stop blaming me for the quality of code you have written. Thanks.

> Please reopen if you have anything else to add, who am I kidding you will
> reopen it anyway and will reassign it to nobody.

Yes, I will reopen that bug because the resolution you decided to set is by far appropriate. It's no mentored bug at the moment, but we have to keep in on our radar for Marionette tests. I will close once we have an appropriate bug filed.

I just wonder why you didn't unassign yourself instead, but making such a useless comment. Please think about that.

Comment 19

[Teodor Druta]

I'm sorry for making such a half-thought and egoistic statement. I do realize that such actions can jeopardize time-built professional relations, and this is an unprofessional way to handle work related frustrations. I hope we will continue our good relations in the future, and by no mean I meant any person targeted attack. I really hope that I will be able to continue the work in the future on this bug and others as well as a contributor in my free time.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm happy to do so Teodor! And good to see this response. It's not always easy to get patches reviewed and have to make such comments when such an amount of work has been put into. But as a module owner I have to keep the quality bar at the level we expect. Even with the Mozmill tests being deprecated in the near future. Thanks for your understanding.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Mozmill is dead, WONTFIX the remaining bugs.