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.

Sign up for GitHub

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

Jump to bottom

Fix concurrent room initialization #255

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Fix concurrent room initialization #255

merged 1 commit into from
Mar 21, 2024

Conversation

davidbrochart
Copy link
Collaborator

No description provided.

@github-actions GitHub Actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch davidbrochart/jupyter_collaboration/test-room-init

@davidbrochart davidbrochart added bug Something isn't working maintenance labels Mar 20, 2024
@davidbrochart davidbrochart force-pushed the test-room-init branch 2 times, most recently from 5cec219 to 3d1e40b Compare March 20, 2024 14:03
@davidbrochart davidbrochart mentioned this pull request Mar 20, 2024
Draft
@davidbrochart davidbrochart force-pushed the test-room-init branch 2 times, most recently from ef8bc5f to 9597357 Compare March 20, 2024 16:06
Zsailer
Zsailer approved these changes Mar 20, 2024
View reviewed changes
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.

Thanks, @davidbrochart. This looks good to me.

@davidbrochart
Copy link
Collaborator Author

I should mention that this PR not only has a test, but also a fix.
I'd like to release it soon if there is no objection.

@Zsailer
Copy link
Member

Zsailer commented Mar 20, 2024

I should mention that this PR not only has a test, but also a fix.

Yeah, I saw this.

No objection. This makes sense to me. I'm not sure how this affects @dlqqq's PR. Perhaps some of this logic will change in #250, but for now this makes sense.

@Zsailer
Copy link
Member

Zsailer commented Mar 20, 2024

I'm kicking the CI really fast to see if the flaky windows test passes and triggers the remaining tests.

I think the UI tests are failing in other PRs, so I think we can ignore here.

@Zsailer
Copy link
Member

Zsailer commented Mar 20, 2024

The windows 3.8 test seems to fail reliably. I've kicked it 3 times and it keeps failing. @davidbrochart can you check if it's related to your changes?

@davidbrochart
Copy link
Collaborator Author

The windows 3.8 test seems to fail reliably. I've kicked it 3 times and it keeps failing. @davidbrochart can you check if it's related to your changes?

It's not, since test_room_handler_doc_client_should_connect is failing in #251 as well. We need to look into this test.

@davidbrochart davidbrochart changed the title Add test for concurrent room initialization Fix concurrent room initialization Mar 21, 2024
@davidbrochart
Copy link
Collaborator Author

I'm not sure how this affects @dlqqq's PR. Perhaps some of this logic will change in #250, but for now this makes sense.

Yes if #250 gets merged it will override the changes in this PR, but until then let's release since it fixes a bug.

@davidbrochart davidbrochart merged commit 1b87ab2 into jupyterlab:main Mar 21, 2024
20 of 21 checks passed
@davidbrochart davidbrochart deleted the test-room-init branch March 21, 2024 09:28
@krassowski krassowski mentioned this pull request Mar 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zsailer Zsailer Zsailer approved these changes

Assignees
No one assigned
Labels
bug Something isn't working maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@davidbrochart @Zsailer