724713 - Update Mozmill Shared Modules to remove deprecated assertJS/assert calls

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Before we can refactor the tests for bug 586286, we need to refactor the shared modules to remove deprecated assertJS calls.

Comment 1

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

I think we should always depend on the assertions module. Anthony, who would be able to work on it?

Comment 2

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> I think we should always depend on the assertions module. Anthony, who would
> be able to work on it?

No, I personally have no time for it. I've requested Vlad to assign and prioritize it.

Comment 3

[Alex Lakatos[:AlexLakatos]]

Taking it and starting to work on it. Should I always use assert? Who should be my reviewer?

Comment 4

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

Not always. If we are accessing properties please stick to the appropriate methods for now.

Comment 5

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

Alex, can we please make progress on this bug? If you can't work on it can Remus help out? We have to hurry up because we need our modules and tests in a state when we can use them with Mozmill 2. Thanks.

Comment 6

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

After I got back from PTO got some merge conflicts, took a while for solving them and retesting.

Comment 7

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

Comment on attachment 603220 [details] [diff] [review]
patch v1.0

Please use Geo or Dave for any shared module related review requests this week.

Comment 8

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

Comment on attachment 603220 [details] [diff] [review]
patch v1.0

Sorry this sat. I somehow missed the Bugzilla mail.

lib/software-update.js, Line 234:

//      assert.notEqual(patch0URL, patch1URL, "Update snippet has the same URL");

...shouldn't that be that they don't have the same URL? Also, the whole thing says uncomment after Fx4 is released. Shouldn't we be doing that?

lib/utils.js, line 187:

assert.equal(visible, expectedVisibility, "Element is visible");

Probably fine, but because I can't see the rest of the function I'm concerned that expectedVisibility might sometimes be "false" (being a variable and all). 

If it's always true, I don't see why we'd put it in a variable. if it's not, then the messages should be more like "Element has expected visibility" or something.

r-'ing for the message change above, which I'm pretty sure is needed, but do me a favor and address the other points when you reply back.

Comment 9

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

(In reply to Geo Mealer [:geo] from comment #8)
> //      assert.notEqual(patch0URL, patch1URL, "Update snippet has the same
> URL");
> 
> ...shouldn't that be that they don't have the same URL? Also, the whole
> thing says uncomment after Fx4 is released. Shouldn't we be doing that?

The problem is that something is or was wrong with the code and I never got to it to check again. Not sure if we wanna uncomment it until the rewrite of the module re: background updates. Also it's outside of the scope of this work so not sure if we even should change that code when it's commented out.

Comment 10

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0 [landed]

(In reply to Geo Mealer [:geo] from comment #8)
> assert.equal(visible, expectedVisibility, "Element is visible");
> 
> Probably fine, but because I can't see the rest of the function I'm
> concerned that expectedVisibility might sometimes be "false" (being a
> variable and all). 
> 
> If it's always true, I don't see why we'd put it in a variable. if it's not,
> then the messages should be more like "Element has expected visibility" or
> something.
You are right, expectedVisibility is received as a parameter. Changed the message.


(In reply to Henrik Skupin (:whimboo) from comment #9)
> (In reply to Geo Mealer [:geo] from comment #8)
> > //      assert.notEqual(patch0URL, patch1URL, "Update snippet has the same
> > URL");
> > 
> > ...shouldn't that be that they don't have the same URL? Also, the whole
> > thing says uncomment after Fx4 is released. Shouldn't we be doing that?
> 
> The problem is that something is or was wrong with the code and I never got
> to it to check again. Not sure if we wanna uncomment it until the rewrite of
> the module re: background updates. Also it's outside of the scope of this
> work so not sure if we even should change that code when it's commented out.
I think it's better if we did the refactor now, even if it's commented code. Because when someone will uncomment the code in the future it is more likely to just uncomment it, not also remember to refactor it.

Comment 11

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

Comment on attachment 607883 [details] [diff] [review]
patch v2.0 [landed]

Looks good, thanks for the changes. I'm fine with refactoring inside the comment (really, could have gone either way).

r+

Comment 12

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

Geo, do you also plan to land this patch across branches?

Comment 13

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

Does Alex not have access?

https://developer.mozilla.org/en/Mozmill_Tests/Commit_Policy

"Once granted commit access you will be granted the following responsibilities:

    Landing your own patches after they have passed review (ideally within 24 hours of r+)
    Checking the live test results the following day to see if your patch introduced a regression
    Promptly backing out your patch when a regression is introduced
    Landing patches for those without commit access after they have passed review"

Comment 14

[Alex Lakatos[:AlexLakatos]]

(In reply to Geo Mealer [:geo] from comment #13)
> Does Alex not have access?
> 
My bug for Commit Access is not resolved yet, so I don't have access.

Comment 15

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

Ah, OK. Did not realize that.

Landed on default as http://hg.mozilla.org/qa/mozmill-tests/rev/30db8005511a

I'll doublecheck results tomorrow and will land across branches if no problems. Keeping [checkin-needed] until then.

Comment 16

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

(In reply to Geo Mealer [:geo] from comment #15)
> I'll doublecheck results tomorrow and will land across branches if no
> problems. Keeping [checkin-needed] until then.

Geo, can you please ensure to land the remaining stuff ASAP if nothing has been regressed? Thanks.

Comment 17

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

Apologies. I'm in the middle of a release and didn't have the cycles to look. Things looks fine, so I'll follow up with a commit.

Comment 18

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

Geo, thanks for the update. To make it easier you should probably use the transplant hg command. I have started to use it today and it is wonderful and makes things way easier.

Comment 19

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

Comment on attachment 607883 [details] [diff] [review]
patch v2.0 [landed]

Landed:

http://hg.mozilla.org/qa/mozmill-tests/rev/20dd60472749 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/43cea71fe86f (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/c66313f8dab8 (mozilla-release)

Comment 20

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

Geo, this patch also has to land on the ESR branch. I don't think it will apply to 1.9.2 so we should skip this.

Comment 21

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

You were pretty clear in a previous query about something similar that only fixes to failures land in ESR, so I'm surprised to hear that a simple refactoring fix would go there.

I won't have bandwidth to take care of the followup this today or tomorrow, so if it's important you should go ahead and do it. Otherwise, I'll revisit Wednesday.

Comment 22

[u279076]

Yeah, this should also be landed on ESR since it will need this for Mozmill-2.0.

I can land this for you Geo.

Comment 23

[u279076]

The patch fails to apply for toolbars.js and utils.js on mozilla-esr10.

Alex, please submit a patch for review for the esr branch.

Comment 24

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0 (esr) [landed]

Patch for the esr branch

Comment 25

[u279076]

Comment on attachment 609702 [details] [diff] [review]
patch v2.0 (esr) [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/a24c08397f63 (mozilla-esr10)