-
Notifications
You must be signed in to change notification settings - Fork 154
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
Semver unit tests for chain package #2293
Conversation
Signed-off-by: Philip-21 <[email protected]>
@jrick @peterzen @buck54321 this is my first time contributing to the decred project . I just decided to push this pr to get started with things. |
FYI, there's usually no need to ping any developers. Your PR will be reviewed as people have availability to do so. |
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.
Thank you for your first contribution! The added tests are reasonable, and while this is a very small contribution, I understand you're just looking to get started with helping with the project.
In light of that, I've been extra thorough on the review, marking improvements and stylistic changes that usually wouldn't be an issue in a standard PR (and usually wouldn't be raised) but should serve as hints for improvements to your future contributions.
"testing" | ||
) | ||
|
||
func TestSemverCompatible(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.
You might wanna add a small comment explaining what exactly you are testing.
chain/semver_test.go
Outdated
} | ||
}) | ||
} | ||
t.Log("Test Passed") |
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.
No need for this. If the test fails, then the standard go tooling will let you know.
chain/semver_test.go
Outdated
} | ||
}) | ||
} | ||
t.Log("Test Passed") |
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.
Same as other comment.
chain/semver_test.go
Outdated
t.Run(tc.name, func(t *testing.T) { | ||
result := semverCompatible(tc.required, tc.actual) | ||
if result != tc.expected { | ||
t.Errorf("Expected: %v, got: %v", tc.expected, result) |
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 isn't strictly required, but most of dcrd and other tools use a got: %v, want: %v
idiom so you might want to get used to writing the checks like that.
Also, we usually use Fatalf
, unless you explicitly want the test to continue so you can perform additional assertions. Given that this is a subtest, doing a Fatalf()
won't prevent the other tests from running.
required: semver{Major: 1, Minor: 2, Patch: 3}, | ||
actual: semver{Major: 2, Minor: 0, Patch: 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.
This is the only significant issue I can find while reviewing.
For completeness, you need to add cases where Major is equal and {Minor,Patch} are higher/smaller with the expected result for them.
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.
@matheusd i can use do something like these
required: semver{Major: 1, Minor: 2, Patch: 4},
actual: semver{Major: 1, Minor: 0, Patch: 2},
required: semver{Major: 1, Minor: 3, Patch: 6},
actual: semver{Major: 1, Minor: 3, Patch: 2},
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.
Yes, those seem to fill the missing gaps.
thanks @matheusd for the reviews i will make adjustments |
Signed-off-by: Philip-21 <[email protected]>
changes have been made @matheusd |
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 now, thank you!
for future reference, we don't require commits to be "signed-off". |
Unit test Created for Semver functions in the chain package