619460 - Initial test and shared module for Selenium IDE add-on

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Initial test and shared module for Selenium IDE add-on

Comment 1

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 497878 [details] [diff] [review]
Initial test and shared module for Selenium IDE add-on

This looks great to me. The things that jump out are:

I still don't like low cohesion getElement functions that take a spec and encapsulate a switch/case statement like this; I'd prefer a series of discrete getFoo functions. However, the way you've done it is consistent with existing shared modules, and Region Object Model will be per-element, so I'd keep as-is.

I'm happy to see us moving towards initial-caps on the module variables. We should add this to the style guide, assuming everyone agrees.

In the test, SeleniumManager should have been seleniumManager, since it was an instance. Only initial-caps the class; objects are just variables.

I think it would have been better to assign a shorter name to seleniumManager (the var in your module, not the original class) like "sm". Reason is that long words/phrases at the start of a phrase tend to be the only thing the eye sees. When you have a bunch of similar lines (seleniumManager.*) in a row, it's better to use a shorter name for readability and to highlight the method calls, even if it's just a temporary var at the top of the block.

On a side note, this is a case where I'd have disagreed with Crockford, and used a with statement--lots of seleniumManager calls and not too long of a block. However, since I'm pretty sure nobody else would agree, "sm" would be a poor-man's with. :)

Comment 2

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

Comment on attachment 497878 [details] [diff] [review]
Initial test and shared module for Selenium IDE add-on

Dave, seeing the progress compared to the former revisions, I'm impressed. It looks great. I haven't tested it yet, but wanna give my first review right away.

># HG changeset patch
># Parent c79ed8e8f01d1577f40a7bc2e26410ee1f4c1455
># User Dave Hunt <[e-mail]>
>Initial tests for Selenium IDE add-on.

Please check out review docs on MDN how we format the commit message.

>+++ b/addons/[e-mail]/shared-modules/selenium.js

>+function SeleniumManager() {
>+  this._controller = null;
>+}

Something I have also learned recently, _controller should be declared in the prototype so it will become a property of the class and not only for the instance.

>+  open : function SeleniumManager_open(browserController) {
>+    var menuItem = new elementslib.Elem(browserController.
>+                                        menus["tools-menu"].menu_ToolsPopupItem);

If you want, you can use the new Menu API, which is sadly not documented yet. For an example see my patch here:
https://bug597330.bugzilla.mozilla.org/attachment.cgi?id=483123

>+  setBaseUrl : function SeleniumManager_setBaseUrl(url) {
>+    var baseUrl = this.getElement({type: "base_url"});
>+    this._controller.type(baseUrl, url);
>+  },

Functions like those you can transform into properties via "set baseURL(url)", and which could be used like "ide.baseURL = 'some url'". It's up to you. We are ok with both cases. Only take care of capital letters for acronyms, i.e. URL.

>+  addCommand : function SeleniumManager_addCommand(spec) {
[..]
>+    var command = this.getElement({type: "command", subtype: "action"});
>+    this._controller.type(command, spec.action);
>+
>+    var target = this.getElement({type: "command", subtype: "target"});
>+    this._controller.type(target, spec.target);
>+
>+    var value = this.getElement({type: "command", subtype: "value"});
>+    this._controller.type(value, spec.value);

Are all those spec members optional? If not you should make sure they have been set before using them. Not sure how .type reacts for undefined values.

>+    //wait until play button is enabled
>+    this._controller.waitFor(function () {
>+      return playTest.getNode().disabled == false;

You should use triple operators, as we have decided on in our this weeks meeting. In this case it will be '==='.  

>+  get isSuiteProgressIndicatorGreen() {
>+    var suiteProgressIndicator = this.getElement({type: "suite_progress",
>+                                                 subtype: "indicator"});
>+    return this._controller.assertJSProperty(suiteProgressIndicator,
>+                                                "className",
>+                                                "success");

The shared module function should not assert itself. Simply pass back the progress indicator to the test, where the color can be asserted.

>+  get log() {
>+    var logConsole = this.getElement({type: "log_console"});
>+    return logConsole.getNode().innerHTML;

To be consistent with your other functions you should pass back the element, or rename the getter to logContent. Depending on what else you want to assert on, the element could be a better approach.

>+  getElement : function SeleniumManager_getElement(spec) {
>+    var elem = null;

Which of our shared modules have you used to check the getElement function? It wasn't the addons.js module from default, right? The version you have used is a bit outdated and also doesn't make use of the nodeCollector. 

>+var setupModule = function(module) {

You should start immediately with named functions. It's also something which has been added to our style guide. It applies to both functions in this test module.

>+  SeleniumManager.setBaseUrl("chrome://selenium-ide/");

Just out of interest, which other values can the base URL contain?

>+  SeleniumManager.addCommand({action: "open",
>+                             target: "/content/tests/functional/aut/search.html"});

Where is this file located? Part of the Selenium extension, and part of the given base URL as given above?

>+  SeleniumManager.addCommand({action: "verifyText",
>+                             target: "link=link with onclick attribute",
>+                             value: "link with onclick attribute"});

I don't know about the structure of the target and value content, but would that be compatible with localized versions of the add-on?

>+  //TODO check the class of the first item in the suite tree

Please file a follow-up and add the bug # as talked earlier this week.

>+  SeleniumManager.controller.assert(function () {
>+    return SeleniumManager.isSuiteProgressIndicatorGreen;
>+  }, "Expected the suite progress indicator to be green");

See my comment for the function in the shared module.

Comment 3

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

(In reply to comment #1)
> I'm happy to see us moving towards initial-caps on the module variables. We
> should add this to the style guide, assuming everyone agrees.

I have added this already to the Etherpad for the ongoing discussion. Sadly we haven't had time last week to check all those details. So please review it and reply to Anthony's mail. 

> I think it would have been better to assign a shorter name to seleniumManager
> (the var in your module, not the original class) like "sm". Reason is that 

Agreed on. Whatever you want to use, even 'ide' would be ok.

Comment 4

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to comment #2)
> >+  get isSuiteProgressIndicatorGreen() {
> >+    var suiteProgressIndicator = this.getElement({type: "suite_progress",
> >+                                                 subtype: "indicator"});
> >+    return this._controller.assertJSProperty(suiteProgressIndicator,
> >+                                                "className",
> >+                                                "success");
> 
> The shared module function should not assert itself. Simply pass back the
> progress indicator to the test, where the color can be asserted.

Totally agree re: shared mods not asserting. I do like this method, though, because tests don't need to know fine details about the indicators like className "success" somehow equals green. In a post-ROM setup, I'd probably have you create a new Widget method for the indicator that encapsulated that, but here it makes sense to have in the shared mod itself.

So, IMO, these types of methods are great but you should just pass back the boolean for the test to assert on.

return (suiteProgressIndicator.getNode().className === "success");

> >+var setupModule = function(module) {
> 
> You should start immediately with named functions. It's also something which
> has been added to our style guide. It applies to both functions in this test
> module.

Not sure this comment will be wholly clear to someone who's not used to Javascript, so pardon if I'm being obvious but I'm going to expand a little:

Henrik means use:

function setupModule() {

instead of the style above, which assigns an anonymous (unnamed) function to a global variable called setupModule. Reason is that lots of tools, including most debuggers and (I believe) mozmill's error reporting work much better that way.

> >+  SeleniumManager.controller.assert(function () {
> >+    return SeleniumManager.isSuiteProgressIndicatorGreen;
> >+  }, "Expected the suite progress indicator to be green");
> 
> See my comment for the function in the shared module.

Per my comment, I actually agree with this style; but as you wrote the code before, this assertion would have never been reached because assertJSProperty would have thrown first. 

If you change the code for isSuiteProgressIndicatorGreen as I suggested, I think this would be the way to go.

Comment 5

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Of course, I mean "function setupModule(module) {" in my comment above.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to comment #2)
> Comment on attachment 497878 [details] [diff] [review]
> 
> >+function SeleniumManager() {
> >+  this._controller = null;
> >+}
> 
> Something I have also learned recently, _controller should be declared in the
> prototype so it will become a property of the class and not only for the
> instance.

Sorry, but I don't understand this at all. Could you give some more details?

> >+  open : function SeleniumManager_open(browserController) {
> >+    var menuItem = new elementslib.Elem(browserController.
> >+                                        menus["tools-menu"].menu_ToolsPopupItem);
> 
> If you want, you can use the new Menu API, which is sadly not documented yet.
> For an example see my patch here:
> https://bug597330.bugzilla.mozilla.org/attachment.cgi?id=483123

I tried using the new Menu API but I must be missing something. I tried to use controller.mainMenu.click("#menu_ToolsPopupItem"); but without success.

> >+  setBaseUrl : function SeleniumManager_setBaseUrl(url) {
> >+    var baseUrl = this.getElement({type: "base_url"});
> >+    this._controller.type(baseUrl, url);
> >+  },
> 
> Functions like those you can transform into properties via "set baseURL(url)",
> and which could be used like "ide.baseURL = 'some url'". It's up to you. We are
> ok with both cases. Only take care of capital letters for acronyms, i.e. URL.

We should add this to our style guide. I've often seen acronyms not being capitalised in this way.

> >+  addCommand : function SeleniumManager_addCommand(spec) {
> [..]
> >+    var command = this.getElement({type: "command", subtype: "action"});
> >+    this._controller.type(command, spec.action);
> >+
> >+    var target = this.getElement({type: "command", subtype: "target"});
> >+    this._controller.type(target, spec.target);
> >+
> >+    var value = this.getElement({type: "command", subtype: "value"});
> >+    this._controller.type(value, spec.value);
> 
> Are all those spec members optional? If not you should make sure they have been
> set before using them. Not sure how .type reacts for undefined values.

The only one that is not optional is the action, however at least one of target or value should also be provided. I'm not sure this restriction should be added to the shared module however as it would hinder negative testing.

> >+  getElement : function SeleniumManager_getElement(spec) {
> >+    var elem = null;
> 
> Which of our shared modules have you used to check the getElement function? It
> wasn't the addons.js module from default, right? The version you have used is a
> bit outdated and also doesn't make use of the nodeCollector. 

Could you provide an example of what I should be using? I believe you originally pointed me in the direction of the implementation I have used.

> >+  SeleniumManager.setBaseUrl("chrome://selenium-ide/");
> 
> Just out of interest, which other values can the base URL contain?

I don't understand. It's a URL, so can contain any characters that are valid for a URL...

> >+  SeleniumManager.addCommand({action: "open",
> >+                             target: "/content/tests/functional/aut/search.html"});
> 
> Where is this file located? Part of the Selenium extension, and part of the
> given base URL as given above?

This is one of several 'test' pages packaged with the Selenium IDE add-on.

> >+  SeleniumManager.addCommand({action: "verifyText",
> >+                             target: "link=link with onclick attribute",
> >+                             value: "link with onclick attribute"});
> 
> I don't know about the structure of the target and value content, but would
> that be compatible with localized versions of the add-on?

These values are related to the content of the page and not the add-on.

> >+  //TODO check the class of the first item in the suite tree
> 
> Please file a follow-up and add the bug # as talked earlier this week.

I am thinking of removing this assertion but would appreciate some of your time to confirm that the effort involved in checking this would outweigh the benefit.

Comment 7

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

(In reply to comment #6)
> > Something I have also learned recently, _controller should be declared in the
> > prototype so it will become a property of the class and not only for the
> > instance.
> 
> Sorry, but I don't understand this at all. Could you give some more details?

See http://hg.mozilla.org/qa/mozmill-tests/file/43f2fb3d66b9/shared-modules/modal-dialog.js#l55

> I tried using the new Menu API but I must be missing something. I tried to use
> controller.mainMenu.click("#menu_ToolsPopupItem"); but without success.

It should work. Do you get any error in the Error Console?

> We should add this to our style guide. I've often seen acronyms not being
> capitalised in this way.

As mentioned on IRC please add this to the Etherpad for style guide discussions from Anthony.

> > >+  addCommand : function SeleniumManager_addCommand(spec) {
> > [..]
> > >+    var command = this.getElement({type: "command", subtype: "action"});
> > >+    this._controller.type(command, spec.action);
> > >+
> > >+    var target = this.getElement({type: "command", subtype: "target"});
> > >+    this._controller.type(target, spec.target);
> > >+
> > >+    var value = this.getElement({type: "command", subtype: "value"});
> > >+    this._controller.type(value, spec.value);
> > 
> > Are all those spec members optional? If not you should make sure they have been
> > set before using them. Not sure how .type reacts for undefined values.
> 
> The only one that is not optional is the action, however at least one of target
> or value should also be provided. I'm not sure this restriction should be added
> to the shared module however as it would hinder negative testing.

Which negative testing? Folding in undefined into MozMillController.type would definitely fail. In such a case you should at least check the parameter before calling type.

> > Which of our shared modules have you used to check the getElement function? It
> > wasn't the addons.js module from default, right? The version you have used is a
> > bit outdated and also doesn't make use of the nodeCollector. 
> 
> Could you provide an example of what I should be using? I believe you
> originally pointed me in the direction of the implementation I have used.

I pointed you to the addons manager of the default branch. But it looks like you have used the implementation from an older branch. It's up to you to change it or not, seeing the widget classes upcoming.

> > >+  SeleniumManager.setBaseUrl("chrome://selenium-ide/");
> > 
> > Just out of interest, which other values can the base URL contain?
> 
> I don't understand. It's a URL, so can contain any characters that are valid
> for a URL...

So what I tried to ask, what is the base URL used for? Is it used by the Selenium IDE to specify the page under test?

> > >+  //TODO check the class of the first item in the suite tree
> > 
> > Please file a follow-up and add the bug # as talked earlier this week.
> 
> I am thinking of removing this assertion but would appreciate some of your time
> to confirm that the effort involved in checking this would outweigh the
> benefit.

Depends on you. We should get at least a bug on file to enhance the handling of trees. Getting properties for rows, columns, or cells would be one feature of it.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Initial test and shared module for Selenium IDE add-on v1.1

Comment 9

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

Comment on attachment 499571 [details] [diff] [review]
Initial test and shared module for Selenium IDE add-on v1.1

>+  addCommand : function SeleniumManager_addCommand(spec) {
[..]
>+    if (undefined != spec.action) {

Can you please use the triple operator and put the constant as second operator?

>+    if (undefined != spec.target != "") {

This looks strange. What are you trying to accomplish? This should always be false.

>+      return playTest.getNode().disabled === false;

nit: We already have a Boolean value here, so "=== false" is not necessary. It's up to you.

>+  get isSuiteProgressIndicatorGreen() {
>+    var suiteProgressIndicator = this.getElement({type: "suite_progress",
>+                                                 subtype: "indicator"});
>+    return (suiteProgressIndicator.getNode().className === "success");

Would have been better to return the element itself, so the test can simply run an assertJSProperty call on it.

>+  get logConsole() {
>+    var logConsole = this.getElement({type: "log_console"});
>+    return logConsole;

nit: you can directly return the element here.

>+  getElements : function SeleniumManager_getElements(aSpec) {
[..]
>+    switch (type) {
>+      /**
>+       * subtype: subtype to match
>+       * value: value to match
>+       */
>+      case "base_url":
>+        nodeCollector.queryNodes("#baseURL");
>+        break;
>+      case "button":
>+        switch(spec.subtype) {
>+          case "play_test":
>+            nodeCollector.queryNodes("#play-button");
>+            break;
>+        }

Initially the subtype property has been created to specify a property name of the to retrieve element. It can be used to retrieve a specific spec.value. So you should have only a single level switch statement and items like "button_playTest". Otherwise you will run into trouble with more complicated tests, which have to use lists. We can leave that for now, because it works. But in the future you will definitely have to change it.

>+  //check suite progress indicator
>+  sm.controller.assert(function () {
>+    return sm.isSuiteProgressIndicatorGreen;
>+  }, "Expected the suite progress indicator to be green");

It's helpful to add the got/expected parts to the message. But it's something you can also do later.

r- because of the suspicious comparison from above.

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Initial test and shared module for Selenium IDE add-on v1.2

(In reply to comment #9)
> Comment on attachment 499571 [details] [diff] [review]
> Initial test and shared module for Selenium IDE add-on v1.1
> 
> >+  addCommand : function SeleniumManager_addCommand(spec) {
> [..]
> >+    if (undefined != spec.action) {
> 
> Can you please use the triple operator and put the constant as second operator?

Done.

> >+    if (undefined != spec.target != "") {
> 
> This looks strange. What are you trying to accomplish? This should always be
> false.

Fixed. I think this must have been a typo.

> >+      return playTest.getNode().disabled === false;
> 
> nit: We already have a Boolean value here, so "=== false" is not necessary.
> It's up to you.

Done.

> >+  get isSuiteProgressIndicatorGreen() {
> >+    var suiteProgressIndicator = this.getElement({type: "suite_progress",
> >+                                                 subtype: "indicator"});
> >+    return (suiteProgressIndicator.getNode().className === "success");
> 
> Would have been better to return the element itself, so the test can simply run
> an assertJSProperty call on it.

I disagree. This way the test doesn't need to know about the class names. If the class names or method of indicating the progress indicator is green then we'd only need to update the shared module.

> >+  get logConsole() {
> >+    var logConsole = this.getElement({type: "log_console"});
> >+    return logConsole;
> 
> nit: you can directly return the element here.

Done.

> >+  getElements : function SeleniumManager_getElements(aSpec) {
> [..]
> >+    switch (type) {
> >+      /**
> >+       * subtype: subtype to match
> >+       * value: value to match
> >+       */
> >+      case "base_url":
> >+        nodeCollector.queryNodes("#baseURL");
> >+        break;
> >+      case "button":
> >+        switch(spec.subtype) {
> >+          case "play_test":
> >+            nodeCollector.queryNodes("#play-button");
> >+            break;
> >+        }
> 
> Initially the subtype property has been created to specify a property name of
> the to retrieve element. It can be used to retrieve a specific spec.value. So
> you should have only a single level switch statement and items like
> "button_playTest". Otherwise you will run into trouble with more complicated
> tests, which have to use lists. We can leave that for now, because it works.
> But in the future you will definitely have to change it.

Done. I've used a snakeCase naming convention, with an underscore to simulate a hierarchy or categorisation - as you've used in your suggestion.

> >+  //check suite progress indicator
> >+  sm.controller.assert(function () {
> >+    return sm.isSuiteProgressIndicatorGreen;
> >+  }, "Expected the suite progress indicator to be green");
> 
> It's helpful to add the got/expected parts to the message. But it's something
> you can also do later.

Done. In this case I didn't feel it was worth storing the result, as it will only fail if a boolean is not the expected value so the message is simple to predict. I also updated all other assert/waitFor messages.

Comment 11

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

Comment on attachment 504672 [details] [diff] [review]
Initial test and shared module for Selenium IDE add-on v1.2

(In reply to comment #10)
> > >+  get isSuiteProgressIndicatorGreen() {
> > >+    var suiteProgressIndicator = this.getElement({type: "suite_progress",
> > >+                                                 subtype: "indicator"});
> > >+    return (suiteProgressIndicator.getNode().className === "success");
> > 
> > Would have been better to return the element itself, so the test can simply run
> > an assertJSProperty call on it.
> 
> I disagree. This way the test doesn't need to know about the class names. If
> the class names or method of indicating the progress indicator is green then
> we'd only need to update the shared module.

Makes sense, but instead of "IndicatorGreen" you should have used a better naming which gives an impression of the real state and not about a color. But it's a wording you may change in the future.

> Done. I've used a snakeCase naming convention, with an underscore to simulate a
> hierarchy or categorisation - as you've used in your suggestion.

Looks good and will give you all the options until we have completely integrated the nodeCollector.

> > >+  //check suite progress indicator
> > >+  sm.controller.assert(function () {
> > >+    return sm.isSuiteProgressIndicatorGreen;
> > >+  }, "Expected the suite progress indicator to be green");
> > 
> > It's helpful to add the got/expected parts to the message. But it's something
> > you can also do later.
> 
> Done. In this case I didn't feel it was worth storing the result, as it will
> only fail if a boolean is not the expected value so the message is simple to
> predict. I also updated all other assert/waitFor messages.

You are right. We do the same in our other Mozmill tests. Feel free to remove those.

Running the test showed me that we can't connect to:
http://xserve.openqa.org:8085/browse/IDE-EDITOR-JOB1-73/artifact/Selenium-IDE-Editor/selenium-ide.xpi

We could checkin the patch on older branches but leave it open for default until the compatibility info has been applied.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Initial test and shared module for Selenium IDE add-on v1.3

(In reply to comment #11)
> Comment on attachment 504672 [details] [diff] [review]
> Initial test and shared module for Selenium IDE add-on v1.2
> 
> (In reply to comment #10)
> > > >+  get isSuiteProgressIndicatorGreen() {
> > > >+    var suiteProgressIndicator = this.getElement({type: "suite_progress",
> > > >+                                                 subtype: "indicator"});
> > > >+    return (suiteProgressIndicator.getNode().className === "success");
> > > 
> > > Would have been better to return the element itself, so the test can simply run
> > > an assertJSProperty call on it.
> > 
> > I disagree. This way the test doesn't need to know about the class names. If
> > the class names or method of indicating the progress indicator is green then
> > we'd only need to update the shared module.
> 
> Makes sense, but instead of "IndicatorGreen" you should have used a better
> naming which gives an impression of the real state and not about a color. But
> it's a wording you may change in the future.

I agree, but I can't think of a more suitable name that I'm happy with. I don't want to make the name much less granular. I'm interested to hear suggestions.

> > > >+  //check suite progress indicator
> > > >+  sm.controller.assert(function () {
> > > >+    return sm.isSuiteProgressIndicatorGreen;
> > > >+  }, "Expected the suite progress indicator to be green");
> > > 
> > > It's helpful to add the got/expected parts to the message. But it's something
> > > you can also do later.
> > 
> > Done. In this case I didn't feel it was worth storing the result, as it will
> > only fail if a boolean is not the expected value so the message is simple to
> > predict. I also updated all other assert/waitFor messages.
> 
> You are right. We do the same in our other Mozmill tests. Feel free to remove
> those.

To clarify, I have now removed the 'got, expected' from assert failure messages that check a boolean.

> Running the test showed me that we can't connect to:
> http://xserve.openqa.org:8085/browse/IDE-EDITOR-JOB1-73/artifact/Selenium-IDE-Editor/selenium-ide.xpi
> 
> We could checkin the patch on older branches but leave it open for default
> until the compatibility info has been applied.

I have reverted back to the current released version of Selenium IDE. As noted, this patch should only be applied to older branches until a new version is released with support for the latest nightlies.

Comment 13

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

Comment on attachment 505041 [details] [diff] [review]
Initial test and shared module for Selenium IDE add-on v1.3

Changes looks great. Lets get this landed on the older branches for now.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/52227ffc0c36 (1.9.2)
http://hg.mozilla.org/qa/mozmill-tests/rev/27f1deb44400 (1.9.1)

Please tell me when it can be checked into default.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

With Mozmill 1.5.2rc4 we now disable the compatibility check, which was preventing us from checking this into default. I have successfully run the tests with 1.5.2rc4 so once 1.5.2 is released we should check this in.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Mozmill 1.5.2 is now released. Henrik, could we land this on default now?

Comment 16

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/d029fca85b45