-
Notifications
You must be signed in to change notification settings - Fork 247
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
Bug 1889721: add skippatch unit test #505
Bug 1889721: add skippatch unit test #505
Conversation
@ankitathomas: This pull request references Bugzilla bug 1889721, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
expected: true, | ||
}, | ||
{ | ||
name: "accept patch version without pre-release", |
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.
@ankitathomas if it is the following data,
{
name: "do not accept patch version with pre-release",
added: "0.0.0-1",
compare: "0.0.0",
expected: false,
},
is it correct? I think so. thanks
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.
@ankitathomas To add on to this, if f(a, c) -> !f(c, a)
and !f(a, c) -> f(c, a)
, then this test could automatically verify both for each case.
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've added checking for the absence of a commutative edge
{ | ||
name: "do not accept different major/minor version", | ||
added: "0.0.0", | ||
compare: "0.0.0", | ||
expected: false, | ||
}, |
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.
It looks like you meant to change the inputs for this case?
expected: true, | ||
}, | ||
{ | ||
name: "accept patch version without pre-release", |
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.
@ankitathomas To add on to this, if f(a, c) -> !f(c, a)
and !f(a, c) -> f(c, a)
, then this test could automatically verify both for each case.
85a2dd5
to
6d94e8e
Compare
Codecov Report
@@ Coverage Diff @@
## master #505 +/- ##
=========================================
Coverage ? 48.73%
=========================================
Files ? 90
Lines ? 6103
Branches ? 0
=========================================
Hits ? 2974
Misses ? 2469
Partials ? 660 Continue to review full report at Codecov.
|
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.
/approve
actual := isSkipPatchCandidate(added, compare) | ||
assert.Equal(t, tt.expected, actual) | ||
|
||
if !tt.commutative { |
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.
Now that I see it (and recognize that there are cases that don't have this property), I'm not sure that adding a second assertion to the test is better than explicitly writing these cases. I won't ask you to change it again if you disagree, though.
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.
We can make this assertion whenever we have a valid skippatch pair. f(a, c) -> !f(c, a)
holds true, so this should cover all the cases we need
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ankitathomas, benluddy 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 |
/lgtm |
/retest Please review the full test history for this PR and help us cut down flakes. |
6 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
@ankitathomas: All pull requests linked via external trackers have merged: Bugzilla bug 1889721 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
…-fix Bug 1889721: add skippatch unit test
Adding unit tests for isSkipPatchCandidate