736428 - Refactor Selenium IDE tests to improve setup and use assertions library

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

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

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.

Comment 1

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

Attachment: Refactor the existing Selenium IDE tests. v1.0

Comment 2

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

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.

Comment 3

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

(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.

Comment 4

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

(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.

Comment 5

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

Attachment: 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.

Comment 6

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

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.

Comment 7

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/e357d526e8f7 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/8ca0036fd9d0 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/baeb29de1784 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/b33d22be61c1 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/51c5b4b8114c (mozilla-esr10)