1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

Bug 1122187 added a wrapper around Services.appinfo to access certain information. If we want access to other members of appinfo in our tests, we could add a more general facility for mirroring JavaScript objects in our testing libraries. I think this could be a reasonable place to use Python's __getattr__ (Python's answer to "method_missing", see [1]), so long as attempts to access members that aren't in Services.appinfo are appropriately handled with an AttributeError.

[1] https://docs.python.org/2/reference/datamodel.html#object.__getattr__

Comment 1

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

The code Chris is referring to can be found at:
https://github.com/mozilla/firefox-ui-tests/blob/master/firefox_puppeteer/api/appinfo.py

Something we additionally would like to have are e.g. the locale and the XPCOMABI values. References for those can be found here:

http://hg.mozilla.org/qa/mozmill-tests/file/b21a5be35480/lib/utils.js#l23
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests/Shared_Modules/UtilsAPI/appInfo

Comment 2

[Ankit Goyal]

I would like to work on this. Would you elaborate more?

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

This would be a great but to start with on this project.

I would start with getting the firefox-ui-tests repo from github:
https://github.com/mozilla/firefox-ui-tests

the link that Henrik pointed to is good information to ensure you read up on.  All of the values that utils.js has for appInfo (as mentioned in the links above) should be accessible from the firefox-ui-tests/.../appinfo.py file.

Ideally we would have some tests that would validate we can query these values.  I am sure I overlooked something obvious, so ask questions if something isn't clear.

Comment 4

[Ankit Goyal]

I am getting this error in all the tests:
AttributeError: 'Marionette' object has no attribute 'current_chrome_window_handle'

Did I miss something in the setup?

Comment 5

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

Ankit, are you on the latest version of marionette-client? It should be 0.87 as listed here:
https://github.com/mozilla/firefox-ui-tests/blob/master/setup.py#L9

Also please ensure to test with latest Nightly and not an Aurora, Beta, or Release of Firefox.

Comment 6

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

Oh, and thanks for your interest to work on this bug!

Comment 7

[Ankit Goyal]

Oops, my marionette-client was outdated.
I am studying code at the moment. Would start working soon.

Comment 8

[Ankit Goyal]

I don't know, it may be of relevance or not but setup.py contain only `install_requires` dependencies.
It should also contain `setup_requires` for specific dependencies.

Comment 9

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

Not sure what setup_requires is, and I have a hard time to find any reference to that. Can you please elaborate?

Comment 10

[Ankit Goyal]

As mentioned in docs at https://github.com/mozilla/firefox-ui-tests/, to setup the firefox-ui-tests we have to run the following command: `python setup.py develop`

Now we have `install_requires` kwargs at https://github.com/mozilla/firefox-ui-tests/blob/master/setup.py#L33, which will get these dependencies(https://github.com/mozilla/firefox-ui-tests/blob/master/setup.py#L10) only when we execute `python setup.py install`.

To support these explicit dependencies for `python setup.py develop`, we need to use another kwargs `setup_requires`.

That's why I was getting outdated version of marionette-client.

The code will look something like this at https://github.com/mozilla/firefox-ui-tests/blob/master/setup.py#L33:

>> ip_safe=False,
>> install_requires=deps,
>> setup_requires=deps,
>> entry_points="""

Comment 11

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

No, the problem was that we haven't released a version of marionette-client for a long time, and API breaking changes in Marionette were introduced. For a faster development pace we were relying on a marionette-client version in mozilla-central (http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/). But given and as your example shows this was a problem, so we released version 0.87 lately. 

The reason why you got 0.86 was that our setup.py had still a dependency to marionette-client 0.86 listed, but the version in mozilla-central was bumped to 0.87. So even you tried to install the client from that location, it would have been downgraded when running setup.py for firefox-ui-tests. But since we have uploaded version 0.87 to pypi all is fine.

Comment 12

[Ankit Goyal]

Henrik, a dumb question. How to use pupetteer?

Comment 13

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

It looks like Chris missed to add a unit test for it. So lets give some examples how to use it. As best is if you take another test as template for the required appinfo unit test. All the unit tests you can find here: https://github.com/mozilla/firefox-ui-tests/tree/master/firefox_puppeteer/tests

I would suggest you check test_prefs.js which is also a unit test for an API module. Via puppeteer we automatically create a class instance and attach it to the FirefoxTestCase class. So to access the appinfo data you have to use `self.appinfo`, which is an instance of AppInfo as it can be found here:

https://github.com/mozilla/firefox-ui-tests/blob/master/firefox_puppeteer/api/appinfo.py#L8

To only run the unit tests you can pass in the path to firefox_puppeteer/tests/manifest as argument of the firefox-ui-tests command line client.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

thanks :whimboo for helping out here!

Comment 15

[Ankit Goyal]

Attachment: Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8558331 [details] [diff] [review]
Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

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

Formatting is important, it helps ensure the code is easy to read and it is maintainable!

In addition to the spacing issues mentioned below, We really should have unit tests for this.  Looking at the firefox-ui-tests repository, I don't see unittests already existing for the code it appears the test specific stuff is related to actually using the code for testing Firefox.

Lets get input from :whimboo about what type of unittest framework we should have or if there is one in the works we can wait.  Ideally we could write a series of unittests to test these conditions.

::: firefox_puppeteer/api/appinfo.py
@@ +8,5 @@
>  class AppInfo(BaseLib):
>  
> +    def __getattr__(self, attr):
> +		with self.marionette.using_context('chrome'):
> +            value = self.marionette.execute_script("""

please be very careful about your spacing here.  This file is currently 2 space indentation, please ensure you indent 2 spaces (not tabs, not more than 2).

@@ -8,5 @@
>  class AppInfo(BaseLib):
>  
> -    @property
> -    def browserTabsRemoteAutostart(self):
> -        return self._get_property("browserTabsRemoteAutostart")

does anybody call this?  If so, code will need to be modified

@@ +19,5 @@
> +
> +			if value:
> +				return value
> +			else:
> +				raise AttributeError()

the spacing here for the if condition needs to be 2 spaces as well.  These appear to be tabs.  Quite possibly you could ensure that your editor puts spaces in the code instead of tabs when you enter the TAB key.

Comment 17

[Ankit Goyal]

I will address the issue with spaces.
`browserTabsRemoteAutostart` is a property of AppInfo, which is also coming from `Services.appinfo`.

So __getattr__ will take of this too.

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks Ankit.  It would be nice to have some unittests verify we can get all the AppInfo properties :)  We can wait for :whimboo to comment on how those should look.  

Is there any existing code that uses browserTabsRemoteAutostart?  If so, it would need to be updated, right?

Comment 19

[Ankit Goyal]

Joel, there is no need for updation.

After applying patch, if you call appinfo.browserTabsRemoteAutoStart, it will call __getattr__(), which will execute Service.appinfo['appinfo.browserTabsRemoteAutoStart'].

Earlier, if you call appinfo.browserTabsRemoteAutoStart, it will call _get_property(), which will execute Service.appinfo['appinfo.browserTabsRemoteAutoStart'].

So the end result is same as before.

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Ankit great!  If you could update your patch to fix the tabs to be spaces, that would be good

Comment 21

[Ankit Goyal]

Attachment: Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8558469 [details] [diff] [review]
Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

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

ok, I messed this up.  We should have 4 space indentation, but I saw the file had 2 space (reading the javascript).  So lets put it back to 4 space indentation.  Otherwise this looks good.

Comment 23

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

Comment on attachment 8558469 [details] [diff] [review]
Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

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

I think that looks fine, so f+ from me. But as Joel already mentioned please add a new test for it. It has to be similar as test_prefs.py which you can find under firefox_puppeteer/tests. Also we need a doc string for the AppInfo class, which explains what it is doing. Best would be if you could add a reference to the appinfo documentation on MDN (https://developer.mozilla.org)

Comment 24

[Ankit Goyal]

How the values have to be validated? Do i have to assert against None values only or validate if the values are correct?

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

Ankit, The values could be different based on the platform, buildtype.  I would make a best effort to get the values you get as well as verifying the values are not None.

Comment 26

[Ankit Goyal]

I could use mozversion to verify values, but that would requires access to `--binary`. So how can I access `--binary` cmd line arg in my test case?

Comment 27

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

We cannot compare all of the values we get via appinfo with output from mozversion. So not sure if we should require such deep tests. At least what would be necessary is to test that for any kind of property we are requesting, data is returned and no exception raised. Also add a negative test with an unknown property, so we can ensure to raise the correct exception type.

In case if you want to add some checks for mozversion, you can get the binary via `self.marionette.bin` inside the unit test.

Comment 28

[Ankit Goyal]

Attachment: Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8559071 [details] [diff] [review]
Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

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

::: firefox_puppeteer/tests/test_appinfo.py
@@ +33,5 @@
> +                if mozversion_attr in version_info:
> +                    self.assertEqual(self.appinfo.__getattr__(attr), version_info[mozversion_attr])
> +
> +    def test_check_exception(self):
> +        self.assertRaises(AttributeError, getattr, self.appinfo, 'unknown')

I am glad you have a test for the unknown here.

Comment 30

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

Comment on attachment 8559071 [details] [diff] [review]
Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

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

Ankit, this version looks good. Even when I have added a couple more comments. :) Most of them are for improvements and clearer code. Beside those I also would propose that you have a look at the documentation for this module under firefox_puppeteer/docs/api/appinfo.rst. Maybe you can add a bit more info in how this module can be used? An example might also be good.

::: firefox_puppeteer/api/appinfo.py
@@ +6,5 @@
>  
>  
>  class AppInfo(BaseLib):
> +    """
> +    This class provides access to various attributes of AppInfo.

nit: please move this sentence up one line right behind the triple quotes, and keep an empty line afterward to separate it from the detailed description.

@@ +24,5 @@
> +
> +            if value:
> +                return value
> +            else:
> +                raise AttributeError()

Please add a message too which should have the same format as the original exception. We have to know which attribute and which class caused it.

::: firefox_puppeteer/tests/test_appinfo.py
@@ +7,5 @@
> +
> +from firefox_ui_harness.testcase import FirefoxTestCase
> +
> +
> +class testAppinfo(FirefoxTestCase):

nit: TestAppInfo

@@ +10,5 @@
> +
> +class testAppinfo(FirefoxTestCase):
> +
> +    def setUp(self):
> +        FirefoxTestCase.setUp(self)

If you do not have to setup anything yourself, you can omit this method.

@@ +12,5 @@
> +
> +    def setUp(self):
> +        FirefoxTestCase.setUp(self)
> +
> +    def test_compare_info(self):

maybe test_valid_properties?

@@ +17,5 @@
> +        binary = self.marionette.bin
> +        version_info = mozversion.get_version(binary=binary)
> +
> +        attr_mapping = {'buildID': 'application_buildid',
> +            'ID': 'application_id',

This and the following lines should start right under the first single quote from the above line.

@@ +26,5 @@
> +            'platformVersion': 'platform_version'}
> +
> +        for attr, mozversion_attr in attr_mapping.iteritems():
> +            try:
> +                self.assertRaises(AttributeError, getattr, self.appinfo, attr)

This won't raise an exception, given that the attr name you specify here is valid, so you can just call it directly and can remove the try/except completely.

@@ +30,5 @@
> +                self.assertRaises(AttributeError, getattr, self.appinfo, attr)
> +            except AssertionError:
> +                self.assertIsNotNone(self.appinfo.__getattr__(attr))
> +                if mozversion_attr in version_info:
> +                    self.assertEqual(self.appinfo.__getattr__(attr), version_info[mozversion_attr])

You should not call the __getattr__ method directly. Instead use self.appinfo.ID and others. Also a single retrieval should be enough, which you can compare with the value from mozversion. The above hash ensures that for each of those we have a valid entry on both sides.

Beside the above entries I would suggest you also test the others and do just a assertNotNone check.

@@ +32,5 @@
> +                self.assertIsNotNone(self.appinfo.__getattr__(attr))
> +                if mozversion_attr in version_info:
> +                    self.assertEqual(self.appinfo.__getattr__(attr), version_info[mozversion_attr])
> +
> +    def test_check_exception(self):

Similar as above but 'test_invalid_properties'?

@@ +33,5 @@
> +                if mozversion_attr in version_info:
> +                    self.assertEqual(self.appinfo.__getattr__(attr), version_info[mozversion_attr])
> +
> +    def test_check_exception(self):
> +        self.assertRaises(AttributeError, getattr, self.appinfo, 'unknown')

Similar as above. Please do not call getattr directly, but use the property variant.

Comment 31

[Ankit Goyal]

Attachment: Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

Comment 32

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

Comment on attachment 8561351 [details] [diff] [review]
Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

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

This looks way better! Thank you for the update! Beside my inline comments, we would also have to add code for retrieving the following properties which are not served by appInfo directly:

* locale (http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/utils.js#l89)
* userAgent (http://hg.mozilla.org/qa/mozmill-tests/file/default/lib/utils.js#l100)

They would need their own property definition and tests for not None. When doing that please also add such a not None check for appinfo[XPCOMABI]. Thanks.

::: firefox_puppeteer/api/appinfo.py
@@ +16,3 @@
>          with self.marionette.using_context('chrome'):
> +            value = self.marionette.execute_script("""
> +              return Services.appinfo[arguments[0]];

Can you please explain why you have removed the the try/catch? In case of an unknown property a Javascript exception will be thrown, which we do not catch anymore now.

You can keep that but then we will have to catch the Javascript exception in the python code. You can no longer have the if/else block below, but it should be try/except. The exception to catch would be JavascriptException, which is provided by marionette.errors.

::: firefox_puppeteer/tests/test_appinfo.py
@@ +23,5 @@
> +        self.assertIsNotNone(self.appinfo.version)
> +        self.assertEqual(self.appinfo.version, version_info['application_version'])
> +        self.assertIsNotNone(self.appinfo.platformBuildID)
> +        self.assertEqual(self.appinfo.platformBuildID, version_info['platform_buildid'])
> +        self.assertIsNotNone(self.appinfo.platformVersion)

This line is obsolete and can be removed.

@@ +28,5 @@
> +        self.assertEqual(self.appinfo.platformVersion, version_info['platform_version'])
> +
> +    def test_invalid_properties(self):
> +        with self.assertRaises(AttributeError):
> +            self.appinfo.unknown

Please do it in the form: `self.assertRaises(AttributeError, self.appinfo.unknown)`.

Comment 33

[Ankit Goyal]

For getting userAgent, I need access to mozmill. How do do that?


>          with self.marionette.using_context('chrome'):
> +            value = self.marionette.execute_script("""
> +              return Services.appinfo[arguments[0]];

Services.appinfo is not throwing any exception, if an unknown argument is passed. It just returns null. May be JS implementation need some changes.


Which specific line is obsolete?


> self.assertRaises(AttributeError, self.appinfo.unknown)
This will raise error. assertRaises require a function. That's why I was using __getattr__ directly earlier.

Comment 34

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

(In reply to Ankit Goyal from comment #33)
> For getting userAgent, I need access to mozmill. How do do that?

The first line of that function simply gets the most recent browser window. But thinking more about it to make it more stable, especially in case if no browser window is open, I found another good way to get this information. The HTTPProtocolHandler provides it for us for free:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/UserAgentOverrides.jsm#18

Just take those three lines and put them into a execute_script() call. 

> >          with self.marionette.using_context('chrome'):
> > +            value = self.marionette.execute_script("""
> > +              return Services.appinfo[arguments[0]];
> 
> Services.appinfo is not throwing any exception, if an unknown argument is
> passed. It just returns null. May be JS implementation need some changes.

Oh cool! So I definitely support that change. Thanks!

> Which specific line is obsolete?

`self.assertIsNotNone(self.appinfo.platformVersion)` because in the next line you test for the specific value against mozinfo.

> > self.assertRaises(AttributeError, self.appinfo.unknown)
> This will raise error. assertRaises require a function. That's why I was
> using __getattr__ directly earlier.

I see. So it's fine then to use the context manager.

Let me know if you have further questions or if the above information is clear enough for you.

Comment 35

[Ankit Goyal]

Attachment: Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

Comment 36

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

Comment on attachment 8562646 [details] [diff] [review]
Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

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

Clearly a f+! This code looks great. Just get the remaining nits fixed and ask for a final review.

Ankit, if you want please also join us on IRC in the #automation channel. You may find it interesting to learn even more given that firefox-ui-tests are the #1 topic in our channel those days. Thanks for working on it!

::: firefox_puppeteer/api/appinfo.py
@@ +18,5 @@
>                try {
>                  return Services.appinfo[arguments[0]];
>                } catch (e) {
>                  return null;
>                }

Oh, you added this back? As we agreed on in my last comment we do not need the try/catch. Or is there a specific reason that you added it back?

@@ +30,5 @@
> +    @property
> +    def locale(self):
> +        with self.marionette.using_context('chrome'):
> +            return self.marionette.execute_script("""
> +              var registry = Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry);

nit: this line is getting a bit too long. You can wrap before .getService and align the dot with the `C` of `Cc`.

@@ +35,5 @@
> +              return registry.getSelectedLocale("global");
> +            """)
> +
> +    @property
> +    def userAgent(self):

Please name that property user_agent. In Python the camel-case notation is not used.

@@ +40,5 @@
> +        with self.marionette.using_context('chrome'):
> +            return self.marionette.execute_script("""
> +              return Cc["@mozilla.org/network/protocol;1?name=http"].
> +                        getService(Ci.nsIHttpProtocolHandler).
> +                        userAgent;

Please put the trailing dots always in the next line so that it is visible that the command continuous. As mentioned above align it with the `C`.

Comment 37

[Ankit Goyal]

Attachment: Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

Comment 38

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

Comment on attachment 8564046 [details] [review]
Bug 1123758 - Provide access to more members of Services.appinfo in firefox puppeteer libraries. r=whimboo

All looks great now. Thanks for working on that!

Comment 39

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

PR has been merged as:
https://github.com/mozilla/firefox-ui-tests/commit/7b2ccef3632b21251c0ea0cee20ee17843ae2185