fix: failing tests due to change in incident name #2550
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
dosubot
bot
added
the
size:M
WalkthroughThe changes in this pull request involve modifications across multiple files, including Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
keep/functions/__init__.py (1)
Line range hint
279-281: Enhance error messages for incident-related validationGiven that this PR aims to fix failing tests due to incident name changes, consider improving the error messages in
get_firing_timeandis_first_timeto be more specific about incident name validation requirements. For example:- raise ValueError("fingerprint is required") + raise ValueError("Invalid or missing incident fingerprint. Please ensure the incident name follows the required format.")This would make it easier to diagnose and fix test failures related to incident names.
Also applies to: 323-324
tests/test_rules_engine.py (1)
Line range hint
573-587: Consider tracking TODOs in issue trackerThe TODO comments provide valuable guidance for future test coverage. Consider creating GitHub issues to track these test cases properly.
Would you like me to help create GitHub issues for tracking these test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
keep/functions/__init__.py(2 hunks)tests/test_rules_engine.py(9 hunks)
🔇 Additional comments (5)
keep/functions/__init__.py (1)
Line range hint 1-350: Verify test coverage for JSON parsing changes
While the changes might fix the immediate test failures, please ensure:
- Test cases cover both successful and error scenarios with the new
json5parsing - Integration tests verify the interaction between functions using different JSON parsing methods
- Edge cases with malformed incident names are properly tested
This will help prevent future issues from the mixed use of JSON parsing methods.
tests/test_rules_engine.py (4)
9-19: LGTM! Import organization improves readability
The grouping of related imports enhances code organization and maintainability.
272-277: Verify incident name changes across the codebase
The assertion now expects the incident name to match the rule name directly, removing any prefix. This change aligns with the PR objective of fixing failing tests due to incident name changes.
The last_seen_time assertion has been reformatted for better readability.
Line range hint 364-374: LGTM! More descriptive alert messages
The alert messages are now more descriptive, making the test cases clearer and easier to understand.
Also applies to: 398-402, 426-430
455-459: LGTM! Improved test parameterization formatting
The test parameterization is now more readable with parameters spread across multiple lines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
keep/api/routes/preset.py (2)
140-143: Consider catching specific exceptions instead of bare ExceptionWhile the change to
logger.exception()is good for including stack traces, catching a bareExceptionmight mask specific errors. Consider catching and handling specific exceptions that could occur during incident pulling (e.g.,ConnectionError,TimeoutError, etc.).- except Exception: + except (ConnectionError, TimeoutError) as e: logger.exception( f"Unknown error pulling incidents from provider {provider.type} ({provider.id})", extra={**extra, "trace_id": trace_id}, ) + except Exception as e: + logger.exception( + f"Unexpected error pulling incidents from provider {provider.type} ({provider.id})", + extra={**extra, "trace_id": trace_id, "error_type": type(e).__name__}, + )
168-171: Enhance error handling and maintain logging consistencySimilar to the incident pulling section, consider:
- Catching specific exceptions instead of bare
Exception- Adding trace_id to the extra context for consistency with other error logs
- except Exception: + except (ConnectionError, TimeoutError) as e: logger.exception( f"Unknown error pulling topology from provider {provider.type} ({provider.id})", - extra={**extra}, + extra={**extra, "trace_id": trace_id}, ) + except Exception as e: + logger.exception( + f"Unexpected error pulling topology from provider {provider.type} ({provider.id})", + extra={**extra, "trace_id": trace_id, "error_type": type(e).__name__}, + )keep/providers/pagerduty_provider/pagerduty_provider.py (1)
Line range hint
578-603: LGTM! Consider adding error handling for missing fields.The implementation is well-structured and handles both new and old format events correctly. The code is clean and follows good practices.
Consider adding defensive programming for potential missing fields:
- status = PagerdutyProvider.ALERT_STATUS_MAP.get(event.get("status", "firing")) + status = PagerdutyProvider.ALERT_STATUS_MAP.get(event.get("status", "firing"), AlertStatus.FIRING)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
keep/api/routes/preset.py(2 hunks)keep/providers/pagerduty_provider/pagerduty_provider.py(2 hunks)
🔇 Additional comments (1)
keep/providers/pagerduty_provider/pagerduty_provider.py (1)
795-803: LGTM! Clean implementation of incident and alert formatting.
The changes properly integrate the new static formatting methods while maintaining the relationship between incidents and their alerts.
Summary by CodeRabbit
New Features
Bug Fixes
Tests