Skip to content
This repository has been archived by the owner on Sep 14, 2020. It is now read-only.

Test diffs calculation and manipulation #40

Merged
merged 2 commits into from
Apr 26, 2019

Conversation

nolar
Copy link
Contributor

@nolar nolar commented Apr 22, 2019

Issue : #13

Diffs are used in the kopf.on.update handlers, and then in the per-field handlers. They are very minimalistic and very simple structures, easy to test.

@zincr
Copy link

zincr bot commented Apr 22, 2019

🤖 zincr found 0 problems , 0 warnings

✅ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

@nolar nolar marked this pull request as ready for review April 22, 2019 23:45
@nolar nolar requested a review from samurang87 as a code owner April 22, 2019 23:45
@nolar nolar mentioned this pull request Apr 22, 2019
19 tasks
a = {'main': {'hello': 'world', 'key': 'old'}}
b = {'main': {'hello': 'world', 'key': 'new'}}
d = diff(a, b)
assert d == (('change', ('main', 'key'), 'old', 'new'),)
Copy link
Contributor

@parking52 parking52 Apr 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to have a new test with a real usecase with Dicts that the operator would be handling : following the concept of 'test as documentation'

Suggested change
assert d == (('change', ('main', 'key'), 'old', 'new'),)
assert d == (('change', ('main', 'key'), 'old', 'new'),)
def test_dicts_sample_kopf_usecase_resource_update():
body_before_update = {'metadata' : {'resourceVersion' : 'myResourceV1'}}
body_after_update = {'metadata' : {'resourceVersion' : 'myResourceV2'}}
d = diff(body_before_update, body_after_update)
d == (('change', ('metadata', 'resourceVersion'), 'myResourceV1', 'myResourceV2'),)

Provided this really happens during a resource update.

Copy link
Contributor Author

@nolar nolar Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parking52 Good idea.

Though, the resourceVersion will never be in the diffs, as this and other system fields are filtered out of the last-seen state calculation (see kopf.structs.lastseen.get_state()) — for the reason that these fields change even when we do not care about these changes.

Only all of the the spec fields & some (non-filtered) metainfo fields, such as metainfo.labels, metainfo.annotations (except few specific annotations) will be taken into account, diffs calculated, and the update-handlers called.

Maybe, you can implement it for all of the mentioned above? I.e.:

  • metainfo.labels.some-label
  • metainfo.annotations.some-annotation
  • spec.field (a string)
  • spec.items (a list of strings)
  • spec.modes (a dict of strings=>strings).

These are the main real-world examples happening in the operators and expected in @kopf.on.field().

See also: examples/obj.yaml, which I use(d) for the local runs and manual testing.

def test_nonmapping_key():
d = {'key': 'val'}
with pytest.raises(TypeError):
resolve(d, ['key', 'sub'])
Copy link
Contributor

@parking52 parking52 Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
resolve(d, ['key', 'sub'])
resolve(d, ['key', 'val'])

I understand from non mapping that the key must be correct though.
(The key is non mapping but 'exists'). Maybe another test ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unexistent key is tested in test_unexisting_key above. There, the data structure before failure is a mapping d['abc']['def'] equal to {'hij': 'val'}, and we tried {'hij': 'val'}['xyz'] — it raises KeyError.

Here, we use the actually existing key — d['key'] — which has a value that is not a mapping: "val". It is not possible to do "val"['key'] — it raises TypeError.

I can probably align this test with the previous one so that they look the same, and then I can select ['abc']['def']['hij']['key'] — with the same effect.

@nolar nolar merged commit 96c2787 into zalando-incubator:master Apr 26, 2019
@nolar nolar deleted the test-diffs branch April 26, 2019 08:05
@nolar nolar added the automation CI/CD: testing, linting, releasing automatically label Apr 26, 2019
@nolar nolar added this to the 1.0 milestone Apr 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automation CI/CD: testing, linting, releasing automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants