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 FakeClient as backend for RESTClient based unit tests #469

Conversation

MatousJobanek
Copy link
Contributor

@MatousJobanek MatousJobanek commented Sep 30, 2024

It's a follow-up of the previous PR doing changes in unit-tests. This time for the CRTClient use-cases.
The PR is a bit bigger, but there is no chance to make it smaller. I could separate the removal, but I wanted to highlight what has been removed/replaced by these changes.

The same as before, the PR is about dropping the old mocks of the CRTClient by replacing their usage with the FakeClient. It doesn't try to improve the coverage nor the tests themself.
In the modified tests, I also dropped the usage of the MockableApplication type. I'll do the same also in the other tests in the following PR(s). There is still so many things to be removed and optimized :-).

KUBESAW-193

Comment on lines +91 to +95
func (s *ServiceFactory) WithSignupService(signupService service.SignupService) {
s.signupServiceFunc = func(_ ...signupservice.SignupServiceOption) service.SignupService {
return signupService
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a temp hack to inject specific fake version of signupService without any extra abstraction and additional mocking.

Comment on lines +17 to +18
type CrtClientProvider struct {
Cl kubeclient.CRTClient
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also a temp hack until this type is gonna be removed completely

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 81.86%. Comparing base (503d232) to head (f60744f).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/server/in_cluster_application.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #469      +/-   ##
==========================================
+ Coverage   81.83%   81.86%   +0.03%     
==========================================
  Files          42       42              
  Lines        2708     2708              
==========================================
+ Hits         2216     2217       +1     
+ Misses        406      405       -1     
  Partials       86       86              
Flag Coverage Δ
unittests 81.86% <73.33%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@alexeykazakov alexeykazakov left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of it!

Copy link

sonarqubecloud bot commented Oct 1, 2024

Copy link
Contributor

@mfrancisc mfrancisc left a comment

Choose a reason for hiding this comment

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

I like the new code structure, it's simpler to read and more maintainable, great job 👏

I have only two minor comments.

test/util/service_context.go Show resolved Hide resolved
test/util/service_context.go Show resolved Hide resolved
Copy link

openshift-ci bot commented Oct 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, MatousJobanek, mfrancisc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [MatousJobanek,alexeykazakov,mfrancisc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@MatousJobanek MatousJobanek merged commit 6d3e720 into codeready-toolchain:master Oct 1, 2024
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants