Closed
Bug 1452490
Opened 7 years ago
Closed 6 years ago
Remove marionetteScriptFinished
Categories
(Remote Protocol :: Marionette, enhancement, P2)
Remote Protocol
Marionette
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: ato, Assigned: vpit3833, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=py][lang=js])
Attachments
(1 file, 1 obsolete file)
marionetteScriptFinished is a global exposed to all scripts injected using WebDriver:ExecuteAsyncScript. It is a non-conforming global that we do not want to expose. Instead, scripts should be relying on the last argument from the arguments local scoped variable to return. See https://searchfox.org/mozilla-central/search?q=marionetteScriptFinished&path= for a list of usages in central.
Comment 1•7 years ago
|
||
(In reply to Andreas Tolfsen βΉ:atoβΊ from comment #0) > Instead, scripts should be relying on the last argument from the > arguments local scoped variable to return. Can you please explain this a bit more? It's not that clear to me. Thanks.
Reporter | ||
Comment 2•7 years ago
|
||
Scripts are wrapped in functions that are passed a set of arguments.
In addition to any user-defined arguments, async scripts always
gets an additional last argument that is a callback or resolve
function.
For example:
> self.marionette.execute_script("""
> let [foo, resolve] = arguments;
> setTimeout(() => resolve(foo), 1000);
> """, script_args("foo",))
Comment 3•6 years ago
|
||
Here an updated link for the currently in use marionetteScriptFinished method: https://dxr.mozilla.org/mozilla-central/search?q=marionetteScriptFinished&=mozilla-central I think this is a good bug for mentoring, so I will take it.
Mentor: hskupin
Whiteboard: [lang=py][lang=js]
Assignee | ||
Comment 4•6 years ago
|
||
I started by removing sandBoxName and MARIONETTE_SCRIPT_FINISHED from evaluate.js, and ./mach marionette test returns error for marionetteScriptFinished not defined. How to proceed from here?
Flags: needinfo?(hskupin)
Comment 5•6 years ago
|
||
You basically want to start with all the tests which are using `marionetteScriptFinished`, and turn them to the new format. As last step you can then remove the implementation in Marionette itself.
Flags: needinfo?(hskupin)
Assignee | ||
Comment 6•6 years ago
|
||
Can you give an example please? For instance, at https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py#21, what will the new format be looking like?
Flags: needinfo?(hskupin)
Comment 7•6 years ago
|
||
Sure. So here the original: > self.assertEqual(1, self.marionette.execute_async_script("marionetteScriptFinished(1);")) Which means that this script tests for the return value of the async script command. Given that we want to get rid of `marionetteScriptFinished` you will have to replace it with `resolve(1)` as I mentioned in my last comment. Then from Andreas' example in comment 2 you can see how to get the reference to resolve. Which means what it will look like this: > self.assertEqual(1, self.marionette.execute_async_script("let [resolve] = arguments; resolve(1);"))
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request) |
Reporter | ||
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review256598 ::: testing/awsy/awsy/awsy_test_case.py:151 (Diff revision 1) > gc_script = """ > Cu.import("resource://gre/modules/Services.jsm"); > Services.obs.notifyObservers(null, "child-mmu-request", null); > > let memMgrSvc = Cc["@mozilla.org/memory-reporter-manager;1"].getService(Ci.nsIMemoryReporterManager); > - memMgrSvc.minimizeMemoryUsage(() => marionetteScriptFinished("gc done!")); > + memMgrSvc.minimizeMemoryUsage(() => {let [resolve] = arguments; resolve("gc done!");}); I would assign resolve at the beginning of the function. ::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:22 (Diff revision 1) > > def test_execute_async_simple(self): > self.assertEqual(1, self.marionette.execute_async_script("arguments[arguments.length-1](1);")) > > def test_execute_async_ours(self): > - self.assertEqual(1, self.marionette.execute_async_script("marionetteScriptFinished(1);")) > + self.assertEqual(1, self.marionette.execute_async_script("let [resolve] = arguments; resolve(1);")) In all these cases you could just call arguments[0](1). ::: testing/marionette/harness/marionette_harness/tests/unit/test_window_close_content.py:126 (Diff revision 1) > - marionetteScriptFinished(true); > + let [resolve] = arguments; > + resolve(true); Assign resolve at beginning of function.
Attachment #8984507 -
Flags: review-
Assignee | ||
Comment 10•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review256598 > In all these cases you could just call arguments[0](1). Done, using arguments[0](xyz). Before submitting next review, How to proceed here: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py#103 ? arguments[2] is None, and 'let [resolve] = arguments; resolve();' returns TypeError on resolve as it is not a function. This is the last place still using marionetteScriptFinished. When this is done, evaluate.js can be edited as well.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review257034 ::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:28 (Diff revision 2) > > def test_execute_async_timeout(self): > self.assertRaises(ScriptTimeoutException, self.marionette.execute_async_script, "var x = 1;") > > def test_execute_async_unique_timeout(self): > - self.assertEqual(2, self.marionette.execute_async_script("setTimeout(function() {marionetteScriptFinished(2);}, 2000);", script_timeout=5000)) > + self.assertEqual(2, self.marionette.execute_async_script("setTimeout(arguments[0](2), 2000);", script_timeout=5000)) Here you are calling arguments[0] literally, whereas the code before passed an anonymous function calling the callback. Because arguments[0](2) returns 2 immediately you need to still keep the anonymous function here. The correct code would be: > setTimeout(() => arguments[0](2), 2000); ::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:102 (Diff revision 2) > + # "let [resolve] = arguments;" > + # "resolve();", Debug? ::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:104 (Diff revision 2) > > def test_sandbox_refresh_arguments(self): > self.marionette.execute_async_script("this.foobar = [arguments[0], arguments[1]];" > + # "let [resolve] = arguments;" > + # "resolve();", > "marionetteScriptFinished();", Remaining use of marionetteScriptFinished. ::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py:117 (Diff revision 2) > - .then(() => marionetteScriptFinished(true)) > - .catch(() => marionetteScriptFinished(false)); > + .then(() => arguments[0](true)) > + .catch(() => arguments[0](false)); Here too I would assign resolve first so that you donβt have to index arguments twice.
Attachment #8984507 -
Flags: review-
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review257034 > Remaining use of marionetteScriptFinished. Yes, this is where I don't know how to go ahead. replacing this with > let [resolve] = arguments; > resolve(); returns type error on resolve as it is not a function but an array. So I commented the two lines to remind me to not miss the spot. Do you recommend any pointers to follow to find a solution?
Reporter | ||
Comment 15•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review257034 > Yes, this is where I don't know how to go ahead. replacing this with > > > let [resolve] = arguments; > > resolve(); > > returns type error on resolve as it is not a function but an array. So I commented the two lines to remind me to not miss the spot. Do you recommend any pointers to follow to find a solution? Ah sorry, I missed your question earlier. Thanks for bringing it to my attention again. The callback is always the last argument, so what you will want to do here is something like this: > let resolve = arguments[arguments.length - 1]; Or: > let [x, y, resolve] = arguments; Or: > let resolve = arguments[2];
Comment hidden (mozreview-request) |
Comment 17•6 years ago
|
||
Venkatesh, is the last update of the patch ready to review? I asked because you didn't set the review flag.
Assignee: nobody → venkateshpitta
Status: NEW → ASSIGNED
Flags: needinfo?(venkateshpitta)
Assignee | ||
Comment 18•6 years ago
|
||
Ah, I missed it Henrik. Yes, this is the recent update of the patch for review. The comment block refers to marionetteScrioptFinished to explain things. For now I have replaced it with ?s
Flags: needinfo?(venkateshpitta)
Assignee | ||
Updated•6 years ago
|
Attachment #8984507 -
Flags: review?(hskupin)
Assignee | ||
Comment 20•6 years ago
|
||
In this try server output at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bca58f77baa0a205559dfadab7cbe27f1fc771, how to find out where the fails are originating from? Says there are 20 fails.
Flags: needinfo?(ato)
Comment 21•6 years ago
|
||
(In reply to Venkatesh from comment #20) > In this try server output at > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=e0bca58f77baa0a205559dfadab7cbe27f1fc771, how to find > out where the fails are originating from? Click on each of those failing jobs and you will notice a new panel is opening at the bottom. Failing tests are marked with `TEST-UNEXPECTED-ERROR`. Check the path as printed directly next to it, and run this test locally. Those should also be broken, and would need an update. I will review the patch once the failures are gone.
Reporter | ||
Comment 22•6 years ago
|
||
I think whimboo mostly answered this, but the failures all seem to be in the Firefox UI tests. When you click the orange en-US icons next to the test type, you will find a list of test errors originating from files such as testing/firefox-ui/tests/puppeteer/test_places.py. Ping me on IRC (ato) if you need more help.
Flags: needinfo?(ato)
Assignee | ||
Comment 23•6 years ago
|
||
./mach test testing/firefox-ui/tests/puppeteer/test_places.py 0:00.02 ERROR Failure during harness execution Traceback (most recent call last): File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/runtests.py", line 94, in cli failed = harness_instance.run() File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/runtests.py", line 72, in run runner = self._runner_class(**self.args) File "/home/venkatesh/src/mozilla-central/testing/firefox-ui/harness/firefox_ui_harness/runners/base.py", line 18, in __init__ super(FirefoxUITestRunner, self).__init__(**kwargs) File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/runner/base.py", line 532, in __init__ self.test_kwargs = deepcopy(kwargs) File "/usr/lib/python2.7/copy.py", line 163, in deepcopy y = copier(x, memo) File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) File "/usr/lib/python2.7/copy.py", line 190, in deepcopy y = _reconstruct(x, rv, 1, memo) File "/usr/lib/python2.7/copy.py", line 334, in _reconstruct state = deepcopy(state, memo) File "/usr/lib/python2.7/copy.py", line 163, in deepcopy y = copier(x, memo) File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) File "/usr/lib/python2.7/copy.py", line 190, in deepcopy y = _reconstruct(x, rv, 1, memo) File "/usr/lib/python2.7/copy.py", line 334, in _reconstruct state = deepcopy(state, memo) File "/usr/lib/python2.7/copy.py", line 163, in deepcopy y = copier(x, memo) File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) File "/usr/lib/python2.7/copy.py", line 163, in deepcopy y = copier(x, memo) File "/usr/lib/python2.7/copy.py", line 230, in _deepcopy_list y.append(deepcopy(a, memo)) File "/usr/lib/python2.7/copy.py", line 190, in deepcopy y = _reconstruct(x, rv, 1, memo) File "/usr/lib/python2.7/copy.py", line 334, in _reconstruct state = deepcopy(state, memo) File "/usr/lib/python2.7/copy.py", line 163, in deepcopy y = copier(x, memo) File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) File "/usr/lib/python2.7/copy.py", line 163, in deepcopy y = copier(x, memo) File "/usr/lib/python2.7/copy.py", line 230, in _deepcopy_list y.append(deepcopy(a, memo)) File "/usr/lib/python2.7/copy.py", line 190, in deepcopy y = _reconstruct(x, rv, 1, memo) File "/usr/lib/python2.7/copy.py", line 334, in _reconstruct state = deepcopy(state, memo) File "/usr/lib/python2.7/copy.py", line 163, in deepcopy y = copier(x, memo) File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) File "/usr/lib/python2.7/copy.py", line 190, in deepcopy y = _reconstruct(x, rv, 1, memo) File "/usr/lib/python2.7/copy.py", line 334, in _reconstruct state = deepcopy(state, memo) File "/usr/lib/python2.7/copy.py", line 163, in deepcopy y = copier(x, memo) File "/usr/lib/python2.7/copy.py", line 257, in _deepcopy_dict y[deepcopy(key, memo)] = deepcopy(value, memo) File "/usr/lib/python2.7/copy.py", line 182, in deepcopy rv = reductor(2) TypeError: __getnewargs__ should return a tuple, not 'unicode' What is going wrong here? I am trying to run the failed tests locally and getting this.
Flags: needinfo?(hskupin)
Comment 24•6 years ago
|
||
(In reply to Venkatesh from comment #23) > "/home/venkatesh/src/mozilla-central/testing/marionette/harness/ > marionette_harness/runner/base.py", line 532, in __init__ > self.test_kwargs = deepcopy(kwargs) You want to check which entries `kwargs` contains, and figure out which of those causing `deepcopy` to fail.
Flags: needinfo?(hskupin)
Assignee | ||
Comment 25•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #24) > > You want to check which entries `kwargs` contains, and figure out which of > those causing `deepcopy` to fail. I get kwargs = {'package_name': None, 'log_mach_level': None, 'test_objects': [{'name': 'test_places.py', 'tags': 'local', 'here': u'/home/venkatesh/src/mozilla-central/testing/firefox-ui/tests/puppeteer', 'manifest': u'/home/venkatesh/src/mozilla-central/testing/firefox-ui/tests/puppeteer/manifest.ini', u'file_relpath': u'testing/firefox-ui/tests/puppeteer/test_places.py', 'path': u'/home/venkatesh/src/mozilla-central/testing/firefox-ui/tests/puppeteer/test_places.py', u'flavor': u'firefox-ui-functional', u'dir_relpath': u'testing/firefox-ui/tests/puppeteer', 'relpath': u'testing/firefox-ui/tests/puppeteer/test_places.py'}], 'emulator_bin': None, 'avd': None, 'prefs_files': None, 'jsdebugger': False, 'log_errorsummary': None, 'log_xunit': None, 'log_mach': None, 'log_tbpl_level': None, 'log': <mozlog.structuredlog.StructuredLogger object at 0x7f26e7cd0ed0>, 'log_tbpl': None, 'log_unittest': None, 'avd_home': None, 'pydebugger': None, 'log_tbpl_compact': None, 'log_tbpl_buffer': None, 'log_raw': None, 'logger_name': 'Marionette-based Tests', 'log_mach_buffer': None, 'log_mach_verbose': None, 'log_raw_level': None, 'adb_path': None, 'prefs_args': None, 'log_html': None, 'device_serial': None} How to work out which of these is the trouble?
Flags: needinfo?(hskupin)
Comment 26•6 years ago
|
||
I don't see anything obvious here. Which version of Python are you using? Maybe also pull and update your local code for latest additions on central. Maybe there was a broken changeset.
Flags: needinfo?(hskupin)
Reporter | ||
Comment 27•6 years ago
|
||
Iβm seeing the same thing as venkatesh when running "MOZ_HEADLESS=1 ./mach test testing/firefox-ui/tests/puppeteer/test_places.py" on central (without this patch applied). Iβve filed https://bugzilla.mozilla.org/show_bug.cgi?id=1470397 about this. To run this test it is possible to use "./mach firefox-ui-test TEST".
Reporter | ||
Comment 28•6 years ago
|
||
(In reply to Andreas Tolfsen γ:atoγ from comment #27) > To run this test it is possible to use "./mach firefox-ui-test > TEST". I mean "./mach firefox-ui-functional TEST".
Reporter | ||
Comment 29•6 years ago
|
||
It appears that the problem with the tests is that resolve is undefined: > JavaScript error: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py, line 43: TypeError: resolve is not a function This fixes the problem: > diff --git a/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py b/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py > index cc1a8e5e43f0..7072bca13872 100644 > --- a/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py > +++ b/testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py > @@ -32,8 +32,8 @@ class Places(BaseLib): > return self.marionette.execute_async_script(""" > Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); > > - let [resolve] = arguments; > - PlacesUtils.bookmarks.fetch({url: resolve}).then(bm => { > + let [url, resolve] = arguments; > + PlacesUtils.bookmarks.fetch({url}).then(bm => { > resolve(bm != null); > }); > """, script_args=[url]) > @@ -49,13 +49,13 @@ class Places(BaseLib): > Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); > > let folderGuids = []; > - let [resolve] = arguments; > + let [url, resolve] = arguments; > > function onResult(bm) { > folderGuids.push(bm.parentGuid); > } > > - PlacesUtils.bookmarks.fetch({url: resolve}, onResult).then(() => { > + PlacesUtils.bookmarks.fetch({url}, onResult).then(() => { > resolve(folderGuids); > }); > """, script_args=[url])
Assignee | ||
Comment 30•6 years ago
|
||
(In reply to Andreas Tolfsen γ:atoγ from comment #29) > It appears that the problem with the tests is that resolve is undefined: > > > JavaScript error: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py, line 43: TypeError: resolve is not a function > How to arrive at this error message? Because test_utils.py, test_software_update.py, and test_places.py all return the same Python error message. They all might share same/similar fix as well. And, where is this patch pasted here to be applied? on the bookmark for this bug? mozilla-central?
Flags: needinfo?(ato)
Reporter | ||
Comment 31•6 years ago
|
||
(In reply to Venkatesh from comment #30) > (In reply to Andreas Tolfsen γ:atoγ from comment #29) > > > It appears that the problem with the tests is that resolve is undefined: > > > > > JavaScript error: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py, line 43: TypeError: resolve is not a function > > How to arrive at this error message? Because test_utils.py, > test_software_update.py, and test_places.py all return the same Python error > message. They all might share same/similar fix as well. Try looking at what Firefox writes to stdout with the "--gecko-log -" argument. > And, where is this patch pasted here to be applied? on the bookmark for this > bug? mozilla-central? It was applied on top your patch. The test is passing on centralβ¦
Flags: needinfo?(ato)
Assignee | ||
Comment 32•6 years ago
|
||
(In reply to Andreas Tolfsen γ:atoγ from comment #31) > Try looking at what Firefox writes to stdout with the "--gecko-log -" > argument. > I looked up ./mach run -h, ./mach test -h, and did not find --gecko-log listed in them. Where can I find it? > > And, where is this patch pasted here to be applied? on the bookmark for this > > bug? mozilla-central? > > It was applied on top your patch. The test is passing on centralβ¦ I applied this fix on top of my bookmark for this bug, did ./mach test testing/firefox-ui/tests/puppeteer/test_places.py -z and still get the same error from Python. I am still trying to understand the connection between applying the fix on my patch and test passing on central. Feels as if I am missing something obvious here. Please explain.
Flags: needinfo?(ato)
Comment 33•6 years ago
|
||
(In reply to Venkatesh from comment #32) > I looked up ./mach run -h, ./mach test -h, and did not find --gecko-log > listed in them. Where can I find it? This option is specific to Marionette harness tests, so it will not be listed by `mach test`. Use at least `mach marionette-test` to see it in the output. > I applied this fix on top of my bookmark for this bug, did ./mach test > testing/firefox-ui/tests/puppeteer/test_places.py -z and still get the same > error from Python. I am still trying to understand the connection between Correct, because you didn't run with `mach firefox-ui-functional ...` but still with `mach test` which is broken as Andreas mentioned in comment 28 about 4 days ago. So please run this command instead.
Flags: needinfo?(ato)
Reporter | ||
Comment 34•6 years ago
|
||
(In reply to Venkatesh from comment #32) > I looked up ./mach run -h, ./mach test -h, and did not find > --gecko-log listed in them. Where can I find it? I already mentioned this in https://bugzilla.mozilla.org/show_bug.cgi?id=1452490#c28. Just if thereβs any confusion, the full command you need for running this test and seeing the Firefox stdout is this: > % ./mach firefox-ui-functional -z --gecko-log - > testing/firefox-ui/tests/puppeteer/test_places.py > I applied this fix on top of my bookmark for this bug, did ./mach > test testing/firefox-ui/tests/puppeteer/test_places.py -z and > still get the same error from Python. I am still trying to > understand the connection between applying the fix on my patch > and test passing on central. Feels as if I am missing something > obvious here. Please explain. That is strange. Iβm sure the patch I quoted fixes it. Maybe the stdout from Firefox you will get when you run the correct command will give you an error that is actionable?
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review259680 Removing review flag for now until the failures have been fixed.
Attachment #8984507 -
Flags: review?(hskupin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•6 years ago
|
||
I am submitting to review after editing to fix the errors that were thrown by the try server tests previous time. On both central and local bookmark for this bug, ./mach clobber && ./mach build && ./mach firefox-ui-functional -z returns the following error message on my PC. FAIL testing/firefox-ui/tests/functional/safebrowsing/test_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download - AssertionError: 'goog-badbinurl-proto.pset' not found in [] Traceback (most recent call last): File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run testMethod() File "/home/venkatesh/src/mozilla-central/testing/firefox-ui/tests/functional/safebrowsing/test_initial_download.py", line 111, in test_safe_browsing_initial_download self.assertIn(f, files_on_disk_google4) What can I do here?
Flags: needinfo?(hskupin)
Comment 38•6 years ago
|
||
That failure is unrelated to your changes and tracked in bug 1385613. It would still be nice to find out why this is happening, so if you are interested to help let us know on that other bug after Francois gave me an answer. Thanks.
Flags: needinfo?(hskupin)
Comment 39•6 years ago
|
||
mozreview-review |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review260270 It's coming along nicely. Thank you for the update. There are still remaining items to fix, so please see my inline comments. Further I pushed a new try build so we can check the current status. ::: testing/marionette/evaluate.js:45 (Diff revision 4) > * > * The arguments provided by the `args<` argument are exposed > * through the `arguments` object available in the script context, > * and if the script is executed asynchronously with the `async` > * option, an additional last argument that is synonymous to the > - * `marionetteScriptFinished` global is appended, and can be accessed > + * `????????` global is appended, and can be accessed This needs an update. Use a wording which matches the `resolve callback`. ::: testing/marionette/evaluate.js:53 (Diff revision 4) > * The `timeout` option specifies the duration for how long the > * script should be allowed to run before it is interrupted and aborted. > * An interrupted script will cause a {@link ScriptTimeoutError} to occur. > * > * The `async` option indicates that the script will not return > - * until the `marionetteScriptFinished` global callback is invoked, > + * until the `????????` global callback is invoked, Here too. `until the "resolve" callback is invoked`. ::: testing/marionette/evaluate.js (Diff revision 4) > > src += `(function() { ${script} }).apply(null, ${ARGUMENTS})`; > > - // marionetteScriptFinished is not WebDriver conformant, > - // hence it is only exposed to immutable sandboxes > - if (sandboxName) { Removing that also removes any usage of `sandboxName` which then causes the ES linter to fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bca58f77baa0a205559dfadab7cbe27f1fc771&selectedJob=183204754
Attachment #8984507 -
Flags: review?(hskupin) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review260270 > Removing that also removes any usage of `sandboxName` which then causes the ES linter to fail: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bca58f77baa0a205559dfadab7cbe27f1fc771&selectedJob=183204754 Removed sandboxName entirely. For ./mach firefox-ui-functional -z, only FAIL testing/firefox-ui/tests/functional/safebrowsing/test_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download - AssertionError: 'goog-badbinurl-proto.pset' not found in [] from previous time fails. No new fails. Also, ./mach lint passes everything.
Comment 42•6 years ago
|
||
Thank you for the update. I triggered a new try build. If it passes please set the review flag for me.
Reporter | ||
Comment 43•6 years ago
|
||
It looks like the try run passes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=117b3fdda1ac4c2b5b5d57f98f97d6b25ea548af But Iβm not able to set the review flag because mozreview is, well, terrible.
Comment 44•6 years ago
|
||
(In reply to Andreas Tolfsen γ:atoγ from comment #43) > But Iβm not able to set the review flag because mozreview is, well, terrible. You cannot set it because there is a second mozreview request now on this bug, and there haven't been all drafts been published. And this can only be done by the patch author.
Comment 45•6 years ago
|
||
Oh, and I wish you would have kept the old mozreview request for easier reviewing. Now we have to go through everything again.
Assignee | ||
Updated•6 years ago
|
Attachment #8989035 -
Flags: review?(hskupin)
Assignee | ||
Comment 46•6 years ago
|
||
Sorry for the hold up. I keep forgetting that there is a publish button. Not again.
Updated•6 years ago
|
Attachment #8984507 -
Attachment is obsolete: true
Comment 47•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review260270 > Removed sandboxName entirely. For ./mach firefox-ui-functional -z, only > > FAIL testing/firefox-ui/tests/functional/safebrowsing/test_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download - AssertionError: 'goog-badbinurl-proto.pset' not found in [] > > from previous time fails. No new fails. > > Also, ./mach lint passes everything. This sounds good. Thanks.
Comment 48•6 years ago
|
||
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. So this patch is actually not obsolete, but you forgot to amend your changes and as such there are two separate commits again. Please fix that.
Attachment #8984507 -
Attachment is obsolete: false
Flags: needinfo?(venkateshpitta)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8989035 -
Attachment is obsolete: true
Attachment #8989035 -
Flags: review?(hskupin)
Assignee | ||
Comment 50•6 years ago
|
||
I pushed after histedit/amend. On review board, there is a r-. I am not able to set it to r?whimboo.
Flags: needinfo?(venkateshpitta)
Comment 51•6 years ago
|
||
(In reply to Venkatesh (:vpit3833) from comment #50) > I pushed after histedit/amend. On review board, there is a r-. I am not > able to set it to r?whimboo. Once the reviewer is set in mozreview there is no need to set it again. Whenever you upload a new version of the patch, a new review request will be automatically send out.
Comment 52•6 years ago
|
||
mozreview-review |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review261830 This looks good and the tests are passing which is great! The remaining issues are mostly cosmetic but for the following I would like to have the feedback from Andreas too: Personally I would like to see the usage of `let [resolve] = arguments;` across the files, and not `arguments[arguments.length - 1]`. First it's shorter and when multiple args are specified it makes it clear what's all in the argument list because you have to destruct everything. ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:99 (Diff revision 5) > fieldname: arguments[0], > value: arguments[1], > firstUsed: (Date.now() - 5000) * 1000, > }; > let finished = false; > + let resolve = arguments[arguments.length - 1]; Please move this up to the first line of the code. ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:189 (Diff revision 5) > def createSync(self): > # This script will write an entry to the login manager and create > # a signedInUser.json in the profile dir. > self.runAsyncCode(""" > Cu.import("resource://gre/modules/FxAccountsStorage.jsm"); > + let resolve = arguments[arguments.length - 1]; Please move this a line up so that getting arguments is always first. ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:216 (Diff revision 5) > # Note that we expect 2 logins - one from us, one from sync. > self.assertEqual(loginCount, 2, "No other logins are present") > > def checkBookmarkInMenu(self): > titleInBookmarks = self.runAsyncCode(""" > + let resolve = arguments[arguments.length - 1]; This should come after `url`. ::: browser/components/migration/tests/marionette/test_refresh_firefox.py:282 (Diff revision 5) > self.assertEqual( > formFieldResults[0]['value'], self._formHistoryValue) > > formHistoryCount = self.runAsyncCode(""" > let count; > + let resolve = arguments[arguments.length - 1]; Move up please. ::: testing/awsy/awsy/awsy_test_case.py:151 (Diff revision 5) > gc_script = """ > Cu.import("resource://gre/modules/Services.jsm"); > Services.obs.notifyObservers(null, "child-mmu-request", null); > > let memMgrSvc = Cc["@mozilla.org/memory-reporter-manager;1"].getService(Ci.nsIMemoryReporterManager); > - memMgrSvc.minimizeMemoryUsage(() => marionetteScriptFinished("gc done!")); > + memMgrSvc.minimizeMemoryUsage(() => {arguments[0]("gc done!");}); Lets add the `resolve` variable for all the instances in this file please. ::: testing/marionette/client/docs/basics.rst:171 (Diff revision 5) > .. parsed-literal:: > result = client.execute_script("return arguments[0] + arguments[1];", > script_args=[2, 3]) > assert result == 5 > > The async method works the same way, except it won't return until a special I would remove `special` now and call it `until the 'resolve' function is called`. ::: testing/marionette/client/docs/basics.rst:176 (Diff revision 5) > The async method works the same way, except it won't return until a special > -`marionetteScriptFinished()` function is called: > +`resolve()` function is called: > > .. parsed-literal:: > result = client.execute_async_script(""" > + let [resolve] = arguments; Personally I like this more and would like to see it everywhere. ::: testing/marionette/harness/marionette_harness/tests/unit/test_addons.py:34 (Diff revision 5) > @property > def all_addon_ids(self): > with self.marionette.using_context("chrome"): > addons = self.marionette.execute_async_script(""" > Components.utils.import("resource://gre/modules/AddonManager.jsm"); > + let [resolve] = arguments; Please move this up, and maybe add an empty line to the import. ::: testing/marionette/harness/marionette_harness/tests/unit/test_addons.py:48 (Diff revision 5) > def reset_addons(self): > with self.marionette.using_context("chrome"): > for addon in (self.all_addon_ids - self.preinstalled_addons): > addon_id = self.marionette.execute_async_script(""" > Components.utils.import("resource://gre/modules/AddonManager.jsm"); > + let [resolve] = arguments; Same here. ::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py (Diff revision 5) > > def test_execute_async_js_exception(self): > try: > self.marionette.execute_async_script(""" > - let [resolve] = arguments; > + arguments[0](foo()); > - resolve(foo()); Please leave that as is. ::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:102 (Diff revision 5) > self.assertEqual(self.marionette.execute_async_script( > - "marionetteScriptFinished(this.foobar);", new_sandbox=False), [23, 42]) > + "arguments[0](this.foobar);", new_sandbox=False), [23, 42]) > > def test_sandbox_refresh_arguments(self): > self.marionette.execute_async_script("this.foobar = [arguments[0], arguments[1]];" > - "marionetteScriptFinished();", > + "let resolve = arguments[arguments.length - 1];" The line is getting way long, better to break the arguments into a new line with 4-chars of indentation. ::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py:52 (Diff revision 5) > """ > return self.marionette.execute_async_script(""" > Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); > > - let folderGuids = [] > + let folderGuids = []; > + let [url, resolve] = arguments; Please move up as first line of the script code. ::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py:79 (Diff revision 5) > def restore_default_bookmarks(self): > """Restore the default bookmarks for the current profile.""" > retval = self.marionette.execute_async_script(""" > Components.utils.import("resource://gre/modules/BookmarkHTMLUtils.jsm"); > > + let [resolve] = arguments; Same here. ::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/places.py:120 (Diff revision 5) > def remove_all_history(self): > """Remove all history items.""" > retval = self.marionette.execute_async_script(""" > Components.utils.import("resource://gre/modules/PlacesUtils.jsm"); > > + let [resolve] = arguments; And here. ::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/software_update.py:386 (Diff revision 5) > :returns: The URL of the update snippet > """ > url = self.marionette.execute_async_script(""" > Components.utils.import("resource://gre/modules/UpdateUtils.jsm"); > let res = UpdateUtils.formatUpdateURL(arguments[0]); > + let resolve = arguments[arguments.length - 1]; And here. ::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/utils.py:57 (Diff revision 5) > """ > > with self.marionette.using_context('chrome'): > result = self.marionette.execute_async_script(""" > var {Sanitizer} = Components.utils.import("resource:///modules/Sanitizer.jsm", {}); > + let resolve = arguments[arguments.length - 1]; And here.
Attachment #8984507 -
Flags: review?(hskupin) → review-
Comment 53•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [away 07/08 - 07/22] from comment #52) > Personally I would like to see the usage of `let [resolve] = arguments;` > across the files, and not `arguments[arguments.length - 1]`. First it's > shorter and when multiple args are specified it makes it clear what's all in > the argument list because you have to destruct everything. Andreas, what do you think?
Flags: needinfo?(ato)
Reporter | ||
Comment 54•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [away 07/08 - 07/22] from comment #53) > (In reply to Henrik Skupin (:whimboo) [away 07/08 - 07/22] from > comment #52) > > > Personally I would like to see the usage of `let > > [resolve] = arguments;` across the files, and not > > `arguments[arguments.length - 1]`. First it's shorter and when > > multiple args are specified it makes it clear what's all in the > > argument list because you have to destruct everything. > > Andreas, what do you think? You canβt always do this if there is more than one argument. If there are three arguments and you would have to expand all four arguments even if you donβt plan on using them, since there is no way to dismiss unused variables in JS: > let [foo, bar, baz, resolve] = arguments; One a more general note I donβt feel that the style used for getting at the callback argument is the most important thing to fret over. If the patch fulfills the goal of removing marionetteScriptFinished, that moves us into a position of removing this from the Marionette server, which is of greater value than the style of our internal tests.
Flags: needinfo?(ato)
Comment 55•6 years ago
|
||
(In reply to Andreas Tolfsen γ:atoγ from comment #54) > You canβt always do this if there is more than one argument. > > If there are three arguments and you would have to expand all four > arguments even if you donβt plan on using them, since there is no > way to dismiss unused variables in JS: > > > let [foo, bar, baz, resolve] = arguments; Sure, in such a case it is totally fine to index the arguments array. But in most of the cases you will also make use of the arguments which are getting passed-in. > One a more general note I donβt feel that the style used for getting > at the callback argument is the most important thing to fret over. > If the patch fulfills the goal of removing marionetteScriptFinished, > that moves us into a position of removing this from the Marionette > server, which is of greater value than the style of our internal > tests. I will go through the open issue, and close those which aren't necessary. For the rest I would like that you do the final review.
Comment 56•6 years ago
|
||
Oh, and I actually meant to give a r+ with the changes made. I will also correct that.
Comment 57•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review261830 > Please move this a line up so that getting arguments is always first. You can also use `let [resolve] = arguments;` here. > This should come after `url`. Even better use destructuring here: `let [url, resolve] = arguments;`. > Move up please. Also use destructuring here please.
Comment 58•6 years ago
|
||
mozreview-review |
Comment on attachment 8984507 [details] Bug 1452490 - Remove marionetteScriptFinished. https://reviewboard.mozilla.org/r/250358/#review261860 I won't be around the next two weeks. So please request an additional review if necessary from Andreas. Thanks.
Attachment #8984507 -
Flags: review- → review+
Comment hidden (mozreview-request) |
Reporter | ||
Comment 61•6 years ago
|
||
Itβs hard for me to tell whatβs been fixed since there are 14 open issues on the review (mainly because mozreview doesnβt really present this in an intuitive way). Can you please close the issues you have fixed? try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d95faa57e12e73c7c302e48a9f429c138144e200
Flags: needinfo?(ato) → needinfo?(venkateshpitta)
Assignee | ||
Comment 62•6 years ago
|
||
Fixed all the issues that were left open. Closed them all now.
Flags: needinfo?(venkateshpitta)
Comment 63•6 years ago
|
||
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8eab40c27903 Remove marionetteScriptFinished. r=whimboo
Comment 64•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8eab40c27903
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Reporter | ||
Comment 65•6 years ago
|
||
Thanks for fixing this Venkatesh! I got your privmsg on IRC, but was on holiday yesterday.
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•