Add unit tests to the Katib SDK

[droctothorpe]

/kind feature

Describe the solution you'd like
The Katib SDK should have unit tests.

Anything else you would like to add:
We'd be happy to contribute them.

---

Love this feature? Give it a 👍 We prioritize the features with the most 👍

[andreyvelich]

That would be awesome if you could help us with the SDK unit tests @droctothorpe!

Do you want to add unit test for KatibClient or for Katib SDK models as well ?
Previously, we disabled generation of test files for models since it was broken: https://github.com/kubeflow/katib/blob/master/hack/gen-python-sdk/post_gen.py#L91-L101

[droctothorpe]

We'd probably start with the various KatibClient methods and branch out from there. We'd probably roll our own test files instead of generating them since we want to test the SDK logic as opposed to the generated client code. Do you mind storing test files alongside the modules they're testing? That's what KFP does.

[andreyvelich]

Sure, thanks @droctothorpe!
Do you mean creating sdk/python/v1beta1/kubeflow/katib/api/katib_client_test.py for unit tests ?
I guess, even Python unittest recommends it: https://docs.python.org/3/library/unittest.html#test-discovery.
In that case, we might want to refactor our tests for Suggestions:
https://github.com/kubeflow/katib/tree/master/test/unit/v1beta1/suggestion to locate them near original files.
WDYT @tenzen-y @johnugeorge @gaocegege @droctothorpe ?

[droctothorpe]

FWIW, I prefer storing unit test files alongside the modules they're testing because it makes iterating on them easier since they're side by side, but I've seen it both ways and am not strongly opinionated one way or the other.

Do ya'll have a preference between unittest and pytest? FWIW, the KFP SDK is migrating to pytest. Long term, it would be awesome if the various Kubeflow SDKs (KFP, katib, training operator) standardized and consolidated in a way that promoted reuse.

[tenzen-y]

https://github.com/kubeflow/katib/tree/master/test/unit/v1beta1/suggestion to locate them near original files.
WDYT @tenzen-y @johnugeorge @gaocegege @droctothorpe ?

I don't have a strong opinion. However, we should select either way, not adopt both ways.

[tenzen-y]

Do ya'll have a preference between unittest and pytest? FWIW, the KFP SDK is migrating to pytest. Long term, it would be awesome if the various Kubeflow SDKs (KFP, katib, training operator) standardized and consolidated in a way that promoted reuse.

It's a good point. Currently, although the katib uses both libs (pytest and unittest), we should select either way.
So, +1 on pytest to standardize the whole of kubeflow SDKs in a way.

[andreyvelich]

Yes, we should use pytest. I guess, we are already using it for our Suggestion unit test:

katib/test/unit/v1beta1/suggestion/test_optuna_service.py

Line 17 in e69235d

import pytest

I am also ok with storing test files alongside the actual files, similar to KFP.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[andreyvelich]

/lifecycle frozen