Improve test coverage from 80.68% to 85.42% and add comprehensive unit tests#14
Merged
Conversation
Co-authored-by: GrantBirki <23362539+GrantBirki@users.noreply.github.com>
Co-authored-by: GrantBirki <23362539+GrantBirki@users.noreply.github.com>
Co-authored-by: GrantBirki <23362539+GrantBirki@users.noreply.github.com>
Co-authored-by: GrantBirki <23362539+GrantBirki@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] task: improve test coverage
Improve test coverage from 80.68% to 85.42% and add comprehensive unit tests
Jun 11, 2025
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds extensive unit tests to improve coverage from 80.68% to 85.42%, focusing on security validations, core logging, default handler behavior, helper utilities, and HTTP endpoints.
- Introduces new spec files for security constants, default handler, core log management, helper methods, and API endpoints.
- Covers edge cases around payload parsing, request limits, handler loading, and API health/version responses.
- Ensures all tests and lint checks pass with the new 85.42% coverage.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/unit/lib/hooks/security_spec.rb | Tests that DANGEROUS_CLASSES is frozen and complete |
| spec/unit/lib/hooks/handlers/default_spec.rb | Validates logging and response of the default handler |
| spec/unit/lib/hooks/core/log_spec.rb | Verifies setting, retrieving, and overriding logger |
| spec/unit/lib/hooks/app/helpers_spec.rb | Covers UUIDs, request limits, payload parsing, handler validation, error codes, and handler loading |
| spec/unit/lib/hooks/app/endpoints/version_spec.rb | Tests version endpoint JSON response and format |
| spec/unit/lib/hooks/app/endpoints/health_spec.rb | Tests health endpoint status, version, timestamp, and uptime |
Comments suppressed due to low confidence (4)
spec/unit/lib/hooks/app/helpers_spec.rb:3
- The
tempfilelibrary is required but never used in this spec; you can remove this unused require to keep the test file clean.
require "tempfile"
spec/unit/lib/hooks/app/helpers_spec.rb:1
- [nitpick] This spec file is quite large (372 lines) and tests many helper methods; consider splitting it into multiple focused spec files (e.g., one for parsing, one for limits, one for handler loading) to improve maintainability.
# frozen_string_literal: true
spec/unit/lib/hooks/handlers/default_spec.rb:35
- The test refers to
TIME_MOCKbut doesn’t define or stub it; consider definingTIME_MOCKor stubbingTime.nowin abeforeblock so the expected timestamp matches.
timestamp: TIME_MOCK
spec/unit/lib/hooks/app/endpoints/health_spec.rb:14
- The test uses
Time.parsebut doesn’t includerequire 'time'; add that at the top to ensureTime.parseis available in the spec.
allow(Hooks::App::API).to receive(:start_time).and_return(Time.parse("2024-12-31T23:59:00Z"))
| @@ -0,0 +1,36 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| describe Hooks::Log do | |||
There was a problem hiding this comment.
[nitpick] Add an after block (e.g., after { described_class.instance = nil }) to reset Hooks::Log.instance between examples and avoid state leakage across tests.
Suggested change
| describe Hooks::Log do | |
| describe Hooks::Log do | |
| after { described_class.instance = nil } |
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.
This PR significantly improves the test coverage and addresses the test sprawl issues mentioned in the original issue. The changes focus on adding comprehensive unit tests for previously untested components while following Ruby best practices.
📊 Coverage Improvements
🆕 New Test Files Added
Security and Core Components
spec/unit/lib/hooks/security_spec.rb(9 tests)DANGEROUS_CLASSESconstant with security edge casesspec/unit/lib/hooks/core/log_spec.rb(4 tests)Handler Testing
spec/unit/lib/hooks/handlers/default_spec.rb(10 tests)API Endpoint Testing
spec/unit/lib/hooks/app/endpoints/version_spec.rb(5 tests)spec/unit/lib/hooks/app/endpoints/health_spec.rb(7 tests)Application Helpers
spec/unit/lib/hooks/app/helpers_spec.rb(42 tests)🔧 Test Quality Improvements
letblocks, and context organization🛡️ Security Testing Highlights
The new tests include extensive security validation:
📁 File Organization
While the largest existing test files remain (to minimize changes), the new test files demonstrate the preferred approach:
The test suite now provides a solid foundation with excellent coverage that exceeds the 80% requirement with a healthy margin, ensuring robust validation of the webhook server framework.
Fixes #7.