Use "SpecialPowers.spawn" instead of "Runtime.evaluate" in non-Runtime tests
Categories
(Remote Protocol :: CDP, task, P3)
Tracking
(Not tracked)
People
(Reporter: impossibus, Unassigned, Mentored)
Details
(Whiteboard: [lang=js])
Attachments
(1 file)
When testing the remote agent, we should aim to use only the domain under test whenever possible.
For example, here we're testing Input, and we're using another protocol method, Runtime.evaluate to do so. However, content.eval in ContentTask.spawn could probably be used instead.
(This is a follow-up from https://phabricator.services.mozilla.com/D47032?id=170493#inline-296911)
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
||
Comment 1•5 years ago
|
||
I don't see how a P5 should be a puppteer-alpha blocker. Removing it from that blocking list.
Otherwise this is great for a mentored bug. Example for ContentTask.span() we already have a lot under /remote/test/browser:
https://searchfox.org/mozilla-central/search?q=ContentTask.spawn&path=remote%2Ftest%2Fbrowser
Once the change has been made, make sure to run the specific test and that it passes:
./mach test remote/test/browser/input/browser_dispatchKeyEvent_race.js
Note that since the bug has been filed, the location of the originally targeted file has been changed.
Hi, I've been working with these files from another bug and I'd like to take on this bug too. Is it still available?
|
|
||
Comment 3•5 years ago
|
||
Yes, you can if you want to work on multiple bugs in parallel. But please do not over-commit, and just pick bugs whenever you are actually free to work on them. Thanks!
Hi, I'm looking to clarify what needs to be done.
I can see the use of "Runtime.evaluate()" in "checkWindowTestValue" in "browser_dispatchKeyEvent_race.js".
And I've looked at the examples of "ContentTask.spawn()" being used.
Do I need to simply replace "Runtime.evaluate()" with "ContentTask.spawn()" as spawn() seems to take in different arguments than whats given to evaluate()?
|
Reporter | |
Comment 5•5 years ago
|
||
It's not a simple replacement, but close. The essential is to take whatever expression is passed to Runtime.evaluate and get it executed in the scope of the html document loaded by the test. We want to do that with ContentTask.spawn instead and remove all references to Runtime.
The other tests in https://searchfox.org/mozilla-central/rev/4df8821c1b824db5f40f381f48432f219d99ae36/remote/test/browser/input/ have lots of examples.
The third arg to ContentTask.spawn is a function that will get executed in a special "frame script" in the document that is currently loaded in the browser. In the scope of that function, the content variable gives you access to the page, I believe. content.eval let's you evaluate expressions on the page's global.
|
||
Updated•4 years ago
|
I've submitted my first attempt. The tests seem to run the same as they did before.
|
|
||
Comment 8•4 years ago
|
||
Please note that with bug 1596918 we are now going to get rid of ContentTask.spawn in favor of SpecialPowers.spawn. Beside changing the method name, also the 2nd argument will have to obeyed which seems to have to be changed from null to [].
|
|
||
Comment 9•4 years ago
|
||
Dan, will you have time to update the patch based on whimboo’s latest feedback?
|
|
||
Comment 10•4 years ago
|
||
We haven't heard back from Dan on several other bugs so I assume that he is not continuing on this bug.
|
|
||
Updated•4 years ago
|
|
||
Comment 11•4 years ago
|
||
I want to work on this bug
|
|
Reporter | |
Comment 12•4 years ago
|
||
Please, go ahead. If you have any questions, you can post comments on this bug.
|
||
Comment 13•4 years ago
|
||
Hello all, is anyone actively working on this bug? If not, may I have a crack at it?
|
|
||
Comment 14•4 years ago
|
||
Hi Lupita. Great to see that you have interest to work on this bug. Feel free to get started, and have a look at the already attached patch and my comment 8. Let me know when you have more questions.
|
|
||
Comment 15•4 years ago
|
||
Thank you, Henrik! I've looked at the attached patch, and it doesn't pass the tests. I've modified the browser_dispatchKeyEvent_race.js as so:
const result = await SpecialPowers.spawn(gBrowser.selectedBrowser, [], () => {
return context.eval("window.testValue");
However, I get a referenceError claiming "context" is not defined, and I'm not sure why. Any help is appreciated. Thanks :)
|
|
||
Comment 16•4 years ago
|
||
You don't need the context in the content function. Just return window.testValue. The right tab is selected by gBrowser.selectedBrowser and that's enough.
|
|
||
Comment 17•4 years ago
|
||
Hi Lupita. I just want to check back with you, do you have any further questions?
|
|
||
Comment 18•4 years ago
|
||
Given that Lupita didn't respond on this bug it's open to get picked-up by someone else.
|
Assignee | |
Updated•3 years ago
|
|
||
Comment 19•2 years ago
|
||
Henrik, is it alright if I commandeer the patch? I found this bug while searching for more information regarding SpecialPowers.spawn.
Based on reading Maja's comment it seems that the only file that needs to be changed in the test directory is browser_dispatchKeyEvent_race.js, although that comment was from a few years ago. Does the statement remain true? There are a couple instances of Runtime.evaluate inside the test directory but outside of runtime/.
|
|
||
Comment 20•2 years ago
|
||
Hi James. Thanks for the question. It's a bug that bitrotted a bit.
Thinking more about the requested change here I feel that we probably should keep what we have right now and not convert any usage of Runtime.evaluate. The benefit from the current code is that we have a way better test coverage for this API, which we would loose when using the standard SpecialPowers.spawn method. Also work on CDP is on hold.
As such I think lets close this bug as wontfix. If needed we could re-evaluate at a later time.
Description
•