search

ray-project / kuberay

A toolkit to run Ray applications on Kubernetes
Apache License 2.0
1.17k stars 373 forks source link

[Bug] Tests for Redis cleanup should not be in the tearDown hook #1997

Closed kevin85421 closed 6 months ago

kevin85421 commented 6 months ago

Search before asking

KubeRay Component

ci

What happened + What you expected to happen

https://github.com/ray-project/kuberay/pull/1989#issuecomment-1996185753

Reproduction script

https://github.com/ray-project/kuberay/pull/1989#issuecomment-1996185753

Anything else

No response

Are you willing to submit a PR?

kevin85421 commented 6 months ago

cc @rueian

rueian commented 6 months ago

Hi @kevin85421, I have opened the PR #2026 to address part of this issue. I added a new cleanup procedure to the compatibility e2e test in the PR as we discussed offline.

The current one in the tearDown hook doesn’t fail the test when needed because the hook catches all exceptions: https://github.com/ray-project/kuberay/blob/522807d64a7e00aadde4242f7b139d8db297b434/tests/framework/prototype.py#L590-L595

Indeed, testing redis cleanup in the tearDown looks weird, but given that it is not the only test we have during tearing down, the next step, I think, is that we should re-raise exceptions from the hook or rewrite all tests in the hook, for example moving them to normal test functions.