1016246 - Turn widgets.js functions into class

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Daniel Gherasim]

We should turn widgets.js functions into a class.

https://hg.mozilla.org/qa/mozmill-tests/file/e386857a0a2b/firefox/lib/widgets.js

Comment 1

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

Andrei, I assume you can take it as mentor.

Comment 2

[Franz Sarmiento]

Hello, I am interested in fixing this bug. Being tagged as a "good first bug", I feel that this is appropriate for me since I'm new to contributing to open source projects.

Right now I'll clone the Mozmill repo to examine the code.

Comment 3

[Franz Sarmiento]

Before I extract a patch and make it mercurial-compatible, I want to make sure I understand the problem correctly. Here is a draft of the patch which I pushed to my fork on GitHub:
https://github.com/franzks/qa-mozmill-tests/commit/37bc5a211c24d57fd9157b648308ec8a4719a8b5

Is this correct approach?

Comment 4

[Andrei Eftimie]

Hi Franz,
Thanks for working on this. The draft is a good start.

As a general pattern we use a constructor function where we specify some initial needed values.
(This usually is the controller from which we operate on. I'm wondering if in this case it would be a good ideea to pass the tree object as well at construction time.)

You can have a look at any other library file (from the same folder). For how they are usually structured.

Besides the class itself you will also have to search for any instance of code that uses it and update accordingly (a quick search reveals this one is used in 3 places).

Comment 5

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

Just to note here... the class which needs to be constructed will be totally related to trees. So its name should visualize it. In the future there may also be other classes covered by this module.

Comment 6

[Franz Sarmiento]

Hi Andrei and Henrik, let me just put it out there that this is my first time diving into other people's codebase as large as this one. Quite frankly I am a bit overwhelmed, but nonetheless I am determined to finish this. As for your feedback:

-As for the constructor function, change is found here: https://github.com/franzks/qa-mozmill-tests/commit/f3227e0b56ffc20b87c26ae6da9cbd8677a2d8a6
Regarding whether the tree object should be passed at construction time, I am still wrapping my head about how this whole thing works so I can't make any judgements on that. In the meantime, all I am passing is the controller. I'll refactor to include the tree object in the parameter list the moment you say so.

-As for its usage in other instances, I did a grep and discovered this is referenced in search.js., sessionstore.js, and selenium.js. Change is here: https://github.com/franzks/qa-mozmill-tests/commit/d663d6a4460e74625bbff86eb26b3a10e6009a3e
I am unsure whether what I did was correct to simply create the object at the same time as the clickTreeCell function is needed, or I should create it beforehand as a member of the class it's used in.

-As per Henrik's comment, in the meantime I have called the class "treeHandler" due to reading the comments. But as I've said, I'm still trying to wrap my head about how the whole thing works so I feel this may not be the best choice for the variable name.

Also, my other concern is I can't get any tests to fail if the widgets.js module is broken. I did a grep to see where classes that use clickTreeCell() are created, and I did another grep to see which tests those classes are referenced in. After running the tests grep has found, I didn't discover any of them which failed due to a broken widgets module.

Please forgive my inexperience, I am quite intent on making myself useful to the open source community in the future once I learn the ropes.

Comment 7

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

Franz, I'm sorry that Andrei as mentor didn't get back to you regarding this work. I'm just wading through all old bugs and this one popped out to me. :(

Meanwhile the Mozmill project is dead and everything has been replaced with Marionette. Means also existing tests have been partly converted from JS to Python. The following document gives a much clearer picture: https://developer.mozilla.org/en-US/docs/Mozilla/QA/firefox-ui-tests

I know that nearly two years passed by but if you would still have interest to learn something more about test automation let us know. Please keep in mind that I'm away the next 4 weeks, so a response might take a while. Thanks and sorry again for not replying to your last comment.