-
Notifications
You must be signed in to change notification settings - Fork 919
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
Added tests for cmd/descheduler #5445
Added tests for cmd/descheduler #5445
Conversation
Hey @XiShanYongYe-Chang is there a need to test cmd/descheduler/app/options/options.go? |
3a7c3dc
to
462a677
Compare
I refactored the pr and tried to avoid tests that are likely to break with minor, inconsequential changes. |
I FORGOT TO PULL THE LATEST COMMITS :) . @XiShanYongYe-Chang Please don't review i'll update the tests. |
Hi @anujagrawal699 the lint ci is failed. |
394ecfc
to
086599f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5445 +/- ##
==========================================
+ Coverage 31.14% 31.27% +0.13%
==========================================
Files 640 640
Lines 44417 44417
==========================================
+ Hits 13832 13890 +58
+ Misses 29583 29519 -64
- Partials 1002 1008 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@XiShanYongYe-Chang Please take a look. |
Thanks |
healthAddress := "127.0.0.1:8082" | ||
metricsAddress := "127.0.0.1:8083" | ||
|
||
go serveHealthzAndMetrics(healthAddress, metricsAddress) |
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.
Too many arguments in call to 'serveHealthzAndMetrics'
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.
@XiShanYongYe-Chang I made the same error. Few days a ago a commit changed the 'serveHealthzAndMetrics' and the new implementation is
func serveHealthzAndMetrics(healthProbeBindAddress, metricsBindAddress string) {
if healthProbeBindAddress == metricsBindAddress {
if healthProbeBindAddress != "0" {
go serveCombined(healthProbeBindAddress)
}
} else {
if healthProbeBindAddress != "0" {
go serveHealthz(healthProbeBindAddress)
}
if metricsBindAddress != "0" {
go serveMetrics(metricsBindAddress)
}
}
}
Your local might be behind by some commits.
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.
Can you rebase the latest master code and re-push it?
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.
Okay
086599f
to
a2fc7dd
Compare
a2fc7dd
to
7a7b025
Compare
Signed-off-by: Anuj Agrawal <[email protected]> Added tests for cmd/descheduler Signed-off-by: Anuj Agrawal <[email protected]>
7a7b025
to
9d5e86e
Compare
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.
Good job!
/lgtm
/approve
assert.NotEmpty(t, cmd.Long) | ||
} | ||
|
||
func TestDeschedulerCommandFlagParsing(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.
This test case feels good 👍
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.
I tried to keep it maintainable and not prone to break with minor changes.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: XiShanYongYe-Chang 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:
Approvers can indicate their approval by writing |
Hey @XiShanYongYe-Chang is there a need to test cmd/descheduler/app/options/options.go? |
Is it possible to make a valid test? If so, please add it. |
Description:
This PR introduces tests for cmd/descheduler/app/descheduler.go .
What type of PR is this?
/kind failing-test
/kind feature
What this PR does / why we need it:
This PR adds comprehensive tests for testing core functionality of cmd/descheduler .
Which issue(s) this PR fixes:
Fixes a part of #5236
Does this PR introduce a user-facing change?: