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 a unit test for the spawner #407

Closed
wants to merge 3 commits into from

Conversation

kpaschen
Copy link

@kpaschen kpaschen commented May 29, 2020

This adds a test for the case where a pod is unschedulable and never leaves 'pending'.
Also a test for what happens when the pod reflector is added twice, and a basic test about creating a pvc (I only test the easy bit here where it raises an ApiException; for testing non-exception case would need to use a mock or a fixture that ensures the pvc is deleted afterwards).

I made two changes to the spawner that need review from someone who knows this code -- both are in places where a TimeoutException has been caught, and the spawner prints 'restarting pods'. The original code calls 'raise' right after that, and that seemed odd, so I changed it. If you think that's wrong, please let me know!

This brings coverage to 78.19%.

I've also noticed that proxy.py appears unused -- I wasn't sure of the status of that code though.

@kpaschen kpaschen marked this pull request as ready for review June 10, 2020 15:47
kubespawner/spawner.py Outdated Show resolved Hide resolved
@betatim
Copy link
Member

betatim commented Jun 13, 2020

Looks great. Let's see if someone remembers why/if the raise should be where it was before this PR or if that was a bug.

Otherwise I think merging this is a no brainer :D Thanks for taking the time to increase the coverage by adding some nice tests

@betatim betatim requested a review from yuvipanda June 26, 2020 11:30
kubespawner/spawner.py Outdated Show resolved Hide resolved
kubespawner/spawner.py Outdated Show resolved Hide resolved
kubespawner/spawner.py Outdated Show resolved Hide resolved
# minikube cluster.
c.KubeSpawner.node_selector = {'disktype': 'ssd'}
c.KubeSpawner.namespace = kube_ns
c.KubeSpawner.start_timeout = 30 # Do not wait very long.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
c.KubeSpawner.start_timeout = 30 # Do not wait very long.
c.KubeSpawner.start_timeout = 5 # Do not wait very long.

This duration will increase the time to complete the tests significantly, so I suggest to make this duration quite low.

Co-authored-by: Developer <contact@paschen.ch>
@yuvipanda
Copy link
Collaborator

image

Well, am here looking at this now :)

I 'solved' the merge conflicts and pushed this up, and will merge this if the tests pass.

@yuvipanda
Copy link
Collaborator

Unfortunately it looks like the internals of kubespawner have changed enough in the last 4 years that these tests no longer apply :( I'm going to close this PR for now, with apologies for the delay that caused this - am sorry @kpaschen!

@yuvipanda yuvipanda closed this Jun 20, 2024
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.

5 participants