-
Notifications
You must be signed in to change notification settings - Fork 762
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
[Tests] Moving test cases comments to dedicated field #881
[Tests] Moving test cases comments to dedicated field #881
Conversation
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.
Looks good to me
exchange/bidder_test.go
Outdated
assert.Equal(t, false, (seatBid == nil && tc.expectedBidsCount != 0), fmt.Sprint("Case:", strings.Join(tc.bidCurrency, ","))) | ||
assert.Equal(t, tc.expectedBidsCount, uint(len(seatBid.bids)), fmt.Sprint("Case:", strings.Join(tc.bidCurrency, ","))) | ||
assert.ElementsMatch(t, tc.expectedBadCurrencyErrors, errs, fmt.Sprint("Case:", strings.Join(tc.bidCurrency, ","))) | ||
assert.Equal(t, false, (seatBid == nil && tc.expectedBidsCount != 0), fmt.Sprint("Case:", strings.Join(tc.bidCurrency, ",")), tc.description) |
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.
Like this approach of adding the descriptions to the table test cases. One question though, since the descriptions are now added, do we still need the existing error messages: fmt.Sprint("Case:", strings.Join(tc.bidCurrency, ","))
?
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.
Actually, you are right, we don't need it any longer, I can remove it :)
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.
Just updated the PR, thanks @mansinahar :)
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.
Awesome. Thanks @benjaminch !
This CL only introduces a change in couple tests having test cases comments moving those comments in a dedicated field in the test case so it will be printed by assert on fail which ease the debug investigations.
30741e6
to
76b5653
Compare
Thanks @mansinahar :) |
Of course! Thanks for the PR @benjaminch :) |
This CL only introduces a change in couple tests having test cases comments moving those comments in a dedicated field in the test case so it will be printed by assert on fail which ease the debug investigations.
This CL only introduces a change in couple tests having test cases comments moving those comments in a dedicated field in the test case so it will be printed by assert on fail which ease the debug investigations.
This CL only introduces a change in couple tests having test cases comments moving those comments in a dedicated field in the test case so it will be printed by assert on fail which ease the debug investigations.
This CL only introduces a change in couple tests having test cases
comments moving those comments in a dedicated field in the test case so
it will be printed by assert on fail which ease the debug
investigations.