-
Notifications
You must be signed in to change notification settings - Fork 189
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 corner cases for comments.go in astbuilder package #3134
Conversation
Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>
if c.comment == "" { | ||
continue | ||
} |
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.
All this does is to suppress the new test case - it's never run at all, so it adds nothing to the coverage.
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.
Will update the same
if c.width == 0 { | ||
continue | ||
} |
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.
Ditto here; all you're doing is skipping the test if the passed width is zero.
Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>
@@ -70,6 +71,8 @@ func TestWordWrap(t *testing.T) { | |||
{"this is a simple line of text", 16, []string{"this is a simple ", "line of text"}}, | |||
{"this is a simple line of text", 20, []string{"this is a simple ", "line of text"}}, | |||
{"this is a simple line of text", 21, []string{"this is a simple line ", "of text"}}, | |||
{"", 0, []string{}}, | |||
{"this is a sample text", 0, []string{}}, |
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 should never throw away content like this - if someone inadvertently passes a width of 0
we want to return the content anyway. For consistency with existing behaviour when width is 1
, this should be:
{"this is a sample text", 0, []string{}}, | |
{"this is a sample text", 0, []string{"this ", "is ", "a ", "sample ", "text"}}, |
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.
Adding coverage of edge cases is valuable, but don't lose sight of the context the methods are used within.
Signed-off-by: Yash Khare <yash2010118@akgec.ac.in>
/ok-to-test sha=7ee6c95 |
Codecov Report
@@ Coverage Diff @@
## main #3134 +/- ##
==========================================
- Coverage 54.17% 54.15% -0.03%
==========================================
Files 1418 1419 +1
Lines 608163 608072 -91
==========================================
- Hits 329486 329311 -175
- Misses 224662 224764 +102
+ Partials 54015 53997 -18
|
/ok-to-test sha=e3be922 |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Thanks @khareyash05. |
What this PR does / why we need it:
Increase test coverage of
astbuillder/comments.go
to add corner cases of empty comments and text width =0If applicable: