Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
reset_module_globalsfixture duplicates the same reset logic before and afteryield; consider extracting this into a small helper function or loop to avoid repetition and make future updates less error-prone. - Many of the
_init_secondary_nb_listtests share similar setup and assertion patterns; you could usepytest.mark.parametrizeto consolidate these into fewer parametrized tests and improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `reset_module_globals` fixture duplicates the same reset logic before and after `yield`; consider extracting this into a small helper function or loop to avoid repetition and make future updates less error-prone.
- Many of the `_init_secondary_nb_list` tests share similar setup and assertion patterns; you could use `pytest.mark.parametrize` to consolidate these into fewer parametrized tests and improve readability and maintainability.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
caeb0a5 to
22a94da
Compare
ideaship
requested changes
May 4, 2026
Contributor
ideaship
left a comment
There was a problem hiding this comment.
The empty_url, missing_token, and whitespace_token cases all pass
log_substring=None. But the production code reaches the except
(yaml.YAMLError, TypeError, ValueError) branch for all three and logs "Error
parsing settings NETBOX_SECONDARIES: ...". The tests only verify result == [],
leaving the error-logging path unverified — yet the six other error cases in
the same parametrize do check logs. Inconsistency means a future deletion of
the logger.error(...) call would not be caught by these three cases.
Cover _init_redis, _init_nb, _init_secondary_nb_list, _get_timeout_http_adapter_class, NetBoxSessionManager, cleanup_netbox_sessions, get_netbox_connection, get_openstack_connection and the lazy __getattr__ indirection in osism/utils/__init__.py. Closes #2228 AI-assisted: Claude Code Signed-off-by: Christian Berendt <berendt@osism.tech>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cover _init_redis, _init_nb, _init_secondary_nb_list, _get_timeout_http_adapter_class, NetBoxSessionManager, cleanup_netbox_sessions, get_netbox_connection,
get_openstack_connection and the lazy getattr indirection in osism/utils/init.py.
Closes #2228
AI-assisted: Claude Code