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

Use cache only for service business service - unit test refactor #5930

Merged
merged 1 commit into from Mar 28, 2023

Conversation

nrfox
Copy link
Contributor

@nrfox nrfox commented Mar 20, 2023

Part of #5599. Most of this change involves updates to the unit tests that previously did not work with the cache enabled.

@nrfox nrfox changed the title Use cache only for workload service Use cache only for service business service - unit test refactor Mar 24, 2023
@nrfox nrfox marked this pull request as ready for review March 24, 2023 14:56
Copy link
Contributor

@hhovsepy hhovsepy left a comment

Choose a reason for hiding this comment

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

Did some regression testing on OCP cluster, works for me.

Copy link
Contributor

@josunect josunect left a comment

Choose a reason for hiding this comment

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

LGTM.

Just one comment, kialiCache is just passed to the service business service because is the only test that didn't work with the disabled cache?

@nrfox
Copy link
Contributor Author

nrfox commented Mar 27, 2023

Just one comment, kialiCache is just passed to the service business service because is the only test that didn't work with the disabled cache?

No before it was using the global var kialiCache. Having it as a global var meant I had to set that global var everywhere but the only way to know which tests were using it was having it crash at runtime. This really obscured how large this change was. If the various business services did not use global vars but required all their dependencies to be passed in, we should know at compile time if something was missing or not.

All of the tests that were updated relied on the cache being disabled and once the cache was required for the business service, each one of the tests now had to setup the cache.

@nrfox nrfox self-assigned this Mar 27, 2023
@josunect josunect self-requested a review March 28, 2023 06:36
@nrfox nrfox merged commit ee9191a into kiali:master Mar 28, 2023
5 checks passed
@nrfox nrfox deleted the kiali-5599-biz-svc branch March 28, 2023 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants