-
Notifications
You must be signed in to change notification settings - Fork 8
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 TestLockedByOthers on Windows #19
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
) | ||
|
||
func lockedByOthers(err error) bool { | ||
return errors.Is(err, windows.ERROR_SHARING_VIOLATION) || strings.Contains(err.Error(), "being used by another process") |
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.
return errors.Is(err, windows.ERROR_SHARING_VIOLATION) || strings.Contains(err.Error(), "being used by another process") | |
return errors.Is(err, windows.ERROR_SHARING_VIOLATION) || strings.Contains(err.Error(), windows.ERROR_SHARING_VIOLATION.Error()) |
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 pattern should probably be repeated for the other platforms as well.
That is, in the _posix.go
file, don't use the equality operator to check syscall error values, instead use errors.Is
with them; and call their Error
method too.
If the Unix error value on the lefthand of the OR isn't already matched up with the one we expect to receive from lock.Lock
we can update it.
As is, I don't think the lefthand condition could ever be true, since Lock
(currently) returns a string error, never a syscall.Errno
.
This test was failing on Windows since both the value's being checked are not the same ones used on this platform.
Specifically, this causes
to always be false (on Windows).
The
Is
check I have in this patch is valid, but currently useless since we'll never receive that concrete value from the lock library.The string check should catch this error regardless.
I'm going to submit a patch to the lock library and see what they think about wrapping the error value.
We can wait on that and maybe include a dependency update with this PR if it goes through.