-
Notifications
You must be signed in to change notification settings - Fork 45
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
Conversation
4c9f95c
to
f700b04
Compare
tests/test_systemdspawner.py
Outdated
@@ -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 |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
-
Added 50968e5 to avoid making any assumptions about the CI environment.
I settled for removing all previous stuff related to this and defined
--system-test-user=USERNAME
and made it required to not skip the new test, which runs a user server with a users home directory mounted. -
Added dc5934a for a chore detail
-
Added d59e532 to provide some preliminary notes in CONTRIBUTING.md on setting up for local development.
-
Opened Local development notes on how to run tests from within a docker container #123 with what I felt in scope for this project to persue about testing against systemd in docker containers
-
Opened Rely on systemd-run's --working-directory, and refactor for readability #124 with some refactoring as a consequence of learning more about the project
There was a problem hiding this comment.
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
Test coverage before this PR:
Test coverage after this PR: