755552 - marionette status() method throws error "unrecognizedPacketType"

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[David Burns :automatedtester]

After creating a session I checked the status and got an error

>>> driver.start_session()                                                                                                      
u'363'
>>> driver.get_windows()
[u'1', u'335']
>>> driver.status()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/davidburns/.virtualenvs/seleniumproxy/lib/python2.7/site-packages/marionette-0.2-py2.7.egg/marionette/marionette.
py", line 202, in status
    return self._send_message('getStatus', 'value')
  File "/Users/davidburns/.virtualenvs/seleniumproxy/lib/python2.7/site-packages/marionette-0.2-py2.7.egg/marionette/marionette.
py", line 167, in _send_message
    self._handle_error(response)
  File "/Users/davidburns/.virtualenvs/seleniumproxy/lib/python2.7/site-packages/marionette-0.2-py2.7.egg/marionette/marionette.
py", line 196, in _handle_error
    raise MarionetteException(message=response, status=500)
marionette.errors.MarionetteException: {u'message': u'Actor "conn2.marionette1" does not recognize the packet type "getStatus"',
 u'from': u'conn2.marionette1', u'error': u'unrecognizedPacketType'}
>>>

Comment 1

[Malini Das [:mdas] - Away, not checking bugmail]

This is not yet implemented

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

Notes to get started:

Get a debug firefox build. These builds have marionette built into them. Go to: ftp://ftp.mozilla.org/pub/firefox/tinderbox-builds/, then click mozilla-central-(YOUR DEV ENVIRONMENT)-debug, then on the next page, click the latest build, and finally, download it.

You should create a new profile for testing. Follow the steps here: http://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles to create a fresh profile.

Once you do that, start the browser and go to about:config. You'll have to set a preference to get marionette working. Right click and create a new Boolean preference. Name it "marionette.defaultPrefs.enabled" and set it to true. Now restart the browser for this to take effect.

Next, once you get the mozilla-central code, go into (mozilla-central)/testing/marionette/client/marionette and run:

sh venv_test.sh (PATH TO PYTHON 2.7)

This will create a virtual environment which will contain all the dependencies for running marionette.

Next, activate that virtual environment:

cd ../marionette_venv
. bin/activate

Now you can run an example test:

cd ../marionette/
python runtests.py --address=localhost:2828 tests/unit/test_log.py

If that passes, then you can start with the bug!

To get the error described in the bug, open an interactive python shell (while the virtual environment is still active!) and execute:

from marionette import Marionette 
client = Marionette(host='localhost', port=2828)
client.start_session()
client.status()

It should cause this error. This happens because the getStatus call (made from the python client to the marionette server running in the browser) is not implemented on the server side. You'll need to add this support to the (mozilla-central)/testing/marionette/marionette-actors.js file. The MarionetteDriverActor.prototype.requestTypes variable is what dispatches client requests to functions, so you'll need to add a new requestType for getStatus, and create the function that will handle it.

The expected json response should be:
 {
     "value": {
         "os": {
             "arch": ,
             "name": ,
             "version": 
         },
         "build": {
             "revision": ",
             "time": ,
             "version": ,
         }
     }
 }

we can tackle how to get the os and build information once we get to that point!

Comment 3

[Alfredos-Panagiotis Damkalis [:fredy]]

Attachment: first patch for bug fixing

This is my first patch.

I have tested it in python interactive mode with a b2g device and with nightly firefox.

All the values returned are 'unknown' except the os.name which is Services.appinfo.OS. I got this value from the similar getSessionCapabilities function.

Is the build info about gecko or something else?

Comment 4

[Jonathan Griffin (:jgriffin)]

Comment on attachment 666606 [details] [diff] [review]
first patch for bug fixing

Resetting review to mdas since I realized she's the designated mentor.

Comment 5

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 666606 [details] [diff] [review]
first patch for bug fixing

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

Good first try, you understand the concept well!

However, instead of returning 'unknown' for the fields, you should return what information you can. For the build information, you can get the 'time' and 'version' from the appinfo object (https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Shared_Modules/UtilsAPI/appInfo), and for the OS section, you're getting the 'name' field correctly, but you can also get the architecture field. This can be found looking at xulRuntime's XPCOMABI member (https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIXULRuntime). If it does not exist, you can send back 'unknown'. The rest can remain 'unknown'.

Comment 6

[Alfredos-Panagiotis Damkalis [:fredy]]

(In reply to Malini Das [:mdas] from comment #5)
> For the build information, you can get the 'time'
> and 'version' from the appinfo object
> (https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Shared_Modules/
> UtilsAPI/appInfo)

I've checked the link above. There are two types of version and time. The first is about the XUL app and the second about Gecko. Which one of these is our case?

Testing both on b2g device there was no difference at the version but there was at time. The appinfo.platformBuildID gave me the time of Gecko which had been built 3 days ago. The appinfo.version was the time of the latest build which was today (with only change to be the marionette-actor.js file).

Thanks for the information.

Comment 7

[David Burns :automatedtester]

There is some overlap with getSessionCapabilities[1]. It might be worth refactoring to get what we need in status and minimise duplicated code.

[1] http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-actors.js#465

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Alfredos-Panagiotis Damkalis (irc: fredy) from comment #6)
> (In reply to Malini Das [:mdas] from comment #5)
> > For the build information, you can get the 'time'
> > and 'version' from the appinfo object
> > (https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Shared_Modules/
> > UtilsAPI/appInfo)
> 
> I've checked the link above. There are two types of version and time. The
> first is about the XUL app and the second about Gecko. Which one of these is
> our case?

Ah for our case 'version' will be version and for 'time' it will be platformBuildID

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to David Burns :automatedtester from comment #7)
> There is some overlap with getSessionCapabilities[1]. It might be worth
> refactoring to get what we need in status and minimise duplicated code.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-
> actors.js#465

Hm, they only really share the OS and version information, and even then, they keys are named differently. I don't think that adding some shared code would really help much here, but maybe I'm missing something?

Comment 10

[Alfredos-Panagiotis Damkalis [:fredy]]

Attachment: second patch

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 668772 [details] [diff] [review]
second patch

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

great! I just tested this and it works as expected. If you're willing to clear up the whitespace issues and resubmit, that would be appreciated.

I just realized that we should add a test for this as well. Are you interested in writing the unit test for this?

Our unit tests live in (mozilla-central)/testing/marionette/client/marionette/tests/unit/. A good unit test would be one that calls the status() method, verifies that no error is received, and checks to see that the returned dict has the expected keys in it('build', 'time', etc.). As an example, you can look at the first few lines (lines 0-14) of test_getattr.py, where we create a MarionetteTestCase which provides a marionette instance that you can use.

::: testing/marionette/marionette-actors.js
@@ +480,5 @@
>      };
>  
>      this.sendResponse(value);
>    },
> +  

extra whitespace here

@@ +501,5 @@
> +            'revision': 'unknown',
> +            'time': Services.appinfo.platformBuildID,
> +            'version': Services.appinfo.version
> +          }
> +    };          

Whitespace should be removed after the ;

Comment 12

[Alfredos-Panagiotis Damkalis [:fredy]]

Attachment: patchv2 with corrected whitespaces

fix previous patch whitespaces

Comment 13

[Alfredos-Panagiotis Damkalis [:fredy]]

(In reply to Malini Das [:mdas] from comment #11)
> Comment on attachment 668772 [details] [diff] [review]
> second patch
> 
> Review of attachment 668772 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> great! I just tested this and it works as expected. If you're willing to
> clear up the whitespace issues and resubmit, that would be appreciated.

done

> 
> I just realized that we should add a test for this as well. Are you
> interested in writing the unit test for this?
> 

Yes, I am going to write this unit test.

Should I open a new bug about it or continue here?

Comment 14

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Alfredos-Panagiotis Damkalis (irc: fredy) from comment #13)
> Yes, I am going to write this unit test.
> 
> Should I open a new bug about it or continue here?

Yes, I'd continue it here, and I'll land both at the same time.

Comment 15

[Alfredos-Panagiotis Damkalis [:fredy]]

Attachment: getStatus unit test

Comment 16

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 671108 [details]
getStatus unit test

Great, the test itself is perfect, the only issues with the patch are 

a) there are tabs instead of spaces, so please remove the tabs and replace them with spaces, otherwise this won't run as is :( 

b) please add the test to the repository under testing/marionette/client/marionette/tests/unit/ (in command line: 'hg add <file>'), then export your changes. The usual procedure we follow in mozilla is to use mercurial queues (https://developer.mozilla.org/en/Mercurial_queues), where you create your path, make your changes, save the patch, and export it. This way, what I review is the exact patch that can be safely applied to the mozilla-central code.

Once these are addressed, I'll r+ the patch and land them:) Thanks for the great work!

Comment 17

[Alfredos-Panagiotis Damkalis [:fredy]]

Attachment: bug fix and unit test

Thanks for the review.

I hope this patch is ok :)

Comment 18

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 671662 [details] [diff] [review]
bug fix and unit test

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

Excellent, thanks so much for your work!

I will add this test to the list of tests we run, and get this landed.

Comment 19

[Malini Das [:mdas] - Away, not checking bugmail]

pushed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9539b74e037

Thanks again, fredy!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b9539b74e037

Comment 21

[Jonathan Griffin (:jgriffin)]

Landed in aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/13af04015a14