-
Notifications
You must be signed in to change notification settings - Fork 0
Add comprehensive test coverage for failure scenarios in plugin system #29
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
Conversation
Co-authored-by: GrantBirki <[email protected]>
…r, builder, helpers, and lifecycle Co-authored-by: GrantBirki <[email protected]>
lib/hooks/core/plugin_loader.rb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR bolsters test coverage for various failure paths in the plugin system and updates the plugin loader to handle syntax errors during file loading.
- Added
SyntaxError
to rescue clauses inPluginLoader
- Introduced extensive specs for auth, handler, lifecycle, and instrument plugin loading failures
- Added tests for builder configuration plugin failures and JSON size limit enforcement
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
spec/unit/lib/hooks/plugins/lifecycle_spec.rb | Added require_relative for spec_helper |
spec/unit/lib/hooks/core/plugin_loader_spec.rb | New failure‐scenario specs for all plugin types |
spec/unit/lib/hooks/core/builder_spec.rb | Test for plugin‐loading failure raising ConfigurationError |
spec/unit/lib/hooks/app/helpers_spec.rb | Test for safe_json_parse JSON payload size limit |
lib/hooks/core/plugin_loader.rb | Rescue SyntaxError alongside StandardError in loader methods |
Comments suppressed due to low confidence (1)
spec/unit/lib/hooks/plugins/lifecycle_spec.rb:3
- The relative path to
spec_helper.rb
appears to be off by one level and may loadspec/unit/spec_helper.rb
instead of the top‐levelspec/spec_helper.rb
. Consider usingrequire_relative "../../../../spec_helper"
orrequire 'spec_helper'
if your load path is configured.
require_relative "../../../spec_helper"
begin | ||
load_custom_auth_plugin(file_path, auth_plugin_dir) | ||
rescue => e | ||
rescue StandardError, SyntaxError => e | ||
raise StandardError, "Failed to load auth plugin from #{file_path}: #{e.message}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The same rescue/raise pattern is duplicated across multiple plugin-loading methods. Consider extracting this into a helper (e.g. handle_load_error(file_path, &block)
) to reduce duplication and make future changes easier.
Copilot uses AI. Check for mistakes.
describe "#safe_json_parse" do | ||
it "raises ArgumentError when JSON payload exceeds size limit" do | ||
# Test the actual size limit by temporarily setting a small limit | ||
stub_const("ENV", ENV.to_h.merge("JSON_MAX_SIZE" => "10")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Stubbing the entire ENV
constant can have unintended side effects. Consider using allow(ENV).to receive(:[]).with("JSON_MAX_SIZE").and_return("10")
or a dedicated environment‐variable helper (e.g. the climate_control
gem) to isolate this change.
stub_const("ENV", ENV.to_h.merge("JSON_MAX_SIZE" => "10")) | |
allow(ENV).to receive(:[]).with("JSON_MAX_SIZE").and_return("10") |
Copilot uses AI. Check for mistakes.
This PR significantly improves test coverage for failure cases across the core plugin system components, addressing missing coverage for
raise
statements and error handling paths.Changes Made
Enhanced Plugin Loader Error Handling
SyntaxError
handling: Updated plugin loading methods to properly catchSyntaxError
exceptions in addition toStandardError
, ensuring malformed plugin files are handled gracefullyBuilder Configuration Error Coverage
Helpers JSON Security Limits
Lifecycle Plugin Component Access
Test Coverage Improvement
Security Benefits
The new tests specifically cover important security scenarios:
Example of New Test Coverage
All changes follow existing code patterns and maintain consistency with the project's testing approach.
Fixes #28.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.