-
Notifications
You must be signed in to change notification settings - Fork 134
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
[YUNIKORN-2623] Create unit tests for Clients #839
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #839 +/- ##
==========================================
+ Coverage 66.06% 67.29% +1.22%
==========================================
Files 69 70 +1
Lines 7550 7592 +42
==========================================
+ Hits 4988 5109 +121
+ Misses 2350 2271 -79
Partials 212 212 ☔ View full report in Codecov by Sentry. |
noOfInformers = 9 | ||
) | ||
|
||
func TestWaitForSync(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can do a bit refactor to Clients
to simplify the test.
- add a new method
informers
toClients
to return all informers - rewrite
WaitForSync
andRun
by looping all informers - in this test we can create a mock clients to override the new method
informers
to return a single mock informer
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do it a separate ticket? In this one I'd rather just focus on the new tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I have filed https://issues.apache.org/jira/browse/YUNIKORN-2625
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pbacsko thanks for this patch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What is this PR for?
Create mock for
SharedIndexedInformer
and update existing types.Write new unit tests for
client.Clients
.What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2623
How should this be tested?
Screenshots (if appropriate)
Questions: