maartenbreddels / ipywebrtc

WebRTC for Jupyter notebook/lab
MIT License
241 stars 40 forks source link

Webrtc refactor #138

Open gjmooney opened 7 months ago

gjmooney commented 7 months ago

Split up webrtc.js into separate files based on the type of stream.

maartenbreddels commented 7 months ago

Nice, I'm generally ok with large files, and prefer them actually, but maybe this file was a bit too much.

My only worry is with all of these changes is that we break ipywebrtc without noticing. I'm not a fan (anymore) of galata based testing, and we've recently switch over to using solara for testing. This is a pure pytest based solution, so it's much simpler for people to adopt.

The docs are on here: https://solara.dev/docs/howto/testing

An example that only tests the DOM on all major widget platforms (notebook/lab/voila/solara) can be found:

A test that only runs on solara (much faster) can be found:

I prefer tests to only test the dom structure and possibly events, I don't think we need to store screenshots for now, a basic smoke test should be enough. In the future we could possibly test the screenshot feature etc, but that is more work.

We set up tests usually like this:

So that the install and test are separate jobs.

If you have the bandwidth to add UI testing, let me know, but I thought let me at least share our vision on testing with you and @martinRenou . Possibly this could be interesting for other ipywidgets projects as well.

martinRenou commented 7 months ago

This is a pure pytest based solution, so it's much simpler for people to adopt.

Sounds exciting! I'm in for testing widgets using this.

The only downside of it is that it does not test widgets in the JupyterLab context, which may break while the solara case still works. Though let's not maintain two test approaches, enough tooling.

You're right that we should add ui-tests before making any big change to this. @gjmooney would you be willing to add solara tests?

maartenbreddels commented 7 months ago

If you use the ipywidgets_runner fixture, we test it on JupyterLab+Notebook (6)+Voila+Solara, as done in: https://github.com/widgetti/ipyaggrid/blob/master/tests/ui/jupyter_test.py

gjmooney commented 7 months ago

Yea, I can look into adding/updating the tests.

maartenbreddels commented 7 months ago

Please take a look at my latest suggestion regarding the failure

maartenbreddels commented 7 months ago

Does it pass locally on your computer?

gjmooney commented 7 months ago

Does it pass locally on your computer?

Nope. I'm thinking the issue is with the file location, if the kernel_code() is running in a separate kernel I'm assuming it can't find that file? I ran the tests in with --headed and all the tests show Error creating view for mediastream: cannot play video stream.

I can get tests involving CameraStream to pass, but only in headed mode since it's requesting the webcam permission. Does Solara have a way to access Playwrights browser context to call grant_permissions?

maartenbreddels commented 7 months ago

Nope. I'm thinking the issue is with the file location, if the kernel_code() is running in a separate kernel I'm assuming it can't find that file?

I recently had this issue, I'll get back to you on this with a way to pass the location.

Does Solara have a way to access Playwrights browser context

You can use all the playwright fixtures, such as browser etc, not that we use page_session instead of page to get a shared single page.

maartenbreddels commented 6 months ago

Note that you can pass argument to the function, see https://github.com/widgetti/ipyreact/blob/b500a53bcf4883627d724281d4f9b4e2b59e5798/tests/ui/module_test.py#L136

But note that in one of the tests (using solara server) the arguments do not get passed, you can do this workaround for now: https://github.com/widgetti/ipyreact/blob/b500a53bcf4883627d724281d4f9b4e2b59e5798/tests/ui/module_test.py#L47