chore: unit tests around protocol usage #48
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
=======================================
Coverage ? 94.63%
=======================================
Files ? 11
Lines ? 205
Branches ? 0
=======================================
Hits ? 194
Misses ? 11
Partials ? 0 ☔ View full report in Codecov by Sentry. |
WilliamBergamin
changed the title
There was a problem hiding this comment.
LGTM and works well in testing! Nice find around the need for the DefaultProtocol too - it makes sense to me that we need this when initializing hooks like with get-hooks but that wasn't so obvious!
I think this is set to merge when you're ready but I left a few comments around defaulting arguments. All of these changes make sense and push the code in a solid direction (thank you for the import sorting) so these could be ignored! 🚀
I did also notice some warnings appear in testing and I'm unsure if these are related to the changes to import sorting or existed beforehand, but wanted to raise regardless:
https://github.com/slackapi/python-slack-hooks/actions/runs/9914063133/job/27392412627#step:5:33
| def test_get_manifest(self, capsys): | ||
| runpy.run_module(get_hooks.__name__, run_name="__main__") | ||
| runpy.run_module(get_hooks.__name__, run_name="__main__", alter_sys=False) |
There was a problem hiding this comment.
Does this change need to be applied to other runpy.run_module methods? It seems like False is the default value but it'd be nice to keep this consistent in whatever case IMO! 🙏
There was a problem hiding this comment.
Good catch 💯 this is a remnant of my experimental work
slack_cli_hooks/hooks/get_hooks.py
Outdated
| @@ -30,5 +26,5 @@ | |||
| } | |||
|
|
|||
| if __name__ == "__main__": | |||
| PROTOCOL = build_protocol() | |||
| PROTOCOL = build_protocol(argv=sys.argv[1:]) | |||
There was a problem hiding this comment.
I'm for this argument being explicit instead of falling back to the same default, but I'm wondering if we'd also want to remove that defaulting value with this change? That might be a breaking change though, so feel free to ignore - this adds a bunch of clarity as is!
There was a problem hiding this comment.
I like this 💯 it isn't a breaking change since the build_protocol util is not exposed publicly
@zimeg these error were present before these changes, it is something I need to address, I think it relates to the scenario test that execute using |
Summary
This PR started as exploratory work to remove the
DefaultProtocolsince it is no longer used by the CLI. Turns out we can't simply remove theDefaultProtocolsince it is used when the user wants to execute the hooks directly. But this lead to the following improvements.A refactor of the
scenario unit testsnow use theMessageBoundaryprotocol, this closer mimics the end user experience through the CLI 👍A
MockProtocolwas introduced in order to facilitate unit testing.Testing
Requirements
./scripts/install_and_run_tests.shafter making the changes.