Closed Bug 755552 Opened 8 years ago Closed 7 years ago

marionette status() method throws error "unrecognizedPacketType"

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: automatedtester, Assigned: fredy)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=mdas][lang=js])

Attachments

(1 file, 4 obsolete files)

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'}
>>>
This is not yet implemented
Blocks: webdriver
Whiteboard: [good first bug][mentor=mdas][lang=js]
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!
Attached patch first patch for bug fixing (obsolete) — Splinter Review
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?
Attachment #666606 - Flags: review?
Attachment #666606 - Flags: review? → review?(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.
Attachment #666606 - Flags: review?(jgriffin) → review?(mdas)
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'.
Attachment #666606 - Flags: review?(mdas) → review-
(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.
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
(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
(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?
Attached patch second patch (obsolete) — Splinter Review
Attachment #666606 - Attachment is obsolete: true
Attachment #668772 - Flags: review?(mdas)
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 ;
Attachment #668772 - Flags: review?(mdas) → review+
fix previous patch whitespaces
Attachment #668772 - Attachment is obsolete: true
Attachment #670361 - Flags: review?(mdas)
(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?
Attachment #670361 - Flags: review?(mdas) → review+
(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.
Attached file getStatus unit test (obsolete) —
Attachment #671108 - Flags: review?(mdas)
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!
Attachment #671108 - Flags: review?(mdas) → review-
Thanks for the review.

I hope this patch is ok :)
Attachment #670361 - Attachment is obsolete: true
Attachment #671108 - Attachment is obsolete: true
Attachment #671662 - Flags: review?(mdas)
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.
Attachment #671662 - Flags: review?(mdas) → review+
pushed to mozilla-inbound:

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

Thanks again, fredy!
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/b9539b74e037
Assignee: nobody → fredy
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.