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

Test the event handling; and separate the cause detection from handling #82

Merged
merged 17 commits into from
May 29, 2019
Merged

Test the event handling; and separate the cause detection from handling #82

merged 17 commits into from
May 29, 2019

Conversation

nolar
Copy link
Contributor

@nolar nolar commented May 28, 2019

Issue: #13

Briefly: add tests for the core part of Kopf: event handling.

This covers the whole chain of handling: from the raw streaming/watching event reception and down to the handler function invocation.

Two sections are explicitly separated from the previous handling routine — to make them testable:

  • Cause detection (kopf.reactor.causation) — for converting all possible event type and object fields combinations to the cause objects.
  • Cause handling (kopf.reactor.handling) — for actual handler invocation, logging, exception handling, status/progress storage, patching, sleeping, etc.

As part of this PR, a refactoring is made to split these two parts into separate modules, so that become testable.

Sergey Vasilyev added 15 commits May 28, 2019 17:38
Fix the typos in the exception messages
0cfa355
Fix the dummy-patch storage (status, not root of the object)
974ddf7
Exclude the timestamps from the last-seen state comparison
c4f9a27
Exclude the empty structs (after purging) from the last-seen state
98d6090
Append/remove the finalizers only if absent/present, ignore otherwise
e7998c1
Help to check & assert the captured logs
376f654
Extract the cause-related code to a separate module (as is)
61249db
Split the cause detection and handling
0dc7cd9
Log the human-readable names of the causes
971e807
Test the cause detection with all possible state combinations
3100e58
Test the operator freezing mode
e6bbe7e
Test the handling of causes end-to-end
a3de0ed
Test the delayed handlers
d79681e
Test the multi-step handler invocation with intermediate progress
0e727dc
Test errors and timeouts in handling
d448e8c
@nolar nolar added the automation CI/CD: testing, linting, releasing automatically label May 28, 2019
@nolar nolar added this to the 1.0 milestone May 28, 2019
@nolar nolar requested a review from samurang87 as a code owner May 28, 2019 15:43
@zincr
Copy link

zincr bot commented May 28, 2019 •
edited
Loading

🤖 zincr found 0 problems , 1 warning

ℹ️ Large Commits
✅ Approvals
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below

Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

  • ℹ️ kopf/reactor/causation.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ kopf/reactor/handling.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/causation/test_detection.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/handling/conftest.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/handling/test_errors.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/handling/test_handling.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
     

@zincr
Copy link

zincr bot commented May 28, 2019

🤖 zincr found 1 problem , 1 warning

❌ Approvals
ℹ️ Large Commits
✅ Specification
✅ Dependency Licensing

Details on how to resolve are provided below

Approvals

All proposed changes must be reviewed by project maintainers before they can be merged

Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.

  • ✅ Approved by PR author @nolar
  • ❌ 1 additional approval needed
     

Large Commits

Checks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues

This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of

  • ℹ️ kopf/reactor/causation.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ kopf/reactor/handling.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/causation/test_detection.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/handling/conftest.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/handling/test_errors.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
  • ℹ️ tests/handling/test_handling.py had +100 lines of code changed in a single commit
    Please review this file to determine the source of this change
     

@nolar nolar changed the title Causation Test the event handling; and separate the cause detection from handling May 28, 2019
@nolar nolar mentioned this pull request May 28, 2019
Closed
19 tasks
samurang87
samurang87 reviewed May 29, 2019
View reviewed changes
kopf/reactor/causation.py Outdated
body = event['object']

# The object was really deleted from the cluster. But we do not care anymore.
if etyp == 'DELETED':
Copy link

Choose a reason for hiding this comment

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

maybe "event_type" instead of "etyp"? I don't like abbreviations/acronyms :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by removing the variable completely. It is used only in one place now (previously (long time ago) there were 2-3 places).

samurang87
samurang87 reviewed May 29, 2019
View reviewed changes
tests/handling/conftest.py Outdated
* Registered handlers in a global registry. Each handler is a normal function,
which calls a mock -- to ease the assertions.

As the output, we check the mocked calls on the following:
Copy link

@samurang87 samurang87 May 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Output:

Check mocked calls of: ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure here about "check mock calls". Fixed to "check mockED calls". Why is it "mock calls"?

Copy link

Choose a reason for hiding this comment

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

I edited :)

Copy link

Choose a reason for hiding this comment

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

Actually I'd have made it less verbose, for instance
`Output
Check mocked calls of:

  • bla
  • bla `

Copy link

Choose a reason for hiding this comment

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

mkay, markdown is not helping

Copy link

Choose a reason for hiding this comment

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

Otherwise yes it's ok to have the "the" in there :)

@nolar nolar merged commit a2889ac into zalando-incubator:master May 29, 2019
@nolar nolar deleted the causation branch June 2, 2019 16:54
@nolar nolar mentioned this pull request Jun 6, 2019
Merged
@kopf-archiver kopf-archiver bot mentioned this pull request Aug 19, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@samurang87 samurang87 samurang87 approved these changes

Assignees
No one assigned
Labels
automation CI/CD: testing, linting, releasing automatically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nolar @samurang87