Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #234 +/- ##
==========================================
+ Coverage 71.81% 74.95% +3.14%
==========================================
Files 12 12
Lines 1685 1685
==========================================
+ Hits 1210 1263 +53
+ Misses 378 336 -42
+ Partials 97 86 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f5ea1b8 to
f522502
Compare
base_lexer_test.go
Outdated
| e := syntaxError{s: "hello", i: 1} | ||
| got := e.Error() | ||
| want := `sdp: syntax error at pos 1: "e"` | ||
| assert.Equal(t, want, got) |
There was a problem hiding this comment.
Maybe we can assert the Error type not the message, which can change?
There was a problem hiding this comment.
Oops, I was quickly throwing stuff together, I meant for this to be a draft pr since I wanted to add more coverage across a couple files. Thanks for catching this, I'll update all the instances like this.
There was a problem hiding this comment.
Oh ok this is a little confusing. the e.Error() line returns a String instead of an actual error:
func (e syntaxError) Error() string {
if e.i < 0 {
e.i = 0
}
return fmt.Sprintf("sdp: syntax error at pos %d: %s", e.i, strconv.QuoteToASCII(e.s[e.i:e.i+1]))
}So I don't think we can do an assert.ErrorIs(...) here.
There was a problem hiding this comment.
I meant if that error returns any value, maybe include the digit, but not the full message, because this error message isn't great and we can change it in the future, many times :)
There was a problem hiding this comment.
It's a nitpick that is up to you, you can merge it as it's.
There was a problem hiding this comment.
Oh I see, I misinterpreted what you were saying. I changed it to just check the int now!
797b180 to
24298b8
Compare
24298b8 to
781e7e1
Compare
|
Unfortunately I wasn't able to finish this today, but I should be able to complete it tomorrow. The current cov is roughly 75%, I'll be shooting for 80% or more! |
|
@philipch07 You can merge this for now if you want :) |
Sure. Thanks for reviewing! |
Description
title
Reference issue
Resolves #193.