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 basic start/stop test against a jupyterhub #120

Merged
merged 7 commits into from
May 31, 2023

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented May 25, 2023

Test coverage before this PR:

---------- coverage: platform linux, python 3.11.3-final-0 -----------
Name                               Stmts   Miss  Cover
------------------------------------------------------
systemdspawner/__init__.py             2      0   100%
systemdspawner/systemd.py             70      5    93%
systemdspawner/systemdspawner.py     100     72    28%
------------------------------------------------------
TOTAL                                172     77    55%

Test coverage after this PR:

---------- coverage: platform linux, python 3.11.3-final-0 -----------
Name                               Stmts   Miss  Cover
------------------------------------------------------
systemdspawner/__init__.py             2      0   100%
systemdspawner/systemd.py             78      7    91%
systemdspawner/systemdspawner.py     111     32    71%
------------------------------------------------------
TOTAL                                191     39    80%

@consideRatio consideRatio marked this pull request as draft May 25, 2023 20:10
@consideRatio consideRatio force-pushed the pr/e22-test branch 3 times, most recently from 4c9f95c to f700b04 Compare May 25, 2023 21:38
@consideRatio consideRatio marked this pull request as ready for review May 25, 2023 21:39
This was referenced May 25, 2023
@consideRatio consideRatio changed the title Add spawn test against a jupyterhub using SystemdSpawner Add basic start/stop test against a jupyterhub May 26, 2023
@@ -0,0 +1,113 @@
"""
These tests are running JupyterHub configured with a SystemdSpawner and starting
a server for the specific user named "runner". It is not meant to be run outside
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can I ask why these are not meant to be run outside the CI system? I am always wary of having tests that can't be run locally...

Copy link
Member Author

Choose a reason for hiding this comment

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

I want that also, but settled for this as an incremental improvement that felt most important.

The background is that I tried this locally on my ubuntu 22.04 i use and it worked if i picked the username erik to test with (existed) and if i started pytest as root like existing tests need for systemctl control. But it felt like a bad idea to create delete test users on the host system, and assume it has systemd of a version of relevance to test against.

So, then what? I think either we run it on github actions' ubuntu runners, or build an image and wrap running everything in a docker image, where pytest either is doing image build etc or is assumed to be started from the already build and started image.

To do such wrapping setups is quite complexity addition i think, and prone to significant maintenance and burden of understanding what goes on if something errors due to the complexity. It involves build image with relevant systemd version and python version, and setting everything up to run inside it.

Certainly doable, but I wanted to settle with this for now, but in full agreement it would be nice to run this locally as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's understandable. How about instead of making it be github specific, we gate on something like CLEAN_CI_ENVIRON=true or something? I just don't want us to encode GitHub specific information into our CI wherever possible. So let's rename this to something descriptive, open an issue / add a comment with rationale (copy pasting your comment here is more than good enough)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for thinking about this with me Yuvi! I learned a lot about working this further.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is amazing, thank you @consideRatio

@minrk minrk merged commit c4cac49 into jupyterhub:main May 31, 2023
@consideRatio consideRatio self-assigned this Jun 6, 2023
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.

3 participants