Update the test screen implementation to allow customizable ports

[engineerjames]

This merge request adds the capability to customize the ports that the test Screen implementation uses. This allows for running multiple selenium-backed tests in parallel, and also allows users who aren't able to run in a container (and may be using this port for other things).

Admittedly I was having some issues running the unit tests that I am still working on, but I wanted to get the code change out there to see what the community thought.

[falkoschindler]

Thanks for the pull request, @engineerjames!

After having another look at your proposed changes, we have some concerns:

• Renaming PORT to DEFAULT_PORT can be considered a breaking change. Therefore we'd prefer to keep the name PORT unchanged.
• You added self.ui_run_kwargs.update(kwargs) to the start_server method. Why is this necessary?
• How to you actually use the new port parameter? The screen fixture doesn't allow to change it. And to change the port in Docker environments you could simply change the static PORT without using the new parameter.

[engineerjames]

Of course! These are great questions.

Renaming PORT to DEFAULT_PORT can be considered a breaking change. Therefore we'd prefer to keep the name PORT unchanged.

That is a fair point--happy to revert that.

You added self.ui_run_kwargs.update(kwargs) to the start_server method. Why is this necessary?
It isn't necessary for allowing the customization of the port (which is my main concern), but it does allow for users to customize other arguments that end up being forwarded to ui.run.

I thought it would be a useful addition, but I'm also happy to revert that part of the MR if you all feel it isn't a good idea.

How to you actually use the new port parameter? The screen fixture doesn't allow to change it. And to change the port in Docker environments you could simply change the static PORT without using the new parameter.

Unfortunately our current setup doesn't allow us to use Docker--for the use of the port parameter I was envisioning something like:

from nicegui.testing import Screen

@pytest.fixture(scope='function')
def screen_fixture(caplog: pytest.LogCaptureFixture):
    yield Screen(selenium.webdriver.Chrome(...), caplog, port=your_port_here)

def test_loading_ui(screen_fixture: Screen) -> None:
   # Your test details here

I'm still fairly new to Python though, so I may have overlooked some easier options to accomplish this.

[rodja]

I think this has almost nothing to do with Docker (except that in a container you may have different ports then on your main machine).
The existing screen fixture is a bit more complicated than your envisioned api:

https://github.com/zauberzeug/nicegui/blob/main/nicegui/testing/screen_plugin.py#L67-L85

I'm still not sure how you would make multiple tests run in parallel. It would be great if you could provide the implementation in a pull request which builds on this one.

[falkoschindler]

Another thought regarding your custom screen_fixture: This should be possible without the new port parameter.

@pytest.fixture(scope='function')
def screen_fixture(caplog: pytest.LogCaptureFixture):
    Screen.PORT = 1234
    yield Screen(selenium.webdriver.Chrome(...), caplog)

[engineerjames]

Another thought regarding your custom screen_fixture: This should be possible without the new port parameter.

@pytest.fixture(scope='function')
def screen_fixture(caplog: pytest.LogCaptureFixture):
    Screen.PORT = 1234
    yield Screen(selenium.webdriver.Chrome(...), caplog)

Unfortunately this won't work because the default port is used in the constructor to set other member variables that are used in the implementation. :(

[falkoschindler]

I just reverted the renaming and the kwargs. The remaining change is

• a new parameter port
• which is assigned to a member variable self.port
• and used to construct self.ui_run_kwargs and self.url.

Now let's come back to my proposal:

@pytest.fixture(scope='function')
def screen_fixture(caplog: pytest.LogCaptureFixture):
    Screen.PORT = 1234
    yield Screen(selenium.webdriver.Chrome(...), caplog)

Unfortunately this won't work because the default port is used in the constructor to set other member variables that are used in the implementation. :(

I don't understand. If Screen.PORT is set before calling Screen(), the effect on self.ui_run_kwargs and self.url equivalent to using a parameter port. The only difference would be if your code relies on the new member self.port or if some test needs to access Screen.PORT.

To be clear, I'm just trying to understand the benefit of the new port parameter. If you need a custom screen fixture anyway, we might just leave it as it is - or find a better way to make it configurable.

[engineerjames]

I don't understand. If Screen.PORT is set before calling Screen(), the effect on self.ui_run_kwargs and self.url equivalent to using a parameter port. The only difference would be if your code relies on the new member self.port or if some test needs to access Screen.PORT.

To be clear, I'm just trying to understand the benefit of the new port parameter. If you need a custom screen fixture anyway, we might just leave it as it is - or find a better way to make it configurable.

My apologies it took me so long to get back to you all; I was on a business trip. Oh! I hadn't realized that setting Screen.PORT prior to construction would effectively do the same thing as I was trying to do. Honestly in that case I don't think there's a need for the change I submitted. Thank you all for the feedback and the consideration--I'm a huge fan of nicegui, and will keep an eye out for other ways to contribute in the future.