Additional tests for the Selenium IDE add-on

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: davehunt, Assigned: davehunt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 519669 [details] [diff] [review]
Additional tests for the Selenium IDE add-on. v1
Attachment #519669 - Flags: review?(hskupin)
(Assignee)

Comment 1

7 years ago
Created attachment 519670 [details] [diff] [review]
Additional tests for the Selenium IDE add-on. v1.1
Assignee: nobody → dave.hunt
Attachment #519669 - Attachment is obsolete: true
Attachment #519669 - Flags: review?(hskupin)
Attachment #519670 - Flags: review?(hskupin)
Comment on attachment 519670 [details] [diff] [review]
Additional tests for the Selenium IDE add-on. v1.1

Gna, an OS X hard-crash completely killed all my review comments right before I was trying to finish it up. Next time please really keep patches smaller and separated into at modules, tests.

>+    if (this._controller) {
>+      // Check if we should force the closing of the Selenium IDE window
>+      this._controller.window.close();

I don't see a usage of force. You always hard-close the window.

>+      this._controller.waitFor(function () {
>+        return mozmill.utils.getWindows().length === (windowCount - 1);
>+      }, "Selenium IDE has closed");

It's more robust to check for the topmost window with the selenium ide type.

>+   * @returns Log info
>+   * @type {array of ElemBase}

That should be '@returns {ElemBase[]} desc'. It also applies to all other instances in that file.

>+    var logInfo = this.logInfo;
>+    var finalLogInfoMessage = logInfo[logInfo.length-1].getNode().textContent;
>+    return finalLogInfoMessage.substring(7, finalLogInfoMessage.length-1);

Any chance to use a regex here? 

>+  ///////////////////////////////
>+  // Checks section
>+  ///////////////////////////////

You should move all those methods to it's own helper module and don't mix it up in the Selenium IDE. Assertions shouldn't happen in ui implementation modules. 

>+var Selenium = require("../../../lib/selenium");

Keep with a lowercase first letter.

>+  sm.addCommand({action: "open",
>+                target: "/content/tests/functional/aut/search.html"});
>+  sm.addCommand({action: "assertNotText",
>+                target: "link=link with onclick attribute",
>+                value: "flying monkies!"});
>+  sm.addCommand({action: "echo",
>+                target: "final command"});

You could think about using an array for this method. That way you can pre-define all commands outside of the test function...

>+  sm.addCommand({action: "verifyText",
>+                target: "link=link with onclick attribute",
>+                value: "flying monkies!"});
>+  sm.addCommand({action: "echo",
>+                target: "final command"});
>+  sm.playTest();
>+
>+  sm.checkCommandFailed("Actual value 'link with onclick attribute' did not match 'flying monkies!'");

... which would also help you in reusing the properties for those checks.

>+function testVerifyTextNotPresentCommandPasses() {
>+  sm.open(controller);

You can move the opening of the window to the setupModule function. Same for close(). There should be one single test which really has it in the test function itself. For all the others it's just a setup step.

Most of the comments are optional and only a recommendation but we should fix the close() function and move our the assertion checks to a separate module.
Attachment #519670 - Flags: review?(hskupin) → review-
(Assignee)

Comment 3

7 years ago
Created attachment 523189 [details] [diff] [review]
Additional tests for the Selenium IDE add-on. v2

(In reply to comment #2)
> Comment on attachment 519670 [details] [diff] [review]
> Additional tests for the Selenium IDE add-on. v1.1
> 
> Gna, an OS X hard-crash completely killed all my review comments right before I
> was trying to finish it up. Next time please really keep patches smaller and
> separated into at modules, tests.
> 
> >+    if (this._controller) {
> >+      // Check if we should force the closing of the Selenium IDE window
> >+      this._controller.window.close();
> 
> I don't see a usage of force. You always hard-close the window.

This was thinking ahead for tests that might not want to force the window closed. For the time being I have just forced the window closed.

> >+      this._controller.waitFor(function () {
> >+        return mozmill.utils.getWindows().length === (windowCount - 1);
> >+      }, "Selenium IDE has closed");
> 
> It's more robust to check for the topmost window with the selenium ide type.

I couldn't find an example of your suggestion, and the code I used was elsewhere in our shared-modules. With a force close I don't think it's necessary to do this assert anyway so I have removed it.

> >+   * @returns Log info
> >+   * @type {array of ElemBase}
> 
> That should be '@returns {ElemBase[]} desc'. It also applies to all other
> instances in that file.

Done. I also took a look over the API refactoring repository as you suggested and have brought this more in-line.

> >+    var logInfo = this.logInfo;
> >+    var finalLogInfoMessage = logInfo[logInfo.length-1].getNode().textContent;
> >+    return finalLogInfoMessage.substring(7, finalLogInfoMessage.length-1);
> 
> Any chance to use a regex here? 

Is there any advantage in using a regex? I would have thought a simple substring would be better.

> >+  ///////////////////////////////
> >+  // Checks section
> >+  ///////////////////////////////
> 
> You should move all those methods to it's own helper module and don't mix it up
> in the Selenium IDE. Assertions shouldn't happen in ui implementation modules. 

Done.

> >+var Selenium = require("../../../lib/selenium");
> 
> Keep with a lowercase first letter.

Done.

> >+  sm.addCommand({action: "open",
> >+                target: "/content/tests/functional/aut/search.html"});
> >+  sm.addCommand({action: "assertNotText",
> >+                target: "link=link with onclick attribute",
> >+                value: "flying monkies!"});
> >+  sm.addCommand({action: "echo",
> >+                target: "final command"});
> 
> You could think about using an array for this method. That way you can
> pre-define all commands outside of the test function...

I'm not sure this benefits the test enough to balance the reduce readability of it, so I think for now I will leave it as is. Perhaps we can discuss this further?

> >+  sm.addCommand({action: "verifyText",
> >+                target: "link=link with onclick attribute",
> >+                value: "flying monkies!"});
> >+  sm.addCommand({action: "echo",
> >+                target: "final command"});
> >+  sm.playTest();
> >+
> >+  sm.checkCommandFailed("Actual value 'link with onclick attribute' did not match 'flying monkies!'");
> 
> ... which would also help you in reusing the properties for those checks.

Again, not sure the benefit is worth the cost.

> >+function testVerifyTextNotPresentCommandPasses() {
> >+  sm.open(controller);
> 
> You can move the opening of the window to the setupModule function. Same for
> close(). There should be one single test which really has it in the test
> function itself. For all the others it's just a setup step.

Done.
Attachment #519670 - Attachment is obsolete: true
Attachment #523189 - Flags: review?(hskupin)
(In reply to comment #3)
> > >+    if (this._controller) {
> > >+      // Check if we should force the closing of the Selenium IDE window
> > >+      this._controller.window.close();
> > 
> > I don't see a usage of force. You always hard-close the window.
> 
> This was thinking ahead for tests that might not want to force the window
> closed. For the time being I have just forced the window closed.

Which is correct. Keep the function parameter and evaluate it in this method. If force is true do it that way, otherwise fire the appropriate keyboard shortcut to close the window. Per default force should be true and optional.

> > >+      this._controller.waitFor(function () {
> > >+        return mozmill.utils.getWindows().length === (windowCount - 1);
> > >+      }, "Selenium IDE has closed");
> > 
> > It's more robust to check for the topmost window with the selenium ide type.
> 
> I couldn't find an example of your suggestion, and the code I used was
> elsewhere in our shared-modules. With a force close I don't think it's
> necessary to do this assert anyway so I have removed it.

You can check the return value of:
mozmill.utils.getMostRecentWindow("global:selenium-ide")

or simply do the same thing as what we do in handleWindow:

      mozmill.utils.waitFor(function () {
        return window.closed;
      }, "Window has been closed.");


> > >+    var logInfo = this.logInfo;
> > >+    var finalLogInfoMessage = logInfo[logInfo.length-1].getNode().textContent;
> > >+    return finalLogInfoMessage.substring(7, finalLogInfoMessage.length-1);
> > 
> > Any chance to use a regex here? 
> 
> Is there any advantage in using a regex? I would have thought a simple
> substring would be better.

It would not fail if positions are changed in the string. Also if the first 7 characters are getting changed, it's more obvious for what you are looking for.

> > >+  sm.addCommand({action: "verifyText",
> > >+                target: "link=link with onclick attribute",
> > >+                value: "flying monkies!"});
> > >+  sm.addCommand({action: "echo",
> > >+                target: "final command"});
> > >+  sm.playTest();
> > >+
> > >+  sm.checkCommandFailed("Actual value 'link with onclick attribute' did not match 'flying monkies!'");
> > 
> > ... which would also help you in reusing the properties for those checks.
> 
> Again, not sure the benefit is worth the cost.

As said on IRC yesterday, that's up to you. But it will make it harder to maintain tests, that's why we are always using constants in our functional tests.
Status: NEW → ASSIGNED
Comment on attachment 523189 [details] [diff] [review]
Additional tests for the Selenium IDE add-on. v2

>+ * @author dhunt@mozilla.com (Dave Hunt)

You can leave this out. We already have this information in the license header.

>+ * @param {SeleniumManager} sm Selenium manager instance 

Nit: Parameters should have a descriptive name. But up to you.

>+ sm.controller.assert(function () {
>+   return logErrors.length === 0;
>+ }, "No error messages present, got " + logErrors.length +" expected " + 0 + "");

Please use: " - got 'xyz', expected 'zyx'" across your assert and waitFor methods.

>+  close : function SeleniumManager_close() {
>+    this._controller.window.close();
>+    this._controller = null;
>+  },

That's the wrong direction you went to. The code from the last iteration was fine, only the handling of the force parameter was missing as stated in an earlier comment.

Otherwise it looks fine.
Attachment #523189 - Flags: review?(hskupin) → review-
(Assignee)

Comment 7

7 years ago
Created attachment 523432 [details] [diff] [review]
Additional tests for the Selenium IDE add-on. v2.1

(In reply to comment #5)
> > > >+    var logInfo = this.logInfo;
> > > >+    var finalLogInfoMessage = logInfo[logInfo.length-1].getNode().textContent;
> > > >+    return finalLogInfoMessage.substring(7, finalLogInfoMessage.length-1);
> > > 
> > > Any chance to use a regex here? 
> > 
> > Is there any advantage in using a regex? I would have thought a simple
> > substring would be better.
> 
> It would not fail if positions are changed in the string. Also if the first 7
> characters are getting changed, it's more obvious for what you are looking for.

I'm sold. Done.

(In reply to comment #6)
> Comment on attachment 523189 [details] [diff] [review]
> Additional tests for the Selenium IDE add-on. v2
> 
> >+ * @author dhunt@mozilla.com (Dave Hunt)
> 
> You can leave this out. We already have this information in the license header.

Removed.

> >+ * @param {SeleniumManager} sm Selenium manager instance 
> 
> Nit: Parameters should have a descriptive name. But up to you.

Changed to seleniumManager.

> >+ sm.controller.assert(function () {
> >+   return logErrors.length === 0;
> >+ }, "No error messages present, got " + logErrors.length +" expected " + 0 + "");
> 
> Please use: " - got 'xyz', expected 'zyx'" across your assert and waitFor
> methods.

Done, although when I'm doing a numerical comparison I'm not including the single quotes.

> >+  close : function SeleniumManager_close() {
> >+    this._controller.window.close();
> >+    this._controller = null;
> >+  },
> 
> That's the wrong direction you went to. The code from the last iteration was
> fine, only the handling of the force parameter was missing as stated in an
> earlier comment.

As discussed on IRC, we're going to keep with the force close method.
Attachment #523189 - Attachment is obsolete: true
Attachment #523432 - Flags: review?(hskupin)
(Assignee)

Comment 8

7 years ago
Created attachment 523465 [details] [diff] [review]
Additional tests for the Selenium IDE add-on. v2.2

Darn. Forgot a qref. Added the quotes around the numbers in the got, expected strings.
Attachment #523432 - Attachment is obsolete: true
Attachment #523432 - Flags: review?(hskupin)
Attachment #523465 - Flags: review?(hskupin)
Comment on attachment 523465 [details] [diff] [review]
Additional tests for the Selenium IDE add-on. v2.2

>+   * Close Selenium IDE
>+   *
>+   * @param {Boolean} force Force the closing of the Selenium IDE window
>+   */
>+  close : function SeleniumManager_close() {

There is no force parameter anymore. Just remove the @param line.

>+    this._controller.window.close();
>+    this._controller = null;

You should add a waitForClosed method which really waits until the window has been closed. See handleWindow in utils.js how we do that in general. If you want you can do that in a follow-up. I would even prefer that.

r=me, at least with the comment fixed.
Attachment #523465 - Flags: review?(hskupin) → review+
(Assignee)

Comment 10

7 years ago
Created attachment 523577 [details] [diff] [review]
Additional tests for the Selenium IDE add-on. v2.3

(In reply to comment #9)
> Comment on attachment 523465 [details] [diff] [review]
> Additional tests for the Selenium IDE add-on. v2.2
> 
> >+   * Close Selenium IDE
> >+   *
> >+   * @param {Boolean} force Force the closing of the Selenium IDE window
> >+   */
> >+  close : function SeleniumManager_close() {
> 
> There is no force parameter anymore. Just remove the @param line.

Done.

> >+    this._controller.window.close();
> >+    this._controller = null;
> 
> You should add a waitForClosed method which really waits until the window has
> been closed. See handleWindow in utils.js how we do that in general.

Done.
Attachment #523465 - Attachment is obsolete: true
Attachment #523577 - Flags: review?(hskupin)
Attachment #523577 - Flags: review?(hskupin) → review+
You need to log in before you can comment on or make changes to this bug.