windows: skip TestJSONFileLoggerWithOpts on sharing violation #39856
Conversation
There was a problem hiding this comment.
I think this is the syscall error corresponding with the error we're running into, but perhaps someone working on a Windows machine can double-check
There was a problem hiding this comment.
By actual testing using a file locking utility [https://www.jensscheffler.de/filelocker], I observed that it is os.PathError and not os.SyscallError.
Trying to print it:
Err3 &{%!!(string=open) %!!(string=ttr) %!!(syscall.Errno=32)}V
So the check against windows.ERROR_SHARING_VIOLATION stays the same because its value is 32.
Why not bypass all this and just check the exact error string - just trying to learn best practices here.
There was a problem hiding this comment.
Errorstrings usually are for humans, and may change (or even be localised in the Windows case), so trying to avoid string matching where possible.
Perhaps this particular error is "stable", but we had a case (need to search for it) where the message changed in Windows RS4 or RS5, causing things to break
thaJeztah
force-pushed
the
conditionally_skip_TestJSONFileLoggerWithOpts
branch
from
65f594d to
da86706
Compare
|
@ddebroy @StefanScherer @vikramhh PTAL - just a quick 'n dirty attempt to fix the flakiness without always skipping the test. Please verify if this looks good (and if I got the right syscall error 😅 |
|
Windows RS1 is failing on a timeout on |
This test is ocassionally failing because we run into a Windows
sharing violation:
```
--- FAIL: TestJSONFileLoggerWithOpts (0.07s)
jsonfilelog_test.go:187: open C:\Users\ContainerAdministrator\AppData\Local\Temp\docker-logger-344571653\container.log.1: The process cannot access the file because it is being used by another process.
```
This patch attempts to skip the test if we encounter this situation.
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
conditionally_skip_TestJSONFileLoggerWithOpts
branch
from
da86706 to
0641147
Compare
|
@vikramhh updated; changed to check for an |
|
Thanks @thaJeztah - LGTM |
|
@ameyag - I think you had a solution for the Sharing violation issue right? |
|
@ddebroy I think that patch is already here in master (it was a backport?), so perhaps there's still another issue at hand |
|
This could be due to the same root cause as #39274 |
|
Related? golang/go#32088 |
|
Does this mean |
|
If we can address the actual issue, that would be better of course; this was my attempt at doing a quick n dirty hack to at least get CI more stable (and without fully skipping the test) while someone can work on a better solution |
6 participants
hopefully addresses #36801
This test is ocassionally failing because we run into a Windows sharing violation:
This patch attempts to skip the test if we encounter this situation.