test: Put test-functions.js helpers on window object #20833
Conversation
martinpitt
force-pushed
the
test-functions-window
branch
from
295eb26
to
44a016a
Compare
test/common/storagelib.py
Outdated
| @@ -193,7 +193,7 @@ def dialog_set_val(self, field, val): | |||
|
|
|||
| def dialog_combobox_choices(self, field): | |||
| return self.browser.call_js_func("""(function (sel) { | |||
| var lis = ph_find(sel).querySelectorAll('li'); | |||
| var lis = window.ph_find(sel).querySelectorAll('li'); | |||
There was a problem hiding this comment.
window is the global object, so do we really need to change all the code like this? Or do you prefer it for clarity?
There was a problem hiding this comment.
I made a testrun with all changes to testlib.py reverted: https://cockpit-logs.us-east-1.linodeobjects.com/pull-20843-be1fb85d-20240806-133921-arch-storage/log.html
Looks pretty green.
There was a problem hiding this comment.
Or in other words, this isn't actually a API break.
There was a problem hiding this comment.
This is a prerequisite for the BiDi conversion in #20832. Exporting globals only works with CDP, you can't do it with webdriver or bidi. So that reversion only proves that main isn't broken, but there was never any doubt about that 😁
There was a problem hiding this comment.
No, it's not for clarity, it's the core of this change: You cannot export globals if all you have is "you define a function and the browser calls it". We have to attach it to an existing global, like sizzle does.
There was a problem hiding this comment.
Yes, we have to say window.ph_select = function () { }, but do we also have to say window.ph_select() everywhere? Is window not the global object when evaluating JavaScript via bidi?
There was a problem hiding this comment.
Ah! Sorry, I misunderstood you there. Testing..
There was a problem hiding this comment.
But yeah, testing this without bidi was bogus, urks.
webdriver's and BiDi's functions for preloading a script only supports declaring and calling a function. This can't define globals. So do what sizzle does and put the helper functions on the `window` global object. Drop the long-obsolete jQuery comment.
martinpitt
force-pushed
the
test-functions-window
branch
from
44a016a
to
2473d84
Compare
martinpitt
changed the title
|
@mvollmer Thanks! This is much nicer indeed. I wasn't aware that you don't have to write |
…lper functions" This wasn't necessary after all, things on `window.` are available in the global namespace. See the final version of cockpit-project/cockpit#20833 This reverts commit b8aae7b.
…lper functions" This wasn't necessary after all, things on `window.` are available in the global namespace. See the final version of cockpit-project/cockpit#20833 This reverts commit b8aae7b.
webdriver's and BiDi's functions for preloading a script only supports declaring and calling a function. This can't define globals.
So do what sizzle does and put the helper functions on the
windowglobal object.Drop the long-obsolete jQuery comment.
cockpit-project/bots#6688 already landed for not breaking naughty patterns.
This is a prerequisite for #20832