Deployment unit tests #236
Conversation
| status: True | ||
| lastUpdateTime: "2018-01-09 22:56:45 UTC" | ||
|
|
||
| --- |
There was a problem hiding this comment.
Seems like this should be two files.
There was a problem hiding this comment.
My thinking is to have one fixture file per unit test file so it's clear what they're intended for, but admittedly I have no idea if that will meet our real needs when we have more of these. Another reason to keep them together is that they collectively represent a single deployed resource (the Deployment), including the cluster-side results (the RS) of shipping that resource. So they actually contain cross references (ownerRef, uid).
| refute deploy.validate_definition | ||
| private | ||
|
|
||
| def build_synced_deployment(status:, new_rs_status:, rollout: nil, max_unavail:) |
There was a problem hiding this comment.
I found status and new_rs_status confusing. How would you feel about status => deployment_status and new_rs_status => rs_status.
Does it make sense to default max_unavail to false and the other two params to {}?
There was a problem hiding this comment.
Good points. I'll try to rework this a bit.
| [spec.to_json, "", SystemExit.new(0)] | ||
| ) | ||
|
|
||
| replicasets = { "items" => [] } |
There was a problem hiding this comment.
Should the building of the rs and deployment be their own functions?
|
|
||
| expected = <<~STRING.strip | ||
| super failed | ||
| 'kubernetes-deploy.shopify.io/required-rollout: bad' is invalid. Acceptable values: maxUnavailable, full, none |
There was a problem hiding this comment.
Its a bit brittle to test against a string. You could expect rollout_annotation_err_msg to be called, though I have mixed feelings about testing for a private function being called too
There was a problem hiding this comment.
True, but IMO the value of asserting something about it is seeing that the full string makes sense. I actually caught a few errors when I wrote these assertions (spacing/punctuation problems, but more importantly that "is invalid with strategy 'rollingUpdate'" was saying the opposite of what it meant, which I didn't realize until I read the whole thing in the test). I can at least reference the constant instead of hard-coding the annotation though.
|
Updated |
| replica_sets: [build_rs_template] | ||
| ) | ||
| msg = "'#{KubernetesDeploy::Deployment::REQUIRED_ROLLOUT_ANNOTATION}: bad' is "\ | ||
| "invalid. Acceptable values: maxUnavailable, full, none" |
There was a problem hiding this comment.
Can you use REQUIRED_ROLLOUT_TYPES here instead of maxUnavailable, full, none.
There was a problem hiding this comment.
I can if you feel strongly, but my opinion is that the further these get from showing the human-readable message, the less valuable the assertion is. The fact that they were written out made me notice that our join was missing a space.
| } | ||
| dep_template = build_deployment_template(status: deployment_status, rollout: 'none', | ||
| strategy: 'RollingUpdate', max_unavailable: 1) | ||
| deploy = build_synced_deployment(template: dep_template, replica_sets: [build_rs_template(status: rs_status)]) |
There was a problem hiding this comment.
Should have made this more clear. I was thinking that build_synced_deployment would call build_rs_template & build_deployment_template. But this works if you like it better.
There was a problem hiding this comment.
I think this will be more flexible, in that a single test with esoteric needs could modify a field in the template before passing it into build_synced_deployment. This als makes it easy to write tests that need more than one replica set.
|
I'm good with the PR as is.
…On Wed, Jan 10, 2018 at 1:09 PM, Katrina Verey ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/unit/kubernetes-deploy/kubernetes_resource/deployment_test.rb
<#236 (comment)>
:
> refute deploy.deploy_succeeded?
end
- def test_deploy_succeeded_fails_with_max_unavailable_as_a_percent
- rollout = {
- 'metadata' => {
- 'name' => 'fake',
- 'annotations' => { KubernetesDeploy::Deployment::REQUIRED_ROLLOUT_ANNOTATION => 'maxUnavailable' }
- }
- }
+ def test_deploy_succeeded_raises_with_invalid_rollout_annotation
+ deploy = build_synced_deployment(
+ template: build_deployment_template(rollout: 'bad'),
+ replica_sets: [build_rs_template]
+ )
+ msg = "'#{KubernetesDeploy::Deployment::REQUIRED_ROLLOUT_ANNOTATION}: bad' is "\
+ "invalid. Acceptable values: maxUnavailable, full, none"
I can if you feel strongly, but my opinion is that the further these get
from showing the human-readable message, the less valuable the assertion
is. The fact that they were written out made me notice that our join was
missing a space.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#236 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALoOBldUPpfBoXEARgwzqaRqJ6PtePx7ks5tJScVgaJpZM4RYpdB>
.
|
| result | ||
| end | ||
|
|
||
| def build_synced_deployment(template:, replica_sets:) |
There was a problem hiding this comment.
I would rename this, perhaps:
build_deployment_from_test_fixture(...)
As a lay person it took me several read throughs to understand that this function creates deployment objects from static test assets.
I think the name indicates that it is syncing something with the cluster.
There was a problem hiding this comment.
Hm, it doesn't actually build it from a fixture though, now that build_deployment_template is a separate helper... It is creating a resource instance, and making that instance look as though sync returned the template arg. So it is in fact supposed to be simulating a sync with the cluster.
There was a problem hiding this comment.
So it is in fact supposed to be simulating a sync with the cluster.
Right, it just wasn't immediately obvious which way stuff was syncing (in this case fixture -> object, as opposed to cluster -> object). Feel free to ignore this comment.
|
|
||
| expected = <<~STRING.strip | ||
| super failed | ||
| '#{KubernetesDeploy::Deployment::REQUIRED_ROLLOUT_ANNOTATION}: bad' is invalid. Acceptable values: maxUnavailable, full, none |
There was a problem hiding this comment.
Perhaps:
Acceptable values: #{REQUIRED_ROLLOUT_TYPES.join(', ')}
... alternatively don't test for "Acceptable values" at all?
There was a problem hiding this comment.
Danny had the same suggestion. My opinion is that the assertion isn't as valuable if it is pretty much the same as the originating code. By writing it out the way the user would read it, you can notice things like spacing/punctuation errors much more easily. But I'm clearly in the minority on this one so I'll change it. 😄
There was a problem hiding this comment.
My opinion is that the assertion isn't as valuable if it is pretty much the same as the originating code.
True, which is why I wonder if it's worthwhile checking for the "Acceptable values".
Effectively that tests that the list hasn't changed (and also punctuation).
I'm ok with it either way.
KnVerey
force-pushed
the
deploy_unit_tests
branch
from
ffb648b
to
d6004c1
Compare
|
I changed the |
@dturn @stefanmb
This PR is based on #208. It proposes an alternative way to unit test the new changes to
Deployment, and adds a few unrelated tests as well to try to demonstrate the usefulness of the new helper. The main idea is to avoid calling/stubbing private methods, or setting private instance variables. It does this using a helper that callssyncand stubs kubectl responses. Gettingsyncinvolved in every test is kinda 💩 , but all the methods worth testing depend heavily on its data. LMK what you think.