Add some tests for Sync

[dgageot]

No description provided.

[codecov-io]

Codecov Report

Merging #1406 into master will increase coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1406      +/-   ##
==========================================
+ Coverage   44.65%   44.83%   +0.17%     
==========================================
  Files         111      111              
  Lines        4546     4546              
==========================================
+ Hits         2030     2038       +8     
+ Misses       2311     2304       -7     
+ Partials      205      204       -1

Impacted Files	Coverage Δ
pkg/skaffold/runner/dev.go	52.77% <0%> (+8.33%)	⬆️
pkg/skaffold/runner/changes.go	100% <0%> (+16.66%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4da3012...eac9151. Read the comment docs.

[codecov-io]

Codecov Report

Merging #1406 into master will increase coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1406      +/-   ##
==========================================
+ Coverage   44.65%   44.83%   +0.17%     
==========================================
  Files         111      111              
  Lines        4546     4546              
==========================================
+ Hits         2030     2038       +8     
+ Misses       2311     2304       -7     
+ Partials      205      204       -1

Impacted Files	Coverage Δ
pkg/skaffold/runner/dev.go	52.77% <0%> (+8.33%)	⬆️
pkg/skaffold/runner/changes.go	100% <0%> (+16.66%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45e6a30...64289f8. Read the comment docs.

[balopat]

I might be missing something and this might be totally idiomatic go - but as a first look, I'm not a big fan of the TestBench concept - because it introduces a new abstraction that is not really necessary and is confusing and will have to implement everything that a Runner would need vs. the separate TestBuilder, TestTester, etc. It seems that TestBench is only used to create a Runner anyways. How about a RunnerBuilder using the builder pattern with fluid syntax, which would in the end give you a MockRunner - e.g. for testcases you could pass in

runner: runnerBuilder().WithBuildErrors([]error{errors.New("")}).Build(),

to capture actions, you could pass the MockRunner instance to TestBuilder, TestTester, etc. and they can record the calls there - or alternatively they can record it themselves and MockRunner could collect it from them on the Actions() call. What do you think?

[dgageot]

The idea here is I think the opposite of what you’re saying. The TestBench does not produce a MockRunner, it creates an actual runner with mocked dependencies. To test the code of the Runner, we test need a TestBuilder, a TestTester, a TestSyncer and a TestDeployer. The TestBench implements all those deps, with a shared state that facilitates the assertions. It’s teally a bench on which you plug a runner and collect information. Is it clearer?

[balopat]

I see, true, calling it a MockRunner would miss the point. Let's go with it, I just haven't seen this before - maybe some comments can help clarifying the responsibility of TestBench that it's a facade for all of the mocked collaborators of Runner.

[balopat]

Not even a facade - it's a "union mock" of all collaborators :)