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

Add unittests to slack annotations #9

Merged
merged 41 commits into from
Dec 10, 2024

Conversation

mtomilov
Copy link
Contributor

@mtomilov mtomilov commented Dec 6, 2024

Fixes #6

@mtomilov mtomilov self-assigned this Dec 6, 2024
@mtomilov mtomilov marked this pull request as ready for review December 6, 2024 14:21
@mtomilov mtomilov requested a review from seanh December 6, 2024 14:43
Copy link
Contributor

@seanh seanh left a comment

Choose a reason for hiding this comment

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

👍 I think this is going in a great direction, and I'm actually relieved to see that you've been able to refactor the one giant method into something reasonable :phew:

Just a couple of requests:

  1. I think you should move conditional logic out of notify() and into the helper functions that it calls instead. See below for specific suggestion.
  2. Move the Slack-formatting functions into a separate file
  3. Group the tests for each function together into a test class
  4. For patching use fixtures with autouse=True and autospec=True (see below for a specific suggestion)

tests/unit/slack_annotations/cli_test.py Outdated Show resolved Hide resolved
tests/unit/slack_annotations/cli_test.py Outdated Show resolved Hide resolved
src/slack_annotations/core.py Outdated Show resolved Hide resolved
src/slack_annotations/core.py Outdated Show resolved Hide resolved
src/slack_annotations/core.py Outdated Show resolved Hide resolved
src/slack_annotations/core.py Outdated Show resolved Hide resolved
src/slack_annotations/core.py Show resolved Hide resolved
src/slack_annotations/core.py Outdated Show resolved Hide resolved
Comment on lines 20 to 23
def test_get_search_after_without_cache():
default = "2024-12-01T00:00:00+00:00"

assert _get_search_after("", default) == default
Copy link
Contributor

Choose a reason for hiding this comment

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

There's quite a few separate functions to be tested, so it's probably worth group the tests for each function into a class. Just makes it easier to see where the tests for one function end and the next begins, and also opens up the possibility to localize fixtures into the classes that use them:

class TestGetSearchAfter:
    def test_without_cache(self):
        ...

    ...

class TestNotify:
    ...

class TestFoo:
    ...

...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend doing this in a separate PR, but if we changed cli.py to pass an opened file object into notify() instead of a cache_path string, then we could add a unit test for get_search_after() that passes in a StringIO and properly tests how it reads the search_after value from the JSON file contents.

@mtomilov mtomilov requested a review from seanh December 9, 2024 11:46
Copy link
Contributor

@seanh seanh left a comment

Choose a reason for hiding this comment

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

Alright, I made a few small suggestions but I think let's go ahead and get this merged.

For future PRs I think there'd be value in adding direct unittests for the rest of the helper functions in core.py: looks like currently only notify() and _get_search_after() have unittests.

Also think there'd be value in adding docstrings to the various functions.

src/slack_annotations/core.py Show resolved Hide resolved
src/slack_annotations/core.py Outdated Show resolved Hide resolved
src/slack_annotations/core.py Outdated Show resolved Hide resolved
tests/unit/slack_annotations/core_test.py Outdated Show resolved Hide resolved
tests/unit/slack_annotations/core_test.py Outdated Show resolved Hide resolved
@mtomilov mtomilov merged commit e64a392 into main Dec 10, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unittests to slack-annotations
2 participants