Closed Bug 724713 Opened 8 years ago Closed 8 years ago

Update Mozmill Shared Modules to remove deprecated assertJS/assert calls

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ashughes, Assigned: AlexLakatos)

References

Details

(Whiteboard: [lib][mozmill-test-refactor])

Attachments

(2 files, 1 obsolete file)

Before we can refactor the tests for bug 586286, we need to refactor the shared modules to remove deprecated assertJS calls.
Whiteboard: [mozmill-test-refactor]
Blocks: 586286
I think we should always depend on the assertions module. Anthony, who would be able to work on it?
Summary: Update Mozmill Shared Modules to remove deprecated assertJS calls → Update Mozmill Shared Modules to remove deprecated assertJS/assert calls
(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.
Taking it and starting to work on it. Should I always use assert? Who should be my reviewer?
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
Not always. If we are accessing properties please stick to the appropriate methods for now.
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.
Attached patch patch v1.0 (obsolete) — Splinter Review
After I got back from PTO got some merge conflicts, took a while for solving them and retesting.
Attachment #603220 - Flags: review?(hskupin)
Comment on attachment 603220 [details] [diff] [review]
patch v1.0

Please use Geo or Dave for any shared module related review requests this week.
Attachment #603220 - Flags: review?(hskupin) → review?(gmealer)
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.
Attachment #603220 - Flags: review?(gmealer) → review-
(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.
(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.
Attachment #603220 - Attachment is obsolete: true
Attachment #607883 - Flags: review?(gmealer)
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+
Attachment #607883 - Flags: review?(gmealer) → review+
Geo, do you also plan to land this patch across branches?
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"
(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.
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #607883 - Attachment description: patch v2.0 → patch v2.0 [landed:default]
(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.
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.
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.
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.
Keywords: checkin-needed
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.
Yeah, this should also be landed on ESR since it will need this for Mozmill-2.0.

I can land this for you Geo.
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.
Patch for the esr branch
Attachment #609702 - Flags: review?(anthony.s.hughes)
Comment on attachment 609702 [details] [diff] [review]
patch v2.0 (esr) [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/a24c08397f63 (mozilla-esr10)
Attachment #609702 - Attachment description: patch v2.0 [mozilla-esr10] → patch v2.0 (esr) [landed]
Attachment #609702 - Flags: review?(anthony.s.hughes) → review+
Keywords: checkin-needed
Component: Mozmill Shared Modules → Mozmill Tests
Whiteboard: [mozmill-test-refactor] → [lib]
Whiteboard: [lib] → [lib][mozmill-test-refactor]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.