511262 - Remove UtilsAPI.delayedAssertNode and UtilsAPI.delayedClick from UtilsAPI

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

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

UtilsAPI.delayedAssertNode and UtilsAPI.delayedClick are not needed in favor of controller.waitForElement and controller.waitThenClick. Both functions should be removed.

Comment 1

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

Attachment: Patch v1

I thought that the patch wouldn't be so big but finally I had to change a bit more to get a consistency in our Mozmill tests.

Following items have been updated:
* Both delayed functions have been removed
* waitForPageLoad doesn't have the current tab as param
* waitThenClick and waitForElement always use a timeout value
* Moved UtilsAPI.closeAllTabs into setup module - no need for tearDownModule
* Use waitThenClick for first click in preferences dialog


Clint, would you prefer var or const for gDelay and gTimeout? I vague remember that we had a problem with modules when using const. That's why I have used var for now.

Restart tests will not work because of a change on AMO. I will fix it in a follow-up bug.

Comment 2

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

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

Comment 3

[cmtalbert]

Um... I wasn't finished.

(In reply to comment #1)
> Created an attachment (id=395213) [details]
You've got an r+ from me, but I have a concern.  It doesn't change any big code refactoring though, so I'm comfortable having you carry the review forward.  Read on...
> Following items have been updated:
> * Both delayed functions have been removed
Cool
> * waitForPageLoad doesn't have the current tab as param
cool
> * waitThenClick and waitForElement always use a timeout value
ok
> * Moved UtilsAPI.closeAllTabs into setup module - no need for tearDownModule
This is really quite a stylistic choice.  Either each test cleans up after itself or each test cleans up the area before it starts. It doesn't matter at all which because both methods are equivalent -- they both ensure that state is clean.  It might be argued that this approach is better (in setupModule) because then every test can depend on itself having a clean slate to run in.
> * Use waitThenClick for first click in preferences dialog
cool
> 
> Clint, would you prefer var or const for gDelay and gTimeout? I vague remember
> that we had a problem with modules when using const. That's why I have used var
> for now.
And here's where my review comment is.  I think you should use const.  I think this because gDelay and gTimeout are some pretty common names.  By using const the tests will break if those values are already defined somewhere else (in a shared library, in the browser etc).  If you use var then you depend on the *order* of the declarations.  So if some bit of Javascript executes during your test and redeclares a var gTimeout=20 then gTimeout is going to be 20 for the rest of the test.  Using const will prevent this because it will break and you will know that in this case you need to change gTimeout to a different variable name.

So, I think using them as const is quite a bit safer and will ensure consistent test runs in the future.  One way to prevent breakage would also be to namespace these to something like:

const gMozmill-Delay
const gMozmill-Timeout

(or something like that ^^)

Also, you missed a const definition in the Amazon test.  It's still const in this patch (unless I read it wrong :) )

> Restart tests will not work because of a change on AMO. I will fix it in a
> follow-up bug.
Ok cool.

Comment 4

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

(In reply to comment #3)
> > * Moved UtilsAPI.closeAllTabs into setup module - no need for tearDownModule
> This is really quite a stylistic choice.  Either each test cleans up after
> itself or each test cleans up the area before it starts. It doesn't matter at
> all which because both methods are equivalent -- they both ensure that state is
> clean.  It might be argued that this approach is better (in setupModule)
> because then every test can depend on itself having a clean slate to run in.

We should distinct between backend changes and ui states. Having multiple tabs open doesn't matter. If a test requires a specific page open in exactly one tab it should call this function in setupModule. Instead modifying preferences has much deeper effect for the profile data. Other tests will not know about those changes. That's why a test has to clear/reset them. Closing tabs all the time takes a bit time we really don't have to spent for.

> And here's where my review comment is.  I think you should use const.  I think
> this because gDelay and gTimeout are some pretty common names.  By using const
> the tests will break if those values are already defined somewhere else (in a
> shared library, in the browser etc).  If you use var then you depend on the
> *order* of the declarations.  So if some bit of Javascript executes during 

Alright. I will change it to const. I have expected this. 

> const gMozmill-Delay
> const gMozmill-Timeout

We should stick with the current implementation. I don't think we will run in trouble without a prefix.

Follow-up landed as http://hg.mozilla.org/qa/mozmill-tests/rev/a65f34c35cd5