Skip to content
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

chore: enable full test coverage for Windows #206

Merged
merged 11 commits into from Apr 8, 2024

Conversation

johnmaguire
Copy link
Contributor

After an unsuccessful night debugging #92, I decided to create a benchmark to try to narrow down the cause of the slowness. Unfortunately, I ran into unexplained "Access denied" errors.

I ran the test suite on Windows and saw similar errors in many tests I read on Stack Overflow that AV can be an issue here, so I turned off Windows Defender RTP, to no avail. I think this may be related to the AppData issues mentioned here: #200 (comment)

To see if this was an issue with my environment, I enabled tests on Github where I discovered the "Access denied" errors do not occur. I have resolved most of the remaining test failures in individual commits for your review.

I'm not sure if there was an actual bug with the ls command on Windows, but I received the following error during TestLs`:

restic_test.go:261: failed to list directory: command "C:\\Program Files (x86)/restic/restic-0.16.4.exe ls --json 0bca3b482c620f384eae2ca000f7d5ce212171049e5563dd16bc4591345407d7 C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\TestLs1936002979\\002 --no-cache" failed: exit status 1
    Process STDOUT:
    Fatal: All path filters must be absolute, starting with a forward slash '/'

I resolved it with a toPathFilter command for Windows.


However, there seems to be an issue with queue ordering. I am seeing occasional failures in the TestTaskQueueOrdering test, and persistent failures in TestFuzzTaskQueue. These functions looked a bit dense, so I figured I'd post the PR as-is and see if you had any thoughts on the queue.


This PR also enables running the full test suite in Github, to encourage keeping them green. Happy to revert that part if you simply want to improve the existing tests, and continue the current testing strategy.

pkg/restic/path_win.go Outdated Show resolved Hide resolved
pkg/restic/path_win.go Outdated Show resolved Hide resolved
pkg/restic/restic.go Outdated Show resolved Hide resolved
@garethgeorge garethgeorge changed the title Enable tests for Windows chore: enable full test coverage for Windows Apr 7, 2024
@garethgeorge
Copy link
Owner

Hey, this is awesome! Thanks for tracking the windows breakages down and patching these, left a few questions about the changes in the restic pkg -- mostly wondering what you ran into / what motivated them (and saw some places where we can maybe simplify).

The task queue failures are interesting -- I remember seeing those and better coverage of (and an improved implementation of) the task queue is actually what's currently on my plate as I suspect there are some bugs in there related to #180. It's possible a related issue is surfacing in the windows fuzztest.

For now I'm happy to land this test enablement though, do you mind adding t.Skip() to those remaining failures for the time being? :)

Overall this is hugely appreciated -- thanks for contributing it!

```
restic_test.go:261: failed to list directory: command "C:\\Program Files (x86)/restic/restic-0.16.4.exe ls --json 0bca3b482c620f384eae2ca000f7d5ce212171049e5563dd16bc4591345407d7 C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\TestLs1936002979\\002 --no-cache" failed: exit status 1
    Process STDOUT:
    Fatal: All path filters must be absolute, starting with a forward slash '/'
```
@johnmaguire
Copy link
Contributor Author

Should be good to go! I do wonder if the task queue tests might actually be a time resolution issue? I think in general, Linux has more precise time resolutions than on Windows. I have to run now, or I'd try changing the Millisecond reference to Second in them.

I skipped the two I've seen fail, but it's possible there are other flaky tests as well. Not sure if it'd help here, but in some of my projects, I enable -race detector in CI tests.

@garethgeorge garethgeorge merged commit ffad2b0 into garethgeorge:main Apr 8, 2024
2 checks passed
@garethgeorge
Copy link
Owner

garethgeorge commented Apr 8, 2024

Time resolution could definitely be at play I've gone ahead and reworked the task queues as a more modular implementation which is easier to cover with tests (and understand), the new coverage seems to pass on windows which is an improvement!

Enabling -race detector is a good idea, doing some local runs I'm seeing there are a couple read / write races in the IO handling when executing restic commands, I'll work through those and get some fixes out (they're all in the test code -- I'll need to check if this ever happens at runtime).

@johnmaguire johnmaguire deleted the windows-test branch April 11, 2024 07:11
@garethgeorge
Copy link
Owner

Went ahead and submitted a patch for some minor races found by enabling --race , this was a great idea. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants