Refactor Selenium IDE tests to improve setup and use assertions library

RESOLVED FIXED

Status

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

People

(Reporter: davehunt, Assigned: davehunt)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Some of the earlier tests used the IDE for steps that should be in setup. This upcoming patch will make the tests consistent, and also make more use of the assertions library.
(Assignee)

Comment 1

6 years ago
Created attachment 606521 [details] [diff] [review]
Refactor the existing Selenium IDE tests. v1.0
Assignee: nobody → dave.hunt
Attachment #606521 - Flags: review?(hskupin)
Comment on attachment 606521 [details] [diff] [review]
Refactor the existing Selenium IDE tests. v1.0

> // Include required modules
> var selenium = require("../../../lib/selenium");
> var checks = require("../../../lib/checks");
>+var {assert} = require("../../../../../../lib/assertions");
>+var tabs = require("../../../../../../lib/tabs");

Please fix the order of requires so that they are in alphabetical order.

>+  tabBrowser = new tabs.tabBrowser(controller);  
>+  tabBrowser.closeAllTabs();

There is no need to instantiate tabBrowser. Just call tabs.closeAllTabs(controller)

>+  controller.open("chrome://selenium-ide/content/tests/functional/aut/alert.html");
>+  controller.waitForPageLoad();
> }
> 
> function teardownModule(module) {
>   sm.close();
> }
> 
> function testAssertAlertCommandFails() {
>-  sm.baseURL = "chrome://selenium-ide/";
>-  sm.addCommand({action: "open",
>-                target: "/content/tests/functional/aut/alert.html"});

Can you please explain this change? It kills a test by replacing it with a setup routine.

>+  assert.notEqual(sm.finalLogInfoMessage, "echo: final command");

You don't want to add a message as last parameter?

Otherwise I skimmed over and the above comments will apply to each file.
Attachment #606521 - Flags: review?(hskupin) → review-
(Assignee)

Comment 3

6 years ago
(In reply to Henrik Skupin (:whimboo) from comment #2)
> >+  controller.open("chrome://selenium-ide/content/tests/functional/aut/alert.html");
> >+  controller.waitForPageLoad();
> > }
> > 
> > function teardownModule(module) {
> >   sm.close();
> > }
> > 
> > function testAssertAlertCommandFails() {
> >-  sm.baseURL = "chrome://selenium-ide/";
> >-  sm.addCommand({action: "open",
> >-                target: "/content/tests/functional/aut/alert.html"});
> 
> Can you please explain this change? It kills a test by replacing it with a
> setup routine.

How do you mean it kills a test? The open command is not what we're testing here, so I'm simply opening the test page before interacting with Selenium IDE. I believe this is how the test should be, limiting failures to the functionality being tested.

> >+  assert.notEqual(sm.finalLogInfoMessage, "echo: final command");
> 
> You don't want to add a message as last parameter?

Did you have a suggestion? I'm happy with the default failure message.
Status: NEW → ASSIGNED
(In reply to Dave Hunt (:davehunt) from comment #3)
> here, so I'm simply opening the test page before interacting with Selenium
> IDE. I believe this is how the test should be, limiting failures to the
> functionality being tested.

Ok, so that's fine then. Thanks for the explanation.

> > >+  assert.notEqual(sm.finalLogInfoMessage, "echo: final command");
> > 
> > You don't want to add a message as last parameter?
> 
> Did you have a suggestion? I'm happy with the default failure message.

If you are happy, then I'm too. Just leave it as it is right now.
(Assignee)

Comment 5

6 years ago
Created attachment 607152 [details] [diff] [review]
Refactor the existing Selenium IDE tests. v1.1

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 606521 [details] [diff] [review]
> Refactor the existing Selenium IDE tests. v1.0
> 
> > // Include required modules
> > var selenium = require("../../../lib/selenium");
> > var checks = require("../../../lib/checks");
> >+var {assert} = require("../../../../../../lib/assertions");
> >+var tabs = require("../../../../../../lib/tabs");
> 
> Please fix the order of requires so that they are in alphabetical order.

Done.

> >+  tabBrowser = new tabs.tabBrowser(controller);  
> >+  tabBrowser.closeAllTabs();
> 
> There is no need to instantiate tabBrowser. Just call
> tabs.closeAllTabs(controller)

Done.
Attachment #606521 - Attachment is obsolete: true
Attachment #607152 - Flags: review?(hskupin)
Comment on attachment 607152 [details] [diff] [review]
Refactor the existing Selenium IDE tests. v1.1

Inspection didn't show up anything suspicious. Looks good and is ready to get landed.
Attachment #607152 - Flags: review?(hskupin) → review+
You need to log in before you can comment on or make changes to this bug.