763056 - Remove controller.assert* methods as they are deprecated due to the assertions module

Status: RESOLVED WONTFIX

(Testing Graveyard :: Mozmill, defect)

Description

[Andrew Halberstadt [:ahal]]

Mozmill has a separate assertions module (see https://github.com/mozautomation/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/assertions.js)

There isn't much reason to keep the controller.assert* methods any longer. They should be removed.

Comment 1

[Andrew Halberstadt [:ahal]]

As part of this bug we'll need to go through and update all the tests that use controller.assert* (e.g mutt/mutt/tests/js/controller/dnd_content.js)

Comment 2

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

Lets do this for version 2.1. First lets see if we can find someone who is interested in doing that.

Comment 3

[Gilles Leblanc]

You can assign this one to me.

Comment 4

[Gilles Leblanc]

Attachment: bug763056.patch

This patch removes the deprecated assert functions in controller.js and also the logDeprecatedAssert function.

The logDeprecated (non-assert) is still there because there is still one other use in controller (__defineGetter__).

This patch also corrects the only use in the Mutt tests of the deprecated functions that I was able to find.

Comment 5

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

Thank you for taking this bug, Gilles! I will assign it to you now.

Comment 6

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

Comment on attachment 8405711 [details] [diff] [review]
bug763056.patch

Review of attachment 8405711 [details] [diff] [review]:
-----------------------------------------------------------------

Adding myself as reviewer for this patch. I will try to do this later today.

Comment 7

[Gilles Leblanc]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Comment on attachment 8405711 [details] [diff] [review]
> bug763056.patch
> 
> Review of attachment 8405711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Adding myself as reviewer for this patch. I will try to do this later today.

I never know if I should assign you as the reviewer. On one hand you have done all the previous reviews. On the other, maybe I should assign it to someone else so as not to overload you (you always seem to have a pretty long queue).

Comment 8

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

(In reply to Gilles Leblanc from comment #7)
> > Adding myself as reviewer for this patch. I will try to do this later today.
> 
> I never know if I should assign you as the reviewer. On one hand you have
> done all the previous reviews. On the other, maybe I should assign it to
> someone else so as not to overload you (you always seem to have a pretty
> long queue).

As of now we don't really have someone else for reviews in the mozmill component, and I'm happy to do those whenever possible. So just keep them going! :)

Comment 9

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

Comment on attachment 8405711 [details] [diff] [review]
bug763056.patch

Review of attachment 8405711 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Just a tiny thing you will have to follow-up before we can get this landed. Thanks Gilles!

::: mutt/mutt/tests/js/testController/testDndContentChrome.js
@@ +21,4 @@
>    controller.waitForPageLoad();
>  
>    let div = new elementslib.ID(controller.tabs.activeTab, "test-div");
> +  assert.ok(div.getNode(), "Found test-div.");

I would change this to 'assert.ok(div.exists(), ...)'.

Comment 10

[Gilles Leblanc]

Attachment: bug763056b.patch

Changed the assert in testDndContentChrome.js from .getNode to .exists following review.

Comment 11

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

Comment on attachment 8407264 [details] [diff] [review]
bug763056b.patch

Review of attachment 8407264 [details] [diff] [review]:
-----------------------------------------------------------------

Wonderful! All fine now, and I will check-in your patch in a moment.

Comment 12

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

Landed on master as part of the mozmill 2.1 release:
https://github.com/mozilla/mozmill/commit/fe59172863b706d8512a7b7fbdc44de177f51dfc

Good to see that those methods are finally gone. Thanks Gilles!

Comment 13

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

Sorry, but I had to revert this landing to get back a working testrun for our Mozmill tests due to bug 989298. We will re-land once we branched for the next Mozmill version, or got the mozmill tests fixed.

https://github.com/mozilla/mozmill/compare/da5d0291712b...42eb58bc1a12

Comment 14

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

Mozmill will reach its end of life soon. We are currently working on getting all the tests for Firefox ported to Marionette. For status updates please see bug 1080766.