-
Notifications
You must be signed in to change notification settings - Fork 13
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
fix issue 26 #27
fix issue 26 #27
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.
Thanks for the fix and especially for the test code, but I have some ideas for improvement:
kaitai/stream_test.go
Outdated
case whence == io.SeekCurrent && fr.pos != initialPosition: | ||
return 0, errors.New("not allowed to seek to the current pos") |
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'm not sure if I fully understand this case. I guess that you're creating the error returned to the first k.Pos()
called after k.Seek(0, io.SeekEnd)
, but this piece code is not exactly self-explanatory. It took me a while to figure out what do you want to achieve by the cryptic condition fr.pos != initialPosition
.
And "not allowed to seek to the current pos"
is not a very helpful message either (or let's call it a comment, because it's not printed anywhere). It made me think at first that instead of whence == io.SeekCurrent && fr.pos != initialPosition
you must have meant whence == io.SeekCurrent && offset == 0
(so as not to allow seeking to the current fr.pos
).
What about this:
switch {
case whence == io.SeekCurrent && fr.pos == -1:
return 0, errors.New("artificial error when seeking with io.SeekCurrent after seeking to end")
// ...
default: // whence == io.SeekEnd
fr.pos = -1
}
It would help me if I saw the code for the first time.
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 agree. Done.
Co-authored-by: Petr Pučil <[email protected]>
kaitai/stream.go
Outdated
@@ -63,19 +63,17 @@ func (k *Stream) Size() (int64, error) { | |||
if err != nil { | |||
return 0, err | |||
} | |||
// Deferred Seek back to the current position | |||
defer k.Seek(curPos, io.SeekStart) |
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.
The tool https://github.com/golangci/golangci-lint noticed this problem:
$ ./golangci-lint-1.35.2-windows-amd64/golangci-lint run kaitai
kaitai\stream.go:67:14: Error return value of `k.Seek` is not checked (errcheck)
defer k.Seek(curPos, io.SeekStart)
^
This might potentially lead to a bug that this call fails, but because its error return value is ignored, k.Size()
will happily return err == nil
(despite the fact that the stream pointer failed to return to the original position for some reason and we're clearly in a situation of failure).
I think the code should be like this:
-func (k *Stream) Size() (int64, error) {
+func (k *Stream) Size() (int64, err error) {
defer k.Seek(curPos, io.SeekStart) | |
defer func() { | |
if _, serr := k.Seek(curPos, io.SeekStart); serr != nil { | |
err = serr | |
} | |
}() |
See https://yourbasic.org/golang/defer/#use-func-to-return-a-value and https://blog.learngoprogramming.com/5-gotchas-of-defer-in-go-golang-part-iii-36a1ab3d6ef1#afb5.
Could you please adjust the tests to check this case as well?
Perhaps we can make the failingReader
more generic by restoring its normal behavior (i.e. io.SeekEnd
behaving as normal, not setting pos
to -1
) and instead passing a param in it (e.g. numSeeksLimit
) that limits the number of succeeding Seek()
calls. Then we can just go iterate 0..(numSeeksThatSizeMethodDoes - 1)
and check for each limit that the Size()
function always returns an error and restores the stream position to its original state.
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 two test cases:
- fails to get the initial pos
- the deferred seek fails
errorCheck: func(err error) bool { | ||
_, ok := err.(artificialError) | ||
return ok | ||
}, |
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 don't quite get why this check function is declared for each case individually. All cases use failingReader
:
kaitai_struct_go_runtime/kaitai/stream_test.go
Lines 151 to 153 in 3e00f49
fr := &failingReader{tt.initialPos, tt.failingCondition} | |
s := NewStream(fr) | |
_, err := s.Size() |
which in turn always returns artificialError
if failingCondition
(also known as mustFail
) is true:
kaitai_struct_go_runtime/kaitai/stream_test.go
Lines 91 to 93 in 3e00f49
if fr.mustFail(*fr, offset, whence) { | |
return 0, artificialError{} | |
} |
So I'd rather replace stream_test.go:159-161
with the previous version:
kaitai_struct_go_runtime/kaitai/stream_test.go
Lines 114 to 117 in b841c18
_, isArtificialErr := err.(artificialError) | |
if !isArtificialErr { | |
t.Fatalf("Expected error of type %T, got one of type %T", artificialError{}, err) | |
} |
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.
Not all cases return artificialError
: when the deferred seek fails it returns an artificialError
wrapped into a fmt.wrapError
.
A more explicit definition could be:
func TestErrorHandlingInStream_Size(t *testing.T) {
isArtificialError := func(err error) bool {
_, ok := err.(artificialError)
return ok
}
isWrappedArtificialError := func(err error) bool {
_, ok := errors.Unwrap(err).(artificialError)
return ok
}
tests := map[string]struct {
initialPos int64
failingCondition func(fr failingReader, offset int64, whence int) bool
errorCheck func(err error) bool
wantFinalPos int64
}{
"fails to get initial position": {
initialPos: 5,
failingCondition: func(fr failingReader, offset int64, whence int) bool {
return whence == io.SeekCurrent && offset == 0
},
errorCheck: isArtificialError,
wantFinalPos: 5,
},
"seek to the end fails": {
initialPos: 5,
failingCondition: func(fr failingReader, offset int64, whence int) bool {
return whence == io.SeekEnd
},
errorCheck: isArtificialError,
wantFinalPos: 5,
},
"deferred seek to the initial pos fails": {
initialPos: 5,
failingCondition: func(fr failingReader, offset int64, whence int) bool {
return whence == io.SeekStart && fr.pos == -1
},
errorCheck: isWrappedArtificialError,
wantFinalPos: -1,
},
}
Anyway, you can go ahead, pick the version you like the most and modify the PR consequently.
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.
Not all cases return
artificialError
: when the deferred seek fails it returns anartificialError
wrapped into afmt.wrapError
.
Oh, thanks, I didn't notice that. I didn't see this little exclamation mark in !ok
here:
kaitai_struct_go_runtime/kaitai/stream_test.go
Lines 142 to 145 in fc91f26
errorCheck: func(err error) bool { | |
_, ok := err.(artificialError) | |
return !ok | |
}, |
The above "check" is totally unacceptable because it only checks that the artificialError
is either wrapped somewhere inside or we're dealing with a completely different error that does not have anything in common with artificialError
at all. It is also deceptive because it looks so similar to return ok
, which would make more sense.
But it turns out that the linter knows about the problem that wrapped errors cannot be simply type asserted, and it suggested a solution:
$ ./golangci-lint-1.35.2-windows-amd64/golangci-lint run kaitai --no-config -E errorlint
...
kaitai\stream_test.go:121:14: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
_, ok := err.(artificialError)
^
So I believe that this silver bullet errors.As
should work for everything:
+ var wantErr artificialError
- _, isArtificialErr := err.(artificialError)
+ _, isArtificialErr := errors.As(err, &wantErr)
if !isArtificialErr {
- t.Fatalf("Expected error of type %T, got one of type %T", artificialError{}, err)
+ t.Fatalf("Expected error of type %T, got one of type %T", wantErr, err)
}
See https://play.golang.org/p/1FN6BVmqRWV for a demo.
Co-authored-by: Petr Pučil <[email protected]>
Closes #26