Conversation
|
Maybe pin pytest=8 for now so that the CI passes |
|
I think it requires rethinking the async fixtures, I can look at it. |
da62498 to
5e14812
Compare
|
Thanks @gjmooney. We'll need a test for the new functionality. |
1a57b31 to
b0c950a
Compare
b0c950a to
a295d55
Compare
davidbrochart
left a comment
There was a problem hiding this comment.
I think that there is too much mocking in the tests, in the end they don't test much.
Could you try and test more with less mocking?
tests/test_comm_awareness.py
Outdated
|
|
||
| def send(self, *, buffers=None, **kwargs): | ||
| if buffers: | ||
| self.sent.append(bytes(memoryview(buffers[0]))) |
There was a problem hiding this comment.
It was used in the CommProvider __init__ when CommProvider got instantiated with the DummyComm
tests/test_comm_awareness.py
Outdated
| payload = awareness.encode_awareness_update([awareness.client_id]) | ||
| frame = create_awareness_message(payload) | ||
|
|
||
| assert frame[0] == YMessageType.AWARENESS | ||
|
|
||
| provider._receive({"buffers": [frame]}) |
There was a problem hiding this comment.
The test passes without this code, so I'm wondering what this test checks?
@gjmooney Don't worry if you cannot reach full coverage. I'm fine with no test and explicit |
de328d0 to
2d86227
Compare
|
There is something I don't understand: your |
For now setting the awareness from the kernel has not been evoked on our side. I wonder if that is something we want to do? |
|
Isn't test_remote_manager_applies_awareness_messages doing just that? I suggest to remove it then. |
d101d6f to
062974a
Compare
062974a to
61782e4
Compare
| self.remote_widget: Widget | None = None | ||
| self.local_widget_created = Event() | ||
| self.remote_widget_created = Event() | ||
| self._remote_awareness: Awareness | None = None |
There was a problem hiding this comment.
If you're not supporting the kernel setting the awareness, no need to test it?
| self._remote_awareness: Awareness | None = None |
| pytestmark = pytest.mark.anyio | ||
|
|
||
|
|
||
| async def test_comm_provider_applies_awareness_frame(synced_widgets, context): |
There was a problem hiding this comment.
| async def test_comm_provider_applies_awareness_frame(synced_widgets, context): | |
| async def test_comm_provider_applies_awareness_message(synced_widgets, context): |
| remote_awareness = Awareness(Doc()) | ||
| remote_awareness.set_local_state({"role": "remote"}) | ||
| payload = remote_awareness.encode_awareness_update([remote_awareness.client_id]) | ||
| frame = create_awareness_message(payload) |
There was a problem hiding this comment.
| frame = create_awareness_message(payload) | |
| message = create_awareness_message(payload) |
| async def test_comm_widget_exposes_provider_awareness(synced_widgets, context): | ||
| async with context: | ||
| widget = await synced_widgets.get_local_widget() | ||
| assert widget.awareness is widget._comm_provider.awareness |
There was a problem hiding this comment.
You just need a CommWidget?
| async def test_comm_widget_exposes_provider_awareness(synced_widgets, context): | |
| async with context: | |
| widget = await synced_widgets.get_local_widget() | |
| assert widget.awareness is widget._comm_provider.awareness | |
| async def test_comm_widget_exposes_provider_awareness(): | |
| widget = CommWidget() | |
| assert widget.awareness is widget._comm_provider.awareness |
| async def test_comm_widget_awareness_observe_and_unobserve(synced_widgets, context): | ||
| async with context: | ||
| widget = await synced_widgets.get_local_widget() | ||
|
|
||
| events: list[str] = [] | ||
| sub_id = widget.awareness.observe(lambda topic, _: events.append(topic)) | ||
|
|
||
| widget.awareness.set_local_state({"ping": 1}) | ||
| assert events | ||
|
|
||
| widget.awareness.unobserve(sub_id) | ||
| events.clear() | ||
| widget.awareness.set_local_state({"ping": 2}) | ||
| assert events == [] |
There was a problem hiding this comment.
Same comment as above.
| async def test_comm_widget_awareness_observe_and_unobserve(synced_widgets, context): | |
| async with context: | |
| widget = await synced_widgets.get_local_widget() | |
| events: list[str] = [] | |
| sub_id = widget.awareness.observe(lambda topic, _: events.append(topic)) | |
| widget.awareness.set_local_state({"ping": 1}) | |
| assert events | |
| widget.awareness.unobserve(sub_id) | |
| events.clear() | |
| widget.awareness.set_local_state({"ping": 2}) | |
| assert events == [] | |
| async def test_comm_widget_awareness_observe_and_unobserve(): | |
| widget = CommWidget() | |
| events: list[str] = [] | |
| sub_id = widget.awareness.observe(lambda topic, _: events.append(topic)) | |
| widget.awareness.set_local_state({"ping": 1}) | |
| assert events | |
| widget.awareness.unobserve(sub_id) | |
| events.clear() | |
| widget.awareness.set_local_state({"ping": 2}) | |
| assert not events |
| case YMessageType.AWARENESS: # pragma: nocover | ||
| if self._remote_awareness is None: | ||
| self._remote_awareness = Awareness( | ||
| self.remote_widget.ydoc # pragma: no | ||
| ) | ||
| payload = read_message(bytes(message[1:])) | ||
| self._remote_awareness.apply_awareness_update(payload, None) |
There was a problem hiding this comment.
See above.
| case YMessageType.AWARENESS: # pragma: nocover | |
| if self._remote_awareness is None: | |
| self._remote_awareness = Awareness( | |
| self.remote_widget.ydoc # pragma: no | |
| ) | |
| payload = read_message(bytes(message[1:])) | |
| self._remote_awareness.apply_awareness_update(payload, None) |
This PR wires up
pycrdtAwareness, so the kernel can consumeYMessageType.AWARENESSframes.CommProvidercreates an Awareness(ydoc) alongside sync/update handling. Inbound messages whose first byte isYMessageType.AWARENESSare decoded withread_messageand applied withapply_awareness_update.CommWidgetexposesawareness, pluson_awareness_change/unobserve_awarenessas thin wrappers overAwareness.observe/unobservefor notebooks and library code.