feat: Add test infrastructure with multi-version Ruby support#22
feat: Add test infrastructure with multi-version Ruby support#22facundofarias merged 4 commits intomasterfrom
Conversation
- Add minitest test suite covering Configuration, Resource, and Request classes - Add Rakefile with default task for running linting and tests - Update CI workflow to test across Ruby 2.7-3.4 in matrix - Update .rubocop.yml to exclude test files from strict metrics - Add CLAUDE.md with project architecture and development commands 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThe pull request establishes comprehensive testing infrastructure for the project. It introduces a CI workflow update with a Ruby version matrix (2.7 to 3.4), adds RuboCop and RSpec test suites with configuration, updates dependencies in Gemfile, creates rake tasks for test execution, and includes documentation for Claude Code integration. The gemspec metadata is updated with new maintainer information. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
- Update authors to DeployHQ Team - Update email to support@deployhq.com - Fix homepage URL to correct GitHub organization (deployhq) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
CLAUDE.md (1)
61-61: Consider wrapping the bare URL in angle brackets.The URL at line 61 should be wrapped in angle brackets per CommonMark specification to avoid markdown linting warnings.
Apply this diff:
-- `account`: DeployHQ account URL (e.g., https://account.deployhq.com) +- `account`: DeployHQ account URL (e.g., <https://account.deployhq.com>)Rakefile (1)
10-10: Consider enabling Ruby warnings in tests.Disabling warnings with
t.warning = falsecan hide important signals like deprecations, unused variables, and method redefinitions. These warnings often catch real issues before they become problems.Consider removing this line to enable warnings:
t.libs << 'test' t.libs << 'lib' t.test_files = FileList['test/**/*_test.rb'] - t.warning = false endIf specific warnings are too noisy, you can selectively suppress them using Ruby's
Warning[]API instead of disabling all warnings.test/resource_test.rb (2)
7-13: Consider adding teardown to reset configuration.While all tests in this file use the same configuration, adding a teardown method to reset
Deploy.configurewould improve test isolation and prevent potential issues if other test files run in the same process.Add after the setup method:
def teardown Deploy.instance_variable_set(:@configuration, nil) end
97-119: Add more comprehensive assertions for pagination test.The test verifies that paginated responses work but only checks the count and first resource's ID. Consider adding assertions to verify:
- The second resource's attributes
- That pagination metadata doesn't interfere with resource extraction
Add these assertions before the end of the test:
assert_equal 'Resource 1', resources[0].name assert_equal 2, resources[1].id assert_equal 'Resource 2', resources[1].name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/ci.yml(2 hunks).rubocop.yml(2 hunks)CLAUDE.md(1 hunks)Gemfile(1 hunks)Rakefile(1 hunks)lib/deploy/cli.rb(1 hunks)test/configuration_test.rb(1 hunks)test/request_test.rb(1 hunks)test/resource_test.rb(1 hunks)test/test_helper.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
test/request_test.rb (1)
lib/deploy/request.rb (3)
make(30-58)success?(20-22)output(24-26)
lib/deploy/cli.rb (3)
lib/deploy/resources/project.rb (1)
servers(43-45)lib/deploy/resources/deployment.rb (1)
servers(27-38)lib/deploy/resources/server_group.rb (1)
servers(22-24)
test/configuration_test.rb (1)
lib/deploy/configuration.rb (2)
websocket_hostname(12-14)from_file(16-27)
test/resource_test.rb (1)
lib/deploy/resource.rb (4)
new_record?(115-117)class_name(80-82)find(30-35)destroy(111-113)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
61-61: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (14)
lib/deploy/cli.rb (2)
103-116: LGTM! Improved idiom for collection checks.The change from
count.positive?toany?is more idiomatic and efficient in Ruby. Theany?method short-circuits on the first truthy element, whilecountiterates through the entire collection.
200-206: LGTM! RuboCop compliance achieved.The removal of lint-disable comments indicates the code now passes RuboCop checks without requiring exceptions, which improves code quality consistency.
.rubocop.yml (1)
1-90: LGTM! Well-configured RuboCop rules for test infrastructure.The configuration appropriately excludes test files from strict metrics while maintaining code quality standards. The
SuggestExtensions: falseprevents unnecessary suggestions, and the specific exclusion forlib/deploy/resource.rbfromNaming/PredicateMethodappears intentional for valid predicate methods in that file.test/test_helper.rb (1)
1-10: LGTM! Proper test isolation setup.The test helper correctly configures the load path, integrates WebMock with Minitest, and disables external HTTP connections to ensure tests are isolated and deterministic.
.github/workflows/ci.yml (2)
5-18: LGTM! Excellent multi-version testing strategy.The Ruby version matrix (2.7 through 3.4) ensures compatibility across a wide range of Ruby versions. The unified
bundle exec rakestep efficiently combines linting and testing in a single command.
32-32: LGTM! Proper release gating.The release job correctly depends on test completion, ensuring that releases only occur after successful testing across all Ruby versions.
test/request_test.rb (1)
1-132: LGTM! Comprehensive test coverage for Deploy::Request.This test suite provides excellent coverage:
- All HTTP methods (GET, POST, PUT, DELETE) are tested
- Success paths verify both status and response body
- Error conditions properly exercise exception handling (404, 403, 401, 503)
- Client error fallback (400) correctly tests the
falsereturn path- WebMock stubs are properly structured with auth and headers
The test structure is clear and well-organized, making it easy to understand the expected behavior of the
Deploy::Requestclass.test/configuration_test.rb (3)
1-11: LGTM! Clean test structure and setup.The frozen string literal, imports, and default websocket hostname test are well-structured and correctly verify the default behavior.
13-30: LGTM! Comprehensive attribute testing.The tests properly verify both custom websocket hostname override and standard configuration attribute setters/getters.
74-78: LGTM! Proper error handling verification.The test correctly verifies that attempting to load from a nonexistent file raises the expected exception.
test/resource_test.rb (4)
15-41: LGTM! Comprehensive attribute and state testing.The tests thoroughly verify dynamic attribute access via
method_missingand thenew_record?state helper logic.
43-53: LGTM! Path helper tests are clear and correct.The tests properly verify the class name and path generation helpers.
55-95: LGTM! Find operations are well-tested.The tests properly stub HTTP interactions and verify both single resource retrieval and collection retrieval with comprehensive assertions.
121-133: LGTM! Destroy operation is properly tested.The test correctly stubs the DELETE request and verifies the destroy method returns true on success.
Add explicit f.flush call after f.write and before f.rewind in all Tempfile tests to ensure data is flushed from user-space buffer to disk before File.read reads the file. This prevents potential intermittent test failures due to buffered data not being visible to subsequent reads. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/configuration_test.rb (1)
32-92: Consider testing partial JSON scenarios.The current tests cover complete JSON, missing
websocket_hostname, missing file, and invalid JSON. You might optionally add a test for valid JSON that's missing other fields (e.g.,account,username) to document whether this is supported or should raise an error.Example test structure:
def test_from_file_with_partial_fields config_data = { 'account' => 'https://test.deployhq.com' } Tempfile.create(['config', '.json']) do |f| f.write(JSON.generate(config_data)) f.flush f.rewind config = Deploy::Configuration.from_file(f.path) assert_equal 'https://test.deployhq.com', config.account assert_nil config.username assert_nil config.api_key # ... verify expected behavior for missing fields end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/configuration_test.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/configuration_test.rb (1)
lib/deploy/configuration.rb (2)
websocket_hostname(12-14)from_file(16-27)
🔇 Additional comments (4)
test/configuration_test.rb (4)
1-94: Excellent test coverage for Deploy::Configuration.The test suite is well-structured and covers the key functionality: defaults, attribute setting, file loading with various scenarios, and error handling. The use of Tempfile with blocks ensures proper cleanup, and test naming is clear and consistent.
41-44: Past buffering issue resolved.The
f.flushcall on line 43 correctly ensures the buffer is written to disk beforeFile.read(path)reads the file, addressing the previous review concern.
64-67: Past buffering issue resolved.The
f.flushcall on line 66 correctly ensures data is written to disk before reading, addressing the previous review concern.
82-92: Past buffering issue resolved; error handling test looks good.The
f.flushcall on line 85 addresses the previous buffering concern, and the test correctly verifies that invalid JSON raisesJSON::ParserError.
thdurante
left a comment
thdurante
left a comment
There was a problem hiding this comment.
Looks good but why not RSpec? 👼
Replace Minitest with RSpec to normalize testing infrastructure across projects. All 27 tests converted and passing with identical coverage. Changes: - Replace test/ directory with spec/ - Update Gemfile and Rakefile for RSpec - Add .rspec configuration - Update CLAUDE.md with RSpec commands 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changed! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CLAUDE.md (1)
64-64: Consider wrapping the URL in angle brackets for markdown compliance.The bare URL triggers a markdown linting warning. Wrapping it in angle brackets improves compliance with markdown best practices.
Apply this diff:
-- `account`: DeployHQ account URL (e.g., https://account.deployhq.com) +- `account`: DeployHQ account URL (e.g., <https://account.deployhq.com>)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.gitignore(1 hunks).rspec(1 hunks)CLAUDE.md(1 hunks)Gemfile(1 hunks)Rakefile(1 hunks)spec/configuration_spec.rb(1 hunks)spec/request_spec.rb(1 hunks)spec/resource_spec.rb(1 hunks)spec/spec_helper.rb(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- .rspec
🚧 Files skipped from review as they are similar to previous changes (2)
- Rakefile
- Gemfile
🧰 Additional context used
🧬 Code graph analysis (3)
spec/request_spec.rb (1)
lib/deploy/request.rb (3)
make(30-58)success?(20-22)output(24-26)
spec/resource_spec.rb (1)
lib/deploy/resource.rb (4)
new_record?(115-117)class_name(80-82)find(30-35)destroy(111-113)
spec/configuration_spec.rb (1)
lib/deploy/configuration.rb (2)
websocket_hostname(12-14)from_file(16-27)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
64-64: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (4)
spec/configuration_spec.rb (1)
1-106: Excellent test coverage for Deploy::Configuration!The test suite is comprehensive and well-structured, covering:
- Default and custom websocket_hostname values
- All attribute accessors
- File loading with complete and partial data
- Error cases (missing file, invalid JSON)
The use of Tempfile for isolated file I/O testing is appropriate and follows best practices.
spec/spec_helper.rb (1)
1-30: Well-configured test environment!The RSpec and WebMock setup follows best practices:
- Load path configuration correctly includes the lib directory
- WebMock disabled external connections for test isolation
- RSpec configured with monkey patching disabled and random order for robust test execution
- Example persistence enables focused test runs
spec/resource_spec.rb (1)
1-161: Comprehensive test coverage for Deploy::Resource!The test suite thoroughly validates:
- Dynamic attribute access through method_missing (both read and write)
- Record state tracking (new_record?)
- Class-level path helpers and naming conventions
- Resource retrieval (single, collection, paginated)
- Resource deletion
The WebMock stubs correctly simulate HTTP interactions with proper authentication and content-type headers.
spec/request_spec.rb (1)
1-142: Excellent test coverage for Deploy::Request!The test suite comprehensively validates:
- All HTTP methods (GET, POST, PUT, DELETE) with proper authentication
- Success cases with correct data handling
- Complete error mapping for various HTTP status codes to Deploy::Errors exceptions
- Client errors returning false with error output
The WebMock stubs accurately simulate HTTP interactions and the assertions validate both success flags and response data.
thdurante
left a comment
thdurante
left a comment
There was a problem hiding this comment.
Sweet! Thank you for switching it, it is what we're using in the other projects so I think it makes sense
2 participants
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores