Marionette does not type check the input of the `script_args` keyword argument. This means you can pass any type to it, but it should only accept arrays.

@:ato , Sir i would like to take this up.

Please read https://wiki.mozilla.org/User:Mjzffr/New_Contributors and attach your patch for review here.

07yogeshgupta, I would suggest that you read chapter 7) in the document Andreas pointed out. Best is to use mozreview for any changes you do, which will be patches and not full files. Thanks.

I have submitted the patch [here](https://reviewboard.mozilla.org/r/111766/) . Please let me know what else needs to done. :)

Comment on attachment 8836309 [details] Bug 1329184 - marionette.py : change script_args parameter of execute_async_script method from tuple to list https://reviewboard.mozilla.org/r/111766/#review113228 Something seems to have went wrong with uploading the diff. The files as contained here are not realted to the bug at all. Please check again, and get a correct patch uploaded. Thanks. Btw. if a patch is ready you want to request review. For this bug it will be :ato. You can do that directly from within mozreview, or by adding `r?ato` at the end of your commit message.

Comment on attachment 8836309 [details] Bug 1329184 - marionette.py : change script_args parameter of execute_async_script method from tuple to list https://reviewboard.mozilla.org/r/111766/#review113228 Sorry sir, to reply so late. I can see there are 3 files but i wanted to upload just `testing/marionette/client/marionette_driver/marionette.py` . can you please tell me if the changes in `marionette.py` file is correct? if not then can you please comment on the bugzilla page and explain the issue again in little more detail please. thank you :)

Comment on attachment 8836309 [details] Bug 1329184 - marionette.py : change script_args parameter of execute_async_script method from tuple to list https://reviewboard.mozilla.org/r/111766/#review115400 ::: .ycm_extra_conf.py:19 (Diff revision 1) > if not os.path.exists(path): > path = os.path.join(os.path.dirname(__file__), 'config.status') > - config = imp.load_module('_buildconfig', open(path), path, ('', 'r', imp.PY_SOURCE)) > + config = imp.load_module('_buildconfig', open( > + path), path, ('', 'r', imp.PY_SOURCE)) > path = os.path.join(config.topsrcdir, 'mach') > -mach_module = imp.load_module('_mach', open(path), path, ('', 'r', imp.PY_SOURCE)) > +mach_module = imp.load_module('_mach', open( > + path), path, ('', 'r', imp.PY_SOURCE)) > > sys.dont_write_bytecode = old_bytecode > > + Unrelated changes. ::: client.py:24 (Diff revision 1) > > topsrcdir = os.path.dirname(__file__) > if topsrcdir == '': > topsrcdir = '.' > > + All the changes to client.py are unnecessary whitespace changes as far as I can tell. ::: testing/marionette/client/marionette_driver/marionette.py:1792 (Diff revision 1) > "line": int(frame[1]), > "filename": os.path.basename(frame[0])} > rv = self._send_message("executeScript", body, key="value") > return self._from_json(rv) > > - def execute_async_script(self, script, script_args=(), new_sandbox=True, > + def execute_async_script(self, script, script_args=[], new_sandbox=True, Arguments should be given as tuples in Python. The type check should be made in the Marionette server, not the client: http://searchfox.org/mozilla-central/source/testing/marionette/driver.js#841

Comment on attachment 8836309 [details] Bug 1329184 - marionette.py : change script_args parameter of execute_async_script method from tuple to list https://reviewboard.mozilla.org/r/111766/#review115400 > Arguments should be given as tuples in Python. The type check should be made in the Marionette server, not the client: http://searchfox.org/mozilla-central/source/testing/marionette/driver.js#841 in `executeAsyncScript`, i cannot find `script_args` argument. there are `script` and `args` two arguments. so should i type check these two?

That's correct, and you may also want to include the timeout argument too.

Comment on attachment 8954154 [details] Bug 1329184 - Add script_args type checks to Python client. https://reviewboard.mozilla.org/r/223314/#review229466 Andreas, why do we have to add this check to the Python client? It should really be added to Marionette itself, given that I do not see any type checks for parameters in driver.js for both methods.

Comment on attachment 8954154 [details] Bug 1329184 - Add script_args type checks to Python client. https://reviewboard.mozilla.org/r/223314/#review229466 It would be fine to add type checks to the server too. I just checked, and they are missing. This bug arised from incorrect use of the client API in the past, so I don’t see why checking the type before it is sent is so bad?

Comment on attachment 8954154 [details] Bug 1329184 - Add script_args type checks to Python client. https://reviewboard.mozilla.org/r/223314/#review229466 None of the existing methods have such a check, so I wouldn't start doing it now with just those two methods. We simply pass-through everything.

Comment on attachment 8954154 [details] Bug 1329184 - Add script_args type checks to Python client. https://reviewboard.mozilla.org/r/223314/#review229466 Well I propose adding it here because we have had bugs with this in the past. I’m trying to solve a real issue here.

Comment on attachment 8954154 [details] Bug 1329184 - Add script_args type checks to Python client. https://reviewboard.mozilla.org/r/223314/#review229466 We had those bugs because we don't do type checks in the server component. And yes, I ca see why we want type checks here similar to geckodriver, but why now and only for those methods. Also please note that with your implementation the error types differ between what the client will raise, and what is raised by the server.

Comment on attachment 8954154 [details] Bug 1329184 - Add script_args type checks to Python client. https://reviewboard.mozilla.org/r/223314/#review229466 So I don’t understand your point exactly. Do we expect the error types that are raised to be in sync between the client and the server? That is not how most clients APIs are implemented. If indeed you think it is a bad idea to have type checking in the client (again I don’t understand why) would you be inclined to accept a patch that adds the type checking to the server?

Comment on attachment 8954154 [details] Bug 1329184 - Add script_args type checks to Python client. https://reviewboard.mozilla.org/r/223314/#review229466 > Do we expect the error types that are raised to be in sync between the client and the server? That is not how most clients APIs are implemented. I don't care about other client APIs. Fact is that both methods would cause problems for consumers of Marionette client because they throw a differnt kind of exception. Please lets avoid that. And yes, I would be happy to see a patch for Marionette server.

Comment on attachment 8954154 [details] Bug 1329184 - Add script_args type checks to Python client. https://reviewboard.mozilla.org/r/223314/#review229466 > I don't care about other client APIs. That is a strange position to take. > Fact is that both methods would cause problems for consumers > of Marionette client because they throw a differnt kind of > exception. Please lets avoid that. FWIW I’m not sure I agree with this assessment, as the client would inherently be safer. But I will close this bug as WONTFIX and file a new one to add the error to the server.

Comment on attachment 8954154 [details] Bug 1329184 - Add script_args type checks to Python client. https://reviewboard.mozilla.org/r/223314/#review229466 Maybe it was too harsh. What I wanted to say is that I don't see a value to change the client behavior for only two methods based on what other clients might handle API calls. If we want to chagne something here we should do it all at once. But I doubt that this is something we have time for.