Draft/toga-web-testing #3728
Draft/toga-web-testing #3728
Conversation
git-subtree-dir: testbed/toga_web_testing git-subtree-split: c653c6b77e0315323ebc80afaddee2bf2a71bdd9
|
One small note to add, when running the test suite as described in the README, the tests should pass successfully. |
There was a problem hiding this comment.
First off - congratulations on getting this far :-) That you've been able to get anything working is a major accomplishment - this isn't a simple task.
Regarding the specifics of the PR - I've pushed a couple of updates to address some high level issues:
- The additions contained
.pycfiles. These are binary artefacts, and shouldn't be committed to version control. I actually don't know how you managed to add them, because the .gitignore file should have excluded them... - There's lots of pre-commit violations - you should ensure your development environment is set up as described in the contribution guide
- The directory structure you committed was unnecessarily convoluted.
As an example of (3) - your README instructions include details about copying app.py to a new location, and manually installing dependencies. Briefcase gives you a directory structure that will allow you to run a project, and manage dependencies... why not present the code in that format, ready to use.
The testbed folder is an example of a working test project. Why not copy that structure verbatim?
I've left a couple of other comments inline; there's some organisational stuff that we need to nail down before we get too much further.
There was a problem hiding this comment.
I'm not sure how you committed this file - anything in the __pycache__ file should be ignored. It's a binary artefact, and shouldn't be committed.
| from tests.tests_backend.proxies.box_proxy import BoxProxy | ||
| #from ..tests_backend.proxies.box_proxy import BoxProxy | ||
|
|
||
| """ TODO: Don't enable until below is implemented. |
There was a problem hiding this comment.
Triple quotes are a string; if you want to comment something out, use #
| _inst = None | ||
| _lock = threading.Lock() | ||
|
|
||
| def __new__(cls): |
There was a problem hiding this comment.
Rather than define a singleton, it may be better to handle this as a session-scoped fixture.
Singletons are a bit of an anti-pattern at the best of times; a session-scoped fixture means you get all the benefits of a single re-used fixture, without needing to implement a bunch of complex singleton logic. Plus, if you do need a second Page object for some reason, you can construct one that isn't a session-linked fixture.
| except Exception: | ||
| self._alock = asyncio.Lock() | ||
| finally: | ||
| self._ready.set() |
There was a problem hiding this comment.
This except/finally block silently eats errors. If you haven't got Playwright correctly installed, the new_page() call fails - but this dutifully creates a lock and progresses, and you get a bunch of async errors that imply the page hasn't been created without any other errors.
(Ask me how I know... :-) )
I don't think the exception handling here is needed - if you get an exception creating the page, you want the fixture to explode as early (and as proximal to the cause of the error) as possible.
| from ..page_singleton import BackgroundPage | ||
|
|
||
|
|
||
| class ButtonProbe: |
There was a problem hiding this comment.
I'm a little confused by how this ButtonProbe has been defined.
I can't deny it works - but it seems inconsistent with an approach that will work long term.
The Probe interface for any given widget (e.g., ButtonProbe) is something that is effectively defined by the testbed tests. If you look at the implementation of ButtonProbe for each other platform, the API is essentially identical - text, assert_no_icon(), assert_icon_size(), and so on.
I'd expect to see exactly the same thing here - a class that implements the required Probe interface. The implementation of the probe almost certainly involves calling the page object to retrieve key details (like the content of a text element) - but the API doesn't need to be run through __getattr__(). __getattr__() is only required if you want to make the object dynamically respond to any method... and the implementation here very clearly isn't doing that.
I'd expect the proxy to be dynamic - because we're looking to be able to retrieve an arbitrary attribute someattr, or call an arbitrary function somefunc() - but the Probe has a very strictly defined interface.
|
|
||
| return page.eval_js("(code) => window.test_cmd(code)", code) | ||
|
|
||
| def setup(self): |
There was a problem hiding this comment.
What's the difference between setup() and __init__()?
| from ..page_singleton import BackgroundPage | ||
|
|
||
|
|
||
| class ButtonProxy: |
There was a problem hiding this comment.
I'd be very surprised if we need a different Proxy class for every widget, except maybe at a very superficial level. The core implementation should be fundamentally the same - a __getattr__, a __setattr__, and maybe a __call__. We're looking for a dynamic wrapper around any object in the browser, not an interface that requires us to wrap every object individually.
… with a new proxy class architecture, only implemented with ButtonProxy.
…eton Remove page singleton method. Experiment with new proxy class architecture.
Web workspace/dynamic proxy
Merge pull request from workspace
|
I believe the above modifications covers the changes you have asked for. Again, please let us know if you would like anything changed further. Any feedback would be appreciated. Regarding the different proxies, only the |
There was a problem hiding this comment.
Definitely some good progress here.
- Using the Javascript interface to determine if the attribute on the remote side is callable is a neat approach, avoiding the need to pass in the object being proxied to the Proxy object itself.
- The duplication of the widget index is a little concerning - there shouldn't be a need to do that. Toga already has a widget index; we should be using that.
- I can see how the page object is being passed around - but it's a lot more complicated than it needs to be, and the approach you've taken undermines all the work that you've done making
pagea session-based fixture. - It's a little hard to tell how far you are down the "proxifyication" process - but assuming Button is a better representation than Box, it's definitely headed in the right direction; but there's still more abstraction that should be possible. ButtonProxy isn't adding anything that is "Button specific" - it should be possible to completely abstract that implementation of class construction.
- You may want to consider whether the API you've got for
test_cmd()is rich enough. It may be necessary to "serialize" results so that you know what return values for fuctions actually are. As it is, you're only really testing an attribute that returns a string; and that's fine because it's what the JS DOM is giving you. But if you're returning an integer, are you going to be able to tell the difference between42and"42"? If the return value oftest_cmd()was{"int": 42}or{"type": "int", "value": 42}, you would - and that would also give you the fidelity to return objects by id.
| if getattr(self, "_init", False): | ||
| return |
web-testbed/pyproject.toml
Outdated
| name = "testbed" | ||
| version = "0.0.1" | ||
|
|
||
| [project.optional-dependencies] |
There was a problem hiding this comment.
This change is no longer consistent with the install instructions. [dependency-groups] is the required header if you're installing pip install --group test. [project.optional-dependencies] would be consistent with pip install -e .[test]. The former (--group test) is the preferred usage; see PEP 735 for details.
web-testbed/tests/conftest.py
Outdated
| p = BackgroundPage() | ||
| yield p |
There was a problem hiding this comment.
If there's no "post yield" behaviour, this can be a simple return p.
web-testbed/tests/conftest.py
Outdated
| @pytest.fixture(scope="session", autouse=True) | ||
| def _wire_page(page): | ||
| BaseProxy.page_provider = staticmethod(lambda: page) | ||
| BoxProxy.page_provider = staticmethod(lambda: page) | ||
| MainWindowProxy.page_provider = staticmethod(lambda: page) | ||
| ButtonProbe.page_provider = staticmethod(lambda: page) |
There was a problem hiding this comment.
This seems like a round-about way to get access to the page.
Why not make the page a fixture that is passed into the app fixture, pass the page as an attribute of the AppProxy when it is constructed, and then make the app fixture a dependency of every other widget?
| def __init__(self, widget): | ||
| object.__setattr__(self, "id", widget.id) | ||
| object.__setattr__(self, "dom_id", f"toga_{widget.id}") |
There was a problem hiding this comment.
Why is this abstraction needed? Why isn't it just self.id = ...?
| # Alternate Method (non-lambda) | ||
| """ | ||
| sel = f"#{self.dom_id}" | ||
|
|
||
| if name == "text": | ||
| async def _text(page): | ||
| return await page.locator(sel).inner_text() | ||
| return w.run_coro(_text) | ||
|
|
||
| if name == "height": | ||
| async def _height(page): | ||
| box = await page.locator(sel).bounding_box() | ||
| return None if box is None else box["height"] | ||
| return w.run_coro(_height) | ||
| """ |
There was a problem hiding this comment.
If code is no longer needed, it can be deleted. It should be in version control history if it needs to be resurrected - but in this case, I doubt it will be.
| async def widget(): | ||
| return ButtonProxy() |
There was a problem hiding this comment.
If a ButtonProxy needs access to page to work - why not use page as a fixture here (or, at least, app, from which you can get access to page).
| code = ( | ||
| f"new_widget = toga.Button({repr(text)})\n" | ||
| "self.my_widgets[new_widget.id] = new_widget\n" | ||
| "result = new_widget.id" | ||
| ) |
There was a problem hiding this comment.
This code is identical to what is used to create a remote Box, except for (a) the class name, and (b) the arguments. Why can't they use the same implementation?
| await self._page.wait_for_timeout(7000) | ||
|
|
||
| await self._page.evaluate( | ||
| "(code) => window.test_cmd(code)", "self.my_widgets = {}" |
There was a problem hiding this comment.
Why is this needed? Based on this and subsequent usage, it looks like you're building an index of widgets by ID, stored on the web browser side of things - the app already has one of these - app.widgets.
| # everything else use text form (what Toga expects for .text, etc) | ||
| return repr(str(value)) |
There was a problem hiding this comment.
I'm not sure this will be safe - is there there any reason to suspect a str() form of an arbitrary argument will be valid?
…idget objects and dynamic attributes, not tested specifically yet. Tried using the in-built toga app 'self.widgets' registry, but had some trouble, sticking with 'my_widgets' for now. Trouble may stem from widget ids being strings(?) so adding to dict and retrieving looks a bit different.
…)' in '_encode_value' in 'expr_proxy.py', otherwise the 'MyObject()' data for the button 'test_text' does not work.
…ating the in-built 'self.widgets' registry, did not go well, I have my suspicions why. Therefore will keep using 'my_widgets' for now.
Web workspace/button press test
…d-new-format Merge to workspace from web-workspace/serialize-test-cmd-new-format
Merge Pull Request from workspace
|
For this update, all of the button-specific test methods have been implemented and pass successfully (apart from The current method for proxies accessing the Playwright page has not been altered (at least for now), as it allows for test methods and pytest fixtures to remain the same, and it gives our new For the The other suggestions have been fixed/dealt with. |
There was a problem hiding this comment.
For this update, all of the button-specific test methods have been implemented and pass successfully (apart from
test_icon(), since icons aren't fully implemented for the web backend yet). TheBoxProxyhas been 'proxified' and there is a new proxy structure, which allows for non-widget objects and object attributes. Thetest_cmdand base proxy methods have also been updated to handle more data types.
I feel like we're still talking at crossed purposes when it comes to Proxies.
BoxProxy has a hard-coded implementation of add(). The purpose of a proxy is that it shouldn't have any hard-coded implementations - it should only require __getattr__ and __setattr__ handling so that any method invoked on the proxy will be invoked on the remote alternative of the object. If you require hard coding of any calls, then you haven't got a full proxy yet.
Another example of where this is going off the rails - the _is_function and _is_primitive_attr methods. These have been carved out as special cases - but they shouldn't be. They're proxies. If you get an attribute of an object, it returns another object. That object might be a primitive, or a widget, or a function, or a class... but that's entirely a question of serialisation. You have an object myobject, and you have code that says myobject(arg1, arg2), that's invoking the __call__() member method of the myobject object.
For the
object.__setattr__andobject.__getattribute__uses, they are necessary to avoid recursion (previously caused test failures) and accidental remote calls when the test suite is running.
I'll need to dig into this; needing access to __getattribute__ is often an example of a deeper architectural issue, but you're also at the edge of things where you might actually have a legitimate use. However, I think we need to get the basic proxying right first before I dig too deep into this.
My suggestion at this point is to step back from widgets entirely (or as much as possible), and focus on proxying completely generic attributes and objects. Don't worry about Toga - treat the proxy of the App object in the testbed as an entry point, and add an "attribute that is an integer", and an "attribute that is a string", and a "attribute that returns an object", and "a function call that accepts no arguments" and so on - that is, get the core of proxying working. Once that is in place, "a button proxy" is "a proxy of a method that returns a Button instance", and shouldn't require any additional work or special handling.
| # no-op | ||
| # await probe.redraw(f"Button text should be {text}") |
There was a problem hiding this comment.
FWIW - it's fine if this is a no-op; in which case, implement it as a no op. Add a redraw() method on the widget's probe that does nothing. In particular, it likely isn't a full no-op, because the "--slow" mode of test execution is likely a call to await asyncio.sleep(1).
| code = ( | ||
| "import uuid\n" | ||
| f"new_obj = {ctor_expr}({call_args})\n" | ||
| "key = str(uuid.uuid4())\n" |
There was a problem hiding this comment.
There's no need to invent a new unique ID for an object. id(myobj) will return a unique integer identifier.
| from .base_proxy import BaseProxy | ||
|
|
||
|
|
||
| class NonWidgetProxy(BaseProxy): |
There was a problem hiding this comment.
There shouldn't be a need to differentiate Widgets and "non-widgets" - they're all just "objects" as far as Python is concerned.
| if name == "text": | ||
| rhs = repr(str(value)) | ||
| else: | ||
| rhs = self._encode_value(value) |
There was a problem hiding this comment.
Why is str special cased? Why can't it be handled as "yet another thing to encode"?
| raise TypeError("not a handle") | ||
| key = h["id"] | ||
| if h.get("ns", "widgets") == "widgets": | ||
| from .widget_proxy import WidgetProxy |
There was a problem hiding this comment.
In general, inline imports are an indicator of a code smell - imports should be at the top of the page, unless there's a circular dependency issue - and if there's a circular dependency issue, you should be refactoring to avoid that issue.
| return {k: self._unwrap(x) for k, x in v.items()} | ||
| return v | ||
|
|
||
| def _encode_value(self, value) -> str: |
There was a problem hiding this comment.
In terms of naming - it's worth maintaining parity in naming of APIs that are co-dependent. The opposite of 'encode' is 'decode', not "unwrap".
…ging of some proxies. Several other smaller modifications
…s. Minor variable renaming and other changes.
Merge branch workspace to draft/toga-web-testing
|
This update is mainly to make each object proxy a full proxy, and to fix other suggestions in the feedback. There are no hard-coded method implementations for any proxy, they should now be full proxies. They now only need to define a class name, for example Apart from that, the inline import, All 3 of the current |
There was a problem hiding this comment.
This looks like it's getting a lot closer to a workable solution.
My biggest concern at this point is the "line protocol". At present, it looks like it's essentially "passing blocks of executable code" back and forth - and that works, but it means there's lots of code "acting at a distance". If possible, it would be desirable to simplify the protocol so that the only information going back and forth are specific requests - essentially, instead of passing getattr(obj, name) as a string that is evaluated at the other side, you decompose that into a command (getattr), and a list of arguments (obj, name), and pass that down the wire.
web-testbed/src/testbed/app.py
Outdated
| self.main_window.show() | ||
|
|
||
| def cmd_test(self, code): | ||
| env = {"self": self, "toga": toga, "my_objs": self.my_objs, "Mock": Mock} |
There was a problem hiding this comment.
I think what you're looking for here is vars(). That's a complete state of all variables that are visible in the current scope.
web-testbed/src/testbed/app.py
Outdated
| def _serialise_payload(self, x): | ||
| # primitives | ||
| if x is None: | ||
| return {"type": "none", "value": None} |
There was a problem hiding this comment.
Value is likely redundant here.
| except Exception: | ||
| raise |
There was a problem hiding this comment.
This is effectively a no-op.
| from .base import SimpleProbe | ||
|
|
||
|
|
||
| class _ColorLike: |
There was a problem hiding this comment.
Is there any reason we can't use a Travertino Color or RGBA instance here?
| attr_expr = AttributeProxy(self, name) | ||
| ok, value = self._try_realise_value(attr_expr.js_ref) | ||
| return value if ok else attr_expr |
There was a problem hiding this comment.
I'm not sure I follow what is going on here. The result of accessing an attribute is another object, which will either be a literal, or a proxy on an object instance. What is the role of an AttributeProxy here?
| async def widget(): | ||
| return ButtonProxy("Hello") |
There was a problem hiding this comment.
Something to investigate - can this be pushed into the backend so that it's the backend responsibility to create the object (e.g., something like test_backend.factory("Button")("Hello")). That would allow the test case to remain generic - the current test suite can be satisfied by literally instantiating toga.Button, but the web backend can instantiate ButtonProxy() (which also opens the door to removing the need for object_proxies (or at least moving that functionality to be clearly in the backend)
…ther minor modifications.
…getattr__' in 'base_proxy.py'.
Web workspace/script and shim
Merge from workspace to draft/toga-web-testing
|
There is now a script Toga objects in the test suite now resolve to their proxies (i.e.
Other changes have been made in response to the feedback, which includes the part about Regarding the 'line protocol' suggestion, we could definitely investigate this method in the future. |
…o be executed to RPC.
Merge to PR
Is the above change what you had in mind for the "line protocol" request @freakboy3742 ? |
Merged web-workspace/python-callables to draft/toga-web-testing
|
This PR extends my previous commits. The proxy can now serialize Python callables (validators/handlers) by sending their source, the host reconstructs them, so assigning widget.validators from the runner correctly drives host-side validation. I’ve tested these changes with the existing suites: test_switch and test_textinput cover the caching/local-attribute behavior, and test_textinput also verifies callable transport. A few failures are currently expected due to the implementation of the web backend. test_on_change_programmatic and test_on_change_focus failure happened because the web backend doesn’t invoke self.interface._value_changed() yet (it should be called in dom_onkeyup() and set_value()). Similarly, is_valid will fail until the web backend is implemented. For proxy testing, I added a minimal shim (set_error()/clear_error() toggling a flag; is_valid() returning its inverse). Once the backend wires _value_changed() and implements is_valid properly, these tests should pass. |
Merged web-workspace/python-callables to workspace
Merged workspace to draft/toga-web-testing
This draft PR is for code review and feedback from @freakboy3742 in regards to @vt37 and my part of the Toga Curtin University capstone project 2025, where we test potential implementations of Toga web testing.
The source for this PR is our repository’s main branch here, and is where we do our development and testing. Please see here for more information on our part of capstone project and here for the issue for our part of the project.
These changes are not meant to be run, it is only a snapshot of our demo/sandbox repository, therefore CI failures are expected.
If you would like to run this test suite, please see the 'README' in our repository here.
We will update this PR over time, when our repository's main branch updates.
Refs #3545
PR Checklist: