No tests for notebooks, fixture does not work when store = True

[krassowski]

Description

Heads up to anyone trying to improve test coverage:

I tried to write tests for #270 using rtc_create_notebook and rtc_create_file fixtures. While I was successful with rtc_create_file, tests with rtc_create_notebook appear to stall if using store=True.

I also noticed that this fixture is not used, not even once in the tests. It looks like it needs some attention.

jupyter-collaboration/tests/conftest.py

Lines 78 to 110 in 7898b25

	@pytest.fixture
	def rtc_create_notebook(jp_root_dir, jp_serverapp, rtc_add_doc_to_store):
	"""Creates a notebook in the test's home directory."""
	fim = jp_serverapp.web_app.settings["file_id_manager"]
	async def _inner(
	path: str, content: str | None = None, index: bool = False, store: bool = False
	) -> tuple[str, str]:
	nbpath = jp_root_dir.joinpath(path)
	# Check that the notebook has the correct file extension.
	if nbpath.suffix != ".ipynb":
	msg = "File extension for notebook must be .ipynb"
	raise Exception(msg)
	# If the notebook path has a parent directory, make sure it's created.
	parent = nbpath.parent
	parent.mkdir(parents=True, exist_ok=True)
	# Create a notebook string and write to file.
	if content is None:
	nb = nbformat.v4.new_notebook()
	content = nbformat.writes(nb, version=4)
	nbpath.write_text(content)
	if index:
	fim.index(path)
	if store:
	await rtc_add_doc_to_store("json", "notebook", path)
	return path, content
	return _inner

It stalls on line 106:

await rtc_add_doc_to_store("json", "notebook", path)

[krassowski]

It looks like the observe event/signal from YNotebook does not fire the callback:

jupyter-collaboration/tests/conftest.py

Lines 152 to 168 in 7898b25

	def _on_document_change(target: str, e: Any) -> None:
	if target == "source":
	event.set()
	async def _inner(format: str, type: str, path: str) -> None:
	if type == "notebook":
	doc = YNotebook()
	else:
	doc = YUnicode()
	doc.observe(_on_document_change)
	async with await rtc_connect_doc_client(format, type, path) as ws, WebsocketProvider(
	doc.ydoc, ws
	):
	await event.wait()
	await sleep(0.1)

Removing target check does not help - there is no callbacks at all.

YNotebook.observe is implemented in jupyter-ydoc, here:

def observe(self, callback: Callable[[str, Any], None]) -> None:
    """
    Subscribes to document changes.

    :param callback: Callback that will be called when the document changes.
    :type callback: Callable[[str, Any], None]
    """
    self.unobserve()
    self._subscriptions[self._ystate] = self._ystate.observe(partial(callback, "state"))
    self._subscriptions[self._ymeta] = self._ymeta.observe_deep(partial(callback, "meta"))
    self._subscriptions[self._ycells] = self._ycells.observe_deep(partial(callback, "cells"))

I am not familiar with the lower-level codebase enough to debug this further easily. Any thoughts?

[davidbrochart]

I didn't write these fixtures, so I'm also not very familiar with them.
Regarding YNotebook.observe, there has been changes recently in the way yrs handles them, and I updated pycrdt accordingly. Maybe worth trying again?
I'll try and check on a simple example and report back.

[davidbrochart]

This shows that observing notebook changes works as expected:

from jupyter_ydoc import YNotebook

ynb = YNotebook()

def callback(target, events):
    print(f"{target=}")
    for event in events:
        print(f"{str(event)=}")

ynb.observe(callback)
ynb.set(
    {
        "cells": [],
    }
)

# target='cells'
# str(event)='{target: [{"execution_count":null,"metadata":{"trusted":true},"source":"","id":"c6506805-eec8-496d-9ec1-28d7239bf0b8","cell_type":"code","outputs":[]}], delta: [{\'insert\': [<pycrdt._map.Map object at 0x7f9a0c5d84a0>]}], path: []}'
# target='meta'
# str(event)='{target: {"metadata":{"language_info":{"name":""},"kernelspec":{"name":"","display_name":""}},"nbformat":4.0,"nbformat_minor":5.0}, keys: {\'metadata\': {\'action\': \'add\', \'newValue\': <pycrdt._map.Map object at 0x7f9a0c5d92b0>}, \'nbformat_minor\': {\'action\': \'add\', \'newValue\': 5.0}, \'nbformat\': {\'action\': \'add\', \'newValue\': 4.0}}, path: []}'

[krassowski]

I think the problem is not that observing does not work when a change is made via set, but that the test fixture assumes that an event is emitted to the observer after establishing an initial connection. I think there might be a difference in behaviour between files and notebooks here.