Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for document dirty attribute #251

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

davidbrochart
Copy link
Collaborator

No description provided.

Copy link
Contributor

Binder 👈 Launch a Binder on branch davidbrochart/jupyter_collaboration/test-dirty

async with WebsocketProvider(jupyter_ydoc.ydoc, ws):
jupyter_ydoc.dirty = True
assert jupyter_ydoc.dirty
await sleep(1.5)
Copy link
Member

@Zsailer Zsailer Mar 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this sleep interval work only because the file_poll_interval trait is set to 1 by default?

If so, we might want to define a constant in this repo that is used in the trait's default_value argument and here in the test. That way, if we ever change the default in the future, we don't have to update it in multiple places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I just didn't find how to do that, even with jp_configurable_serverapp. Do you have an idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just thinking we'd define a constants.py module with e.g.

# constants.py
FILE_POLL_INTERVAL = 1

and use this constant as the default value of the YDocExtension.file_poll_interval, like

# app.py
from .contants import FILE_POLL_INTERVAL

...

file_poll_interval = Float(default_value=FILE_POLL_INTERVAL
...

Then, in this unit test, do something like:

from jupyter_collaboration.constants import FILE_POLL_INTERVAL

...
await sleep(FILE_POLL_INTERVAL + 0.5)
...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this isn't great either, since if we changed FILE_POLL_INTERVAL to a larger value some point in the future, it would make this test very slow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you pass the configuration through the jp_server_config pytest fixture?

FILE_POLL_INTERVAL = 2

@pytest.fixture
def jp_server_config():
    return {
        "YDocExtension": {"file_poll_interval": FILE_POLL_INTERVAL}
    }

I can't remember if this config makes it to the extension or not...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this works and I added an rtc_document_save_delay fixture in b92aefd so that we can parameterize it.

Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good @davidbrochart.

I added some comments, but they shouldn't block from merging.

@davidbrochart davidbrochart merged commit 44f6a7a into jupyterlab:main Mar 21, 2024
18 of 20 checks passed
@davidbrochart davidbrochart deleted the test-dirty branch March 21, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants