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

feat: Add initial JobController reconciler test #27

Merged
merged 13 commits into from
Apr 3, 2022

Conversation

irvinlim
Copy link
Member

@irvinlim irvinlim commented Mar 28, 2022

Adds unit tests, contributing to improved test coverage (#3).

  • Add runtime/testing package that will use fake.Client Actions to capture API requests to test expected API requests made for a single sync.
  • Switch all references to time.Now() and metav1.Now() in jobcontroller.Reconciler to use ktime.Now() instead for fake clock.
  • Add some initial tests just for JobController first.
  • Fix several small bugs.

After writing some initial reconciler tests, I have some thoughts about how to write proper tests in this style (which is probably midway between unit tests and integration tests):

  1. The current test format takes in a list of fixtures to create initially, triggers a SyncOne call, and specifies expected API requests to be made by hooking onto*fake.Client's Reactor mechanism.
    • One technique to avoid hardcoding too much unrelated test data is to compute the resultant state using the same methods that the actual controller uses. This means that if the state computation method changes, we don't have to fix our integration tests, and shifts the responsibility of ensuring the correct state to the state computation method's unit tests instead.
    • Another problem is that since we don't talk to a real Kubernetes API server, certain expected fields that will be added via mutating webhooks will no longer be present, such as creationTimestamp. The tests will then have to take care to use the correct object, which can be easy to trip up.
  2. Reconciler tests could be implemented as a few broad types of tests:
    • BDD-style tests: Given an input target and a few initial resources, define the expected behavior after a single Sync.
    • No-op tests: Given some input, assert that no resources are updated (avoids infinite sync bugs). This is slightly tricky to implement and cover all cases, especially when the clock is mocked and does not advance like in the real world.
    • Delay tests: Given some input, assert that it will be synced again after a maximum duration.
    • Performance tests: Given some input, assert that the number of API requests made is bounded to some fixed amount. (the current style of tests also ensures this, actually)

We can write more full-blown integration tests as described in the Kubebuilder documentation in the future, which could help cover additional test cases but will probably run for significantly longer than the current style of tests.

@irvinlim irvinlim added component/execution Issues or PRs related exclusively to the Execution component (Job, JobConfig) area/testing Related to automated testing labels Mar 28, 2022
@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #27 (e16f5b7) into main (a477b3f) will increase coverage by 8.86%.
The diff coverage is 85.34%.

@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
+ Coverage   38.40%   47.26%   +8.86%     
==========================================
  Files         154      158       +4     
  Lines        7794     7934     +140     
==========================================
+ Hits         2993     3750     +757     
+ Misses       4662     3994     -668     
- Partials      139      190      +51     
Impacted Files Coverage Δ
pkg/execution/controllers/jobcontroller/control.go 76.31% <0.00%> (-6.55%) ⬇️
.../execution/controllers/jobcontroller/controller.go 30.55% <58.33%> (+30.55%) ⬆️
pkg/utils/testutils/time.go 61.53% <61.53%> (ø)
.../execution/controllers/jobcontroller/reconciler.go 67.18% <72.72%> (+67.18%) ⬆️
...ecution/taskexecutor/podtaskexecutor/pod_client.go 70.45% <100.00%> (ø)
...ntime/controllercontext/mock/context_clientsets.go 100.00% <100.00%> (ø)
pkg/runtime/testing/action.go 100.00% <100.00%> (ø)
pkg/runtime/testing/schema.go 100.00% <100.00%> (ø)
pkg/runtime/testing/testing.go 100.00% <100.00%> (ø)
pkg/utils/execution/job/condition.go 100.00% <100.00%> (ø)
... and 21 more

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 a477b3f...e16f5b7. Read the comment docs.

@irvinlim irvinlim requested a review from phanhuyn March 29, 2022 05:48
@irvinlim irvinlim merged commit 31d88de into main Apr 3, 2022
@irvinlim irvinlim deleted the irvinlim/feat/job-controller-tests branch April 3, 2022 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Related to automated testing component/execution Issues or PRs related exclusively to the Execution component (Job, JobConfig)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant