Test a seed that changes while an extraction is in progress #221
Conversation
cmd/desync/extract_test.go
Outdated
| // When targeting at least Go 1.17, we can replace the following with "t.Setenv()" | ||
| err := os.Setenv("DESYNC_MOCK_VALID_PLAN", "1") | ||
| t.Cleanup(func() { os.Unsetenv("DESYNC_MOCK_VALID_PLAN") }) |
There was a problem hiding this comment.
Go 1.17 introduced scoped env vars, so these two lines could be replaced with t.Setenv("DESYNC_MOCK_VALID_PLAN", "1")
Github VMs seems to include Go 1.17 now actions/runner-images#5280 but in go.mod the minimum Go version is the 1.15.
Probably not worth it to bump the Go version for just this minor improvement?
There was a problem hiding this comment.
Yes, it's definitely time to bump the min version to 1.17 since 1.18 is out already. Should also update the dependencies at that time.
There was a problem hiding this comment.
Now that I don't use the environment variable anymore, we can probably bump the min version in a separate PR.
cmd/desync/extract_test.go
Outdated
| // When targeting at least Go 1.17, we can replace the following with "t.Setenv()" | ||
| err := os.Setenv("DESYNC_MOCK_VALID_PLAN", "1") | ||
| t.Cleanup(func() { os.Unsetenv("DESYNC_MOCK_VALID_PLAN") }) |
There was a problem hiding this comment.
Yes, it's definitely time to bump the min version to 1.17 since 1.18 is out already. Should also update the dependencies at that time.
sequencer.go
Outdated
| @@ -108,6 +108,10 @@ func (p Plan) Validate(ctx context.Context, n int, pb ProgressBar) (err error) { | |||
| in = make(chan Job) | |||
| fileMap = make(map[string]*os.File) | |||
| ) | |||
| if mock := os.Getenv("DESYNC_MOCK_VALID_PLAN"); mock != "" { | |||
There was a problem hiding this comment.
Not a fan of injecting testing variables via the environment. Let's think of alternatives:
- The test could move into the
desyncpackage rather thanmain. This would provide more control over what is mocked. One could use an interface with a testing-implementation, or a non-exported global variable that the test sets before running - Exporting a variable that can be set from
main. That's not ideal though.
Any other ideas?
There was a problem hiding this comment.
Thanks for the suggestions!
I'll research a bit more about the possible solutions and I'll try to improve this PR.
There was a problem hiding this comment.
After reading the Go documentation and a few articles online, I ended up using a non-exported global variable to flag the mock validation and moved the test in the desync package.
Please let me know if you preferred a different approach instead.
Signed-off-by: Ludovico de Nittis <ludovico.denittis@collabora.com>
|
I guess this PR fell through the cracks. Is it Okay as-is or would you prefer some changes? |
|
Thanks for following up, forgot about this. |
This adds an automated tests for #220.
I added an environment variable in
sequencerbecause that seemed the cleaner way to mock theValidate()function.Please let me know if instead there is a better way to do that.