Refactor: Reorganize tests and move ActiveJob to Rails-compliant paths #902
Conversation
|
Warning Rate limit exceeded@mensfeld has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR reorganizes ActiveJob integration (new Shoryuken ActiveJob adapter and JobWrapper), adds bulk enqueuing (enqueue_all) with SQS batching, persists Rails CurrentAttributes across enqueue/execute boundaries, introduces a large isolated integration test suite and runner tooling, tightens Ruby/Rails support, and updates CI and test configs. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant AJ as ActiveJob (enqueue_all)
participant Adapter as ShoryukenAdapter
participant SQS as AWS SQS (send_message_batch)
App->>AJ: perform_all_later / enqueue_all(jobs)
AJ->>Adapter: enqueue_all(jobs)
Note over Adapter: Group jobs by queue\nBatch into groups of 10
loop each batch
Adapter->>Adapter: build message bodies & attributes
Adapter->>SQS: send_message_batch(entries: batch)
SQS-->>Adapter: response (success/errors)
Adapter->>Adapter: mark jobs enqueued
end
Adapter-->>App: return enqueue results
sequenceDiagram
participant Job as ActiveJob (producer)
participant Serializer as CurrentAttributes::Serializer
participant Adapter as ShoryukenAdapter
participant Queue as SQS Queue
participant JobWrapper as Shoryuken::ActiveJob::JobWrapper
participant Loader as CurrentAttributes::Loading
participant Worker as Job perform method
Job->>Serializer: serialize registered CurrentAttributes -> hash
Serializer-->>Adapter: wrapped into message body
Adapter->>Queue: send_message (message includes attrs)
Note over Queue: message stored with persisted attrs
Queue->>JobWrapper: receive message
JobWrapper->>Loader: deserialize/restore attributes
Loader->>Worker: worker.perform runs with restored CurrentAttributes
Worker-->>Loader: perform done
Loader->>Loader: reset CurrentAttributes after execution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This is a major reorganization that improves code structure and follows Rails conventions. Changes are atomic and must ship together. Test Infrastructure Changes: - Move unit tests from spec/shoryuken/ to spec/lib/shoryuken/ (mirrors lib/) - Move Rails version gemfiles from gemfiles/ to spec/gemfiles/ - Remove Appraisal gem in favor of custom multi-version testing - Update RSpec, Rake, and Renovate configurations for new structure ActiveJob Adapter Changes: - Move adapters to Rails-compliant paths: lib/active_job/queue_adapters/ - Extract JobWrapper to lib/shoryuken/active_job/job_wrapper.rb - Update all requires and constant references - Change JobWrapper constant: ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper → Shoryuken::ActiveJob::JobWrapper - Add Rails 8.1 compatibility (stopping? method for Continuations) Configuration Updates: - Enhanced SimpleCov configuration with groups and minimum coverage - Updated zeitwerk ignore pattern for new ActiveJob location - Add YARD documentation to configuration methods This reorganization provides: - Clearer code structure matching Rails conventions - Better test organization mirroring lib/ structure - Improved ActiveJob adapter discoverability - Foundation for future enhancements Breaking Changes: - JobWrapper constant path changed (if users reference it directly) - Test file locations changed (CI/tooling may need updates) - Gemfile organization changed (Appraisal removed) Test Results: 512 examples, 0 failures, 90.43% coverage
Bundle install failures (e.g., Rails versions not available in CI) now result in skipped tests rather than failures.
Clear bundler-related environment variables before running bundle install to avoid inheriting CI cache settings that restrict gem installation to locally cached gems only.
Set BUNDLE_PATH to local vendor/bundle directory for each integration test to avoid CI bundler cache interference.
- Create local .bundle/config per test directory - Set BUNDLE_APP_CONFIG to override project-level config - Exclude vendor/ and .bundle/ from spec discovery
mensfeld
force-pushed
the
refactor/reorganize-tests-and-activejob
branch
from
7efa27d to
a0ec431
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the test infrastructure and ActiveJob integration to follow Rails conventions. The changes reorganize tests to mirror the lib/ structure, move ActiveJob adapters to Rails-compliant paths (lib/active_job/queue_adapters/), and update the JobWrapper constant location from ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper to Shoryuken::ActiveJob::JobWrapper. Additionally, it adds Rails 8.1 compatibility with the stopping? method for Continuations support, enhances SimpleCov configuration, and removes the Appraisal gem in favor of custom multi-version testing.
Key Changes:
- Test reorganization: unit tests moved from
spec/shoryuken/tospec/lib/shoryuken/to mirror lib/ structure - ActiveJob adapter path changes: moved to
lib/active_job/queue_adapters/following Rails conventions - JobWrapper constant refactored from nested adapter constant to
Shoryuken::ActiveJob::JobWrapper
Reviewed changes
Copilot reviewed 61 out of 96 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/spec_helper.rb | Enhanced SimpleCov configuration with groups, filters, branch coverage, and minimum coverage thresholds; warning suppression added |
| spec/shoryuken/extensions/*.rb | Deleted ActiveJob extension specs (moved to new locations) |
| spec/shared_examples_for_active_job.rb | Updated JobWrapper constant references from described_class::JobWrapper to Shoryuken::ActiveJob::JobWrapper |
| spec/lib/shoryuken/**/*.rb | New test file locations mirroring lib/ structure with comprehensive unit tests |
| spec/lib/active_job/**/*.rb | New ActiveJob-related test files for extensions and adapters |
| spec/integrations_helper.rb | New integration test helper with LocalStack support and test utilities |
| spec/integration/**/*.rb | Comprehensive integration tests for Rails versions, worker lifecycle, retry behavior, etc. |
| shoryuken.gemspec | Ruby version requirement bumped from >= 3.1.0 to >= 3.2.0 |
| .rspec | Added exclude pattern for integration tests |
| gemfiles/.gitignore | Removed (gemfiles directory relocated) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
spec/integration/fifo_and_attributes/fifo_and_attributes_spec.rb
Outdated
Show resolved
Hide resolved
- Add enqueue_all method to ShoryukenAdapter for efficient bulk job enqueuing using SQS send_message_batch API (batches of 10) - Remove run_test_suite wrapper from Rails spec files (Karafka-style) - Add 3 new LocalStack integration tests: - activejob_roundtrip: full round-trip job execution - bulk_enqueue: perform_all_later with 15 jobs - activejob_scheduled: delayed jobs with set(wait:)
Move common requires to integrations_helper.rb: - shoryuken - active_job - active_job/queue_adapters/shoryuken_adapter - active_job/extensions - securerandom Integration specs no longer need #!/usr/bin/env ruby shebang since they are run via `bundle exec ruby -r ./spec/integrations_helper.rb`.
Move ActiveJob requires out of integrations_helper.rb - not all specs need it. Only shoryuken and securerandom are universally needed. ActiveJob specs now require their own dependencies: - active_job - active_job/queue_adapters/shoryuken_adapter
Inspired by Karafka's DataCollector pattern: - Thread-safe singleton for collecting test data - DT[:key] << value to append, DT[:key].size to check - DT.queue provides unique queue names for tests - DT.clear resets all collected data - Replaces global variables in integration specs
Update remaining 10 specs to use DataCollector (DT) instead of class-level attr_accessor patterns: - batch_processing: DT[:messages], DT[:batch_sizes] - concurrent_processing: DT[:processing_times] - fifo_ordering: DT[:messages] - large_payloads: DT[:bodies] - launcher: local atomic counter (DT.queue for queue name) - message_attributes: DT[:attributes] - polling_strategies: DT[:by_queue] - retry_behavior: DT[:receive_counts] - visibility_timeout: DT[:messages], DT[:visibility_extended] - worker_lifecycle: DT[:messages] Specs that don't need DT (use different patterns): - adapter_configuration: no data collection (pure assertions) - error_handling: uses JobCapture for mocked tests - fifo_and_attributes: uses local captured_params variables
Enables Rails ActiveSupport::CurrentAttributes to flow from enqueue
to job execution, based on Sidekiq's approach:
- Serializes current attributes into job payload when enqueuing
- Restores attributes before job execution, resets them afterward
- Supports multiple CurrentAttributes classes
- Uses ActiveJob::Arguments for GlobalID/Symbol support
Usage:
require 'shoryuken/active_job/current_attributes'
Shoryuken::ActiveJob::CurrentAttributes.persist('MyApp::Current')
Also removes redundant DT.clear calls from integration specs since
each spec runs in its own process with fresh state.
Add comprehensive test coverage for CurrentAttributes persistence: 1. Multiple CurrentAttributes classes (TestCurrent + RequestContext) 2. Empty context (job enqueued without any attributes set) 3. Partial context (only some attributes set) 4. Complex data types (hashes with symbols, arrays) 5. Bulk enqueue via perform_all_later preserves context Based on test patterns from Sidekiq and Karafka's CurrentAttributes implementations to ensure feature parity.
- Remove redundant reset_shoryuken calls (each process starts fresh) - Merge Shoryuken config options into setup_localstack - setup_localstack now does all necessary configuration for LocalStack tests - Specs not using LocalStack (mocked tests) don't call setup_localstack
These integration tests complement the existing mocked tests: - activejob_retry: Tests retry_on/discard_on with real LocalStack - activejob_custom_attributes: Tests custom SQS message attributes round-trip
- Add setup_active_job helper method to consolidate ActiveJob requires and adapter setup - Replace manual requires in 14 ActiveJob specs with setup_active_job call - Add bin/clean_localstack script to remove all it-* test queues from LocalStack - Remove delete_test_queue calls from specs (queues cleaned via script instead) This follows Karafka's pattern where test resources aren't cleaned up after each test. In CI, LocalStack dies after the run anyway. For local dev, use bin/clean_localstack to remove test queues.
- Move all ActiveJob specs into spec/integration/active_job/ directory - Split multi-scenario specs into individual files: - current_attributes: 5 separate specs (full_context, empty_context, partial_context, complex_types, bulk_enqueue) - retry: 2 specs (retry_on, discard_on) - custom_attributes: 2 specs (string_attributes, number_attributes) - middleware_chain: 4 specs (execution_order, short_circuit, removal, empty_chain) - Remove obvious comments from all integration specs - Rename files to avoid redundant prefixes (e.g., activejob_roundtrip → roundtrip) Total: 29 granular integration specs (up from 20 combined specs)
Resolved conflict in specs.yml by keeping the rails_specs job from main and adding it to the ci-success needs array.
The rails_specs job from main depends on gemfiles that don't exist in this branch. Removing it to fix CI.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/shoryuken/util.rb (1)
32-45: The adapter must be updated to use the new JobWrapper class from Shoryuken::ActiveJob.The hardcoded string
'Shoryuken::ActiveJob::JobWrapper'at line 38 will never match the message attributes becauselib/shoryuken/extensions/active_job_adapter.rbstill defines and uses a localJobWrapperclass. The MESSAGE_ATTRIBUTES constant writesJobWrapper.to_swhich evaluates to'ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper', not the newShoryuken::ActiveJob::JobWrapperclass that was created.This breaks worker name detection for all ActiveJob messages. Either:
- Replace the local JobWrapper in active_job_adapter.rb with a require and reference to
Shoryuken::ActiveJob::JobWrapper, or- Update util.rb line 38 to check for
'ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper'if keeping the old JobWrapperAlso address backward compatibility: existing messages with the old string value won't be recognized by the new check.
spec/spec_helper.rb (1)
9-16: Unconditionalnextdisables warning processing and leaves dead code.The
nexton line 10 short-circuits the entireWarning.processblock, making lines 11-15 unreachable dead code. If warning suppression is intentional, consider removing the dead code. If this is temporary for debugging, consider adding a comment or TODO.Warning.process do |warning| - next - next unless warning.include?(Dir.pwd) - next if warning.include?('useless use of a variable in void context') && warning.include?('core_ext') - next if warning.include?('vendor/') - - raise "Warning in your code: #{warning}" + # Warnings are currently suppressed; uncomment the following to re-enable strict checking: + # next unless warning.include?(Dir.pwd) + # next if warning.include?('useless use of a variable in void context') && warning.include?('core_ext') + # next if warning.include?('vendor/') + # raise "Warning in your code: #{warning}" end
🧹 Nitpick comments (45)
spec/lib/shoryuken/version_spec.rb (1)
10-12: Consider making the regex more precise for semantic versioning validation.The regex
/^\d+\.\d+\.\d+/only validates the X.Y.Z prefix but doesn't enforce full semantic versioning format. For a test claiming to validate semver format, you might want:- expect(Shoryuken::VERSION).to match(/^\d+\.\d+\.\d+/) + expect(Shoryuken::VERSION).to match(/^\d+\.\d+\.\d+(-[0-9A-Za-z\-\.]+)?(\+[0-9A-Za-z\-\.]+)?$/)This would validate:
- Core version (X.Y.Z)
- Optional pre-release identifier (e.g.,
-rc1)- Optional build metadata (e.g.,
+20130313144700)Note: The current version
7.0.0.rc1uses a dot instead of the standard semver hyphen (7.0.0-rc1)..gitignore (1)
30-32: Integration env artifacts are correctly ignored (with some redundancy)Ignoring
spec/integration/**/vendor/is helpful for per-scenario bundle installs. TheGemfile.lockand.bundlepatterns underspec/integration/**are technically redundant with the existing top-levelGemfile.lockand.bundleignores, but they’re harmless and may improve clarity.shoryuken.gemspec (1)
29-29: Clarify alignment betweenrequired_ruby_versionand RuboCop’s TargetRubyVersionYou’ve set
required_ruby_versionto'>= 3.2.0', while RuboCop is configured withTargetRubyVersion: 3.4, which RuboCop flags as a mismatch. If you intend to support 3.2/3.3 in production, keeping>= 3.2.0is correct, and it might be better to lowerTargetRubyVersion(or disable that cop). If actual support starts at 3.4, consider bumpingrequired_ruby_versionto'>= 3.4.0'instead.spec/integration/retry_behavior/retry_behavior_spec.rb (1)
1-45: Retry behavior spec correctly exercises ApproximateReceiveCount, with minor polish possibleThe setup (short
VisibilityTimeout,auto_delete: false, manualdelete, and a failing worker with an atomic counter) looks solid and should reliably generate multiple deliveries with increasingApproximateReceiveCount. Two small, optional cleanups if you want to keep RuboCop fully green:
- Rename the unused
bodyargument inperformto_bodyto satisfyLint/UnusedBlockArgument.- Use single-quoted strings for the static messages (
'Simulated failure',"Receive counts should be increasing"→ single quotes) to appeaseStyle/StringLiterals.None of these are behavior-changing.
spec/lib/shoryuken/middleware/entry_spec.rb (1)
1-68: Nice coverage ofShoryuken::Middleware::Entry; consider avoiding direct@argsinspectionThe specs do a good job of covering initialization, argument storage, instance creation, and identity. One minor improvement you might consider:
- In the “stores initialization arguments” example, you’re asserting via
instance_variable_get(:@args). To reduce coupling to internal state, you could instead assert through behavior (e.g., callmake_newand check the constructed instance’sargs), which will keep the spec stable if the internal representation of arguments ever changes.- Very minor style nit:
['arg1', 'arg2']could be written as%w[arg1 arg2]if you want to follow RuboCop’sStyle/WordArraysuggestion.Purely optional; the current tests are already functionally robust.
spec/lib/shoryuken/worker_registry_spec.rb (1)
48-61: Clarify or tighten the “designed to be subclassed” exampleThe NotImplementedError specs and
respond_to?checks are useful. The second example here (be < Objectandancestors.include?(described_class)) is effectively always true for a normal Ruby class, so it doesn’t add much signal. Consider either dropping it or replacing it with something more specific to the intended extension pattern (e.g., asserting that a minimal subclass can override the abstract methods without raising).spec/integration/launcher/launcher_spec.rb (1)
3-5: Align the launcher spec comment with what’s actually testedThe top comment mentions single-message consumption, batch consumption, and command workers, but this spec currently only exercises processing multiple messages via a batch worker and asserts that more than one message is processed. To avoid confusion, consider updating the comment (or expanding the spec) so the description matches the behavior under test.
For example:
-# This spec tests the Launcher's ability to consume messages from SQS queues, -# including single message consumption, batch consumption, and command workers. +# This spec tests the Launcher's ability to consume messages from SQS queues +# using a batch worker against a LocalStack-backed queue.spec/integration/message_attributes/message_attributes_spec.rb (2)
17-21: Rename unusedbodyargument to avoid lint noise
bodyisn’t used insideperform, which matches the RuboCop hint. To keep the signature compatible but silence the unused-argument warning, you can rename it:- def perform(sqs_msg, body) - DT[:attributes] << sqs_msg.message_attributes - end + def perform(sqs_msg, _body) + DT[:attributes] << sqs_msg.message_attributes + endThis keeps the behavior the same while making the intent explicit.
3-5: Ensure the file-level comment reflects what’s assertedThe comment mentions system attributes and custom type suffixes, but the expectations only cover the three custom attributes (
StringAttr,NumberAttr,BinaryAttr). Either adding assertions for system attributes/type suffixes or narrowing the comment to describe the current checks would make the spec self-documenting.spec/lib/shoryuken/middleware/server/auto_extend_visibility_spec.rb (1)
68-116: Logging behavior around auto-extend visibility is well coveredThe added contexts exercise the key branches: batch worker warning, error logging when
change_visibilityfails, and debug logging on successful extension. The expectations match the middleware implementation and should guard against regressions in log wording and control flow.If you want to tighten the batch-worker contract further, you could also assert that
change_visibilityis never called in that path, but the current coverage is already quite good.bin/clean_localstack (1)
13-52: LocalStack queue cleanup logic is sound; consider optional configurabilityThe script correctly discovers
it--prefixed queues and deletes them using a small worker pool withSizedQueue, exiting cleanly once the queue is drained. This is a solid utility for keeping long-lived LocalStack instances tidy.If you ever want to reuse it beyond the default localhost setup, you could optionally wire the endpoint/region through environment variables or share the SQS client construction logic from the existing CLI (
bin/cli/sqs.rb) to avoid duplication, but that’s not required for this use case.spec/lib/shoryuken/message_spec.rb (1)
71-96: Tests correctly verify parameter merging behavior.Both test cases appropriately use
hash_includingto verify that user-provided options are merged withqueue_urlandreceipt_handle. The static analysis hint about indentation is a minor style preference.If you want to align with RuboCop's
Layout/FirstArgumentIndentationrule, indent the first argument one step more:expect(client).to receive(:change_message_visibility).with(hash_including( - visibility_timeout: 300, - queue_url: 'https://sqs.us-east-1.amazonaws.com/123456789/test-queue', - receipt_handle: 'handle-456' + visibility_timeout: 300, + queue_url: 'https://sqs.us-east-1.amazonaws.com/123456789/test-queue', + receipt_handle: 'handle-456' ))However, this may reduce readability; consider disabling this rule locally if preferred.
spec/integration/concurrent_processing/concurrent_processing_spec.rb (1)
18-34: Prefix unused arguments with underscore.The
sqs_msgandbodyparameters are required by the worker interface but unused in this test. Prefix them with underscores to indicate intentional non-use and silence the RuboCop warning.- define_method(:perform) do |sqs_msg, body| + define_method(:perform) do |_sqs_msg, _body|bin/scenario (3)
9-13: Unused exit code constant.
EXIT_TIMEOUTis defined but never used in this script. The timeout handling appears to be delegated tobin/integrations.Consider removing the unused constant or adding timeout handling to this script if individual scenario timeouts are needed:
EXIT_SUCCESS = 0 EXIT_FAILURE = 1 -EXIT_TIMEOUT = 2 EXIT_SETUP_ERROR = 3
41-46: Duplicatebundler/setuprequire.
bundler/setupis already required at line 7. This duplicate require is harmless but unnecessary.def setup_scenario # Each test handles its own specific requirements - require 'bundler/setup' - puts "Setting up isolated test environment" if ENV['VERBOSE'] end
128-135: Side effects from Rails availability check.
rails_available?actually loads Rails when checking availability, which may have unintended side effects. Consider checking gem availability instead.def rails_available? - begin - require 'rails' - true - rescue LoadError - false - end + # Check if rails gem is available without loading it + Gem::Specification.find_by_name('rails') + true + rescue Gem::MissingSpecError + false endspec/integrations_helper.rb (1)
93-109: Optional: Use single-quoted strings for simple messages.The assertion methods use double-quoted strings where single quotes would suffice. This is a minor style preference.
If you'd like to align with RuboCop conventions:
- def assert(condition, message = "Assertion failed") + def assert(condition, message = 'Assertion failed') raise TestFailure, message unless condition end # ... - def refute(condition, message = "Refutation failed") + def refute(condition, message = 'Refutation failed') assert(!condition, message) endspec/integration/large_payloads/large_payloads_spec.rb (1)
15-17: Prefix unusedsqs_msgargument to keep RuboCop quietThe worker signature is correct for Shoryuken, but
sqs_msgis unused and flagged by RuboCop. If you run RuboCop on integration specs, you can silence the warning without changing behavior by prefixing the arg with an underscore:- def perform(sqs_msg, body) + def perform(_sqs_msg, body)This keeps the method arity explicit while avoiding
Lint/UnusedMethodArgumentnoise.spec/integration/worker_lifecycle/worker_lifecycle_spec.rb (1)
15-17: Align unusedsqs_msgargument with RuboCop expectationsThe worker
performsignature is correct for Shoryuken, butsqs_msgis unused and flagged by RuboCop. You can keep the signature while silencingLint/UnusedMethodArgumentlike this:- def perform(sqs_msg, body) + def perform(_sqs_msg, body)Everything else in this lifecycle scenario looks solid.
spec/integration/middleware_chain/short_circuit_spec.rb (1)
5-21: Tidy up unused middleware arguments to avoid RuboCop noiseThe short‑circuit behavior is correctly modeled, but all the
callparameters except the block are unused, and RuboCop flags them. If you run RuboCop on integration specs, you can either prefix them with underscores or collapse them into splats to keep the interface obvious while silencingLint/UnusedBlockArgument, e.g.:def create_middleware(name) Class.new do - define_method(:call) do |worker, queue, sqs_msg, body, &block| + define_method(:call) do |_worker, _queue, _sqs_msg, _body, &block| DT[:calls] << :"#{name}_before" block.call DT[:calls] << :"#{name}_after" end end end def create_short_circuit_middleware Class.new do - define_method(:call) do |worker, queue, sqs_msg, body, &block| + define_method(:call) do |_worker, _queue, _sqs_msg, _body, &_block| DT[:calls] << :short_circuit end end endThis preserves the documented middleware signature while keeping static analysis happy.
spec/integration/rails/rails_81/activejob_adapter_spec.rb (1)
26-29:SerializationJob#performwill raise if ever executed with this payload
complex_datacontains nested hashes and arrays:complex_data = { 'user' => { 'name' => 'John', 'age' => 30 }, 'preferences' => ['email', 'sms'] }but
performcalls:complex_data.transform_values(&:upcase)This will fail because
transform_values(&:upcase)attempts to callupcaseon the nested Hash under'user'and the Array under'preferences', neither of which respond to that method. The test works becauseperform_lateronly enqueues the job without executing it—the spec only validates what gets serialized in the message, not actual job execution. However, if this job is ever dequeued and processed, it will raise.If this is meant to be a test-only fixture, make
performa no-op:def perform(complex_data) - complex_data.transform_values(&:upcase) + # no-op: this spec only validates serialization endOr implement proper deep traversal if the job should handle nested structures.
spec/spec_helper.rb (1)
33-54: SimpleCov configuration looks good; minor indentation fix needed.The coverage configuration with filters, groups, branch coverage, and minimum thresholds is well-structured. However, RuboCop flagged an indentation issue on line 36 - the block body should be indented 2 spaces.
unless ENV['SIMPLECOV_DISABLED'] require 'simplecov' SimpleCov.start do - add_filter '/spec/' - add_filter '/test_workers/' - add_filter '/examples/' - add_filter '/vendor/' - add_filter '/.bundle/' - - add_group 'Library', 'lib/' - add_group 'ActiveJob', 'lib/active_job' - add_group 'Middleware', 'lib/shoryuken/middleware' - add_group 'Polling', 'lib/shoryuken/polling' - add_group 'Workers', 'lib/shoryuken/worker' - add_group 'Helpers', 'lib/shoryuken/helpers' - - enable_coverage :branch - - minimum_coverage 89 - minimum_coverage_by_file 60 + add_filter '/spec/' + add_filter '/test_workers/' + add_filter '/examples/' + add_filter '/vendor/' + add_filter '/.bundle/' + + add_group 'Library', 'lib/' + add_group 'ActiveJob', 'lib/active_job' + add_group 'Middleware', 'lib/shoryuken/middleware' + add_group 'Polling', 'lib/shoryuken/polling' + add_group 'Workers', 'lib/shoryuken/worker' + add_group 'Helpers', 'lib/shoryuken/helpers' + + enable_coverage :branch + + minimum_coverage 89 + minimum_coverage_by_file 60 end endspec/integration/middleware_chain/execution_order_spec.rb (1)
5-13: Prefix unused block arguments with underscore.The middleware signature requires these parameters for interface compliance, but they're unused in this test. Per Ruby convention, prefix with
_to indicate intentional non-use.def create_middleware(name) Class.new do - define_method(:call) do |worker, queue, sqs_msg, body, &block| + define_method(:call) do |_worker, _queue, _sqs_msg, _body, &block| DT[:order] << :"#{name}_before" block.call DT[:order] << :"#{name}_after" end end endspec/lib/active_job/queue_adapters/shoryuken_adapter_spec.rb (1)
28-29: Remove extra empty line before block end.Minor formatting fix flagged by RuboCop.
end end - endspec/integration/fifo_ordering/fifo_ordering_spec.rb (1)
16-18: Unusedsqs_msgargument in test workerThe FIFO ordering check looks solid, but
sqs_msgis unused inperform. Consider renaming it to_sqs_msg(or similar) to satisfy linting and signal intentional non-use.spec/integration/polling_strategies/polling_strategies_spec.rb (1)
14-18: Spec only validates multi-queue reachability, not WeightedRoundRobin biasThis verifies that all three queues are polled and receive messages, but it doesn’t exercise the relative weighting (3/2/1). If the goal is to cover WeightedRoundRobin behavior, consider extending the spec (e.g., enqueue more messages and assert that the high-weight queue is observed more often than the low-weight one).
Also applies to: 31-38, 44-46
spec/integration/batch_processing/batch_processing_spec.rb (1)
17-21: Batch-size assertion could be mildly brittle across environmentsThe test correctly checks that batch mode processes all 5 messages and that at least one batch has
size > 1. Depending on how the underlying SQS/LocalStack and poller behave, there’s a theoretical risk of only single-message deliveries even in batch mode, which would make this assertion flaky. If that becomes an issue, you could weaken the condition (e.g., assert thatDT[:batch_sizes].max <= 10and that all 5 messages were processed) while still covering batch semantics.Also applies to: 29-37
spec/integration/middleware_chain/removal_spec.rb (1)
5-13: Tidy up unused block arguments in middleware stubThe chain-removal behavior is well covered. To quiet lint and make intent clear, consider prefixing the unused
worker,queue,sqs_msg, andbodyargs with underscores increate_middleware(or collapsing them to*_), since only the yielded block and DT instrumentation are used.Also applies to: 29-31
spec/integration/active_job/fifo_and_attributes/deduplication_spec.rb (1)
26-33: Good coverage of FIFO dedup and attributes; minor stub/style cleanups possibleThis spec does a good job of:
- Verifying that FIFO messages get a 64-char SHA256
message_deduplication_idbased onmessage_bodywithoutjob_idandenqueued_at.- Ensuring custom
message_attributesare preserved whileshoryuken_classis correctly set to"Shoryuken::ActiveJob::JobWrapper".Two small polish ideas:
- In the
register_workerandqueuesstubs, consider naming unused parameters with a leading underscore (e.g.,|*_args|/|_queue_name = nil|) to avoid unused-arg warnings.- You could switch the wrapper name literal to single quotes to match common style in the repo.
None of these affect behavior, just test hygiene.
Also applies to: 35-41, 43-44, 55-66, 68-76, 81-86
spec/shared_examples_for_active_job.rb (1)
2-2: Shared examples correctly updated forShoryuken::ActiveJob::JobWrapperThe switch to
require 'active_job/extensions'and the expectations referencingShoryuken::ActiveJob::JobWrapper(both inmessage_attributes['shoryuken_class']andShoryuken.register_workercalls) line up with the new JobWrapper location and adapter behavior. This keeps the adapter specs tightly coupled to the public wrapper constant instead of an internal nested class.If you care about RuboCop’s line-length warnings, the hash expectations containing
Shoryuken::ActiveJob::JobWrapper.to_scould be split over more lines, but that’s purely cosmetic.Also applies to: 25-31, 53-54, 134-141, 160-162, 191-193, 277-277
spec/integration/rails/rails_81/continuation_spec.rb (1)
8-12: Solid coverage ofstopping?and continuation delays; consider minimal cleanupThis script gives good end-to-end coverage for:
ShoryukenAdapter#stopping?with and without an initialized, stopping launcher.- How
enqueue_atturns past, current, and future timestamps intodelay_seconds, with sensible bounds.Two minor nits you might consider:
- Capture and restore the original
Shoryuken::Runner.instancelauncher around the test to future-proof against added scenarios in the same process.- Switch the assertion messages to single-quoted strings if you want to satisfy RuboCop’s Style/StringLiterals here.
Behavior-wise, this looks correct.
Also applies to: 14-28, 32-79
spec/integration/rails/rails_80/continuation_spec.rb (1)
19-27: Consider extracting launcher state setup to a test helper.The test directly manipulates internal instance variables (
@launcher,@stopping) to test thestopping?behavior. While this works for integration testing, consider whether the Launcher class should expose methods for test setup to avoid coupling tests to implementation details.spec/lib/shoryuken/middleware/server/active_record_spec.rb (1)
34-49: Remove unused mock methods.The
rails_71_or_higher!andrails_70!methods defined on the version mock are never called in the tests. The tests override the version behavior directly usingallow().to receive().and_return()instead.Apply this diff to simplify the mock:
- def active_record_module.version - @version ||= Object.new.tap do |v| - def v.>=(other) - # For our tests, we'll control this with instance variables - @is_rails_71_or_higher ||= false - end - - def v.rails_71_or_higher! - @is_rails_71_or_higher = true - end - - def v.rails_70! - @is_rails_71_or_higher = false - end - end - end + def active_record_module.version + @version ||= Object.new + endspec/integration/active_job/error_handling/job_wrapper_spec.rb (1)
1-53: Test name suggests broader scope than actual coverage.The spec tests job enqueuing and JobWrapper configuration but doesn't exercise actual error handling behavior (retry or discard). The test validates that jobs with retry_on and discard_on configurations are properly enqueued, but doesn't trigger failures to verify the error handling mechanisms work correctly.
Consider either:
- Renaming to clarify this tests enqueueing with error configurations
- Adding tests that trigger actual failures to validate retry/discard behavior
spec/integration/active_job/bulk_enqueue/bulk_enqueue_spec.rb (1)
47-50: Consider parallel assertion for clearer failure messages.The current loop validates each execution's data individually, which is good for pinpointing failures. However, if the loop completes without finding a match for an index, the failure might not be immediately obvious.
This is a minor optional improvement - the current implementation works fine:
DT[:executions].each do |execution| expected_data = "payload_#{execution[:index]}" - assert_equal(expected_data, execution[:data], "Job #{execution[:index]} should have correct data") + assert_equal(expected_data, execution[:data], "Job index #{execution[:index]} data mismatch") endspec/integration/active_job/retry/discard_on_spec.rb (1)
34-43: Consider adding assertion for success job attempt count.The test validates successes for the non-failing job but doesn't explicitly assert it was only attempted once.
success_successes = DT[:successes].select { |s| s[:job_id] == success_job.job_id } assert_equal(1, success_successes.size, "Non-failing job should succeed") + +success_attempts = DT[:attempts].select { |a| a[:job_id] == success_job.job_id } +assert_equal(1, success_attempts.size, "Non-failing job should only attempt once")spec/lib/active_job/extensions_spec.rb (1)
66-71: The ruby2_keywords compatibility test may not be meaningful.This test checks if
job_class.method(:new).respond_to?(:ruby2_keywords), butruby2_keywordsis a decorator applied at module definition time, not a method on the resultingMethodobject. The test will likely always pass vacuously without actually verifying the intended behavior.Consider removing or rewriting this test case to actually verify keyword argument forwarding works correctly:
it 'properly forwards keyword arguments through initialize' do # Test that kwargs are forwarded correctly job = job_class.new('arg1', key: 'value') expect(job.sqs_send_message_parameters).to eq({}) endspec/lib/shoryuken/logging_spec.rb (1)
26-36: Consider cleaning up thread-local storage after context tests.The tests modify
Thread.current[:shoryuken_context]but don't clean up after themselves in anafterblock. While this may not cause issues currently, it could lead to test pollution if other specs run in the same thread.describe '#context' do + after do + Thread.current[:shoryuken_context] = nil + end + it 'returns empty string when no context is set' do Thread.current[:shoryuken_context] = nil expect(formatter.context).to eq('') endRakefile (1)
12-15: Integration task delegates to external script.The new integration task properly exits with non-zero status on failure. Consider using single quotes per RuboCop convention.
task :integration do - puts "Running integration tests..." + puts 'Running integration tests...' system('./bin/integrations') || exit(1) endspec/integration/active_job/current_attributes/complex_types_spec.rb (1)
53-55: Consider verifying array element values.The test validates that
tenant_idis an Array of size 2, but doesn't verify the actual values. Consider adding assertions for the element values to ensure complete serialization/deserialization fidelity.tenant_data = result[:tenant_id] assert(tenant_data.is_a?(Array)) assert_equal(2, tenant_data.size) +# Values may be strings after JSON serialization +assert(tenant_data.map(&:to_s).include?('tenant_a')) +assert(tenant_data.map(&:to_s).include?('tenant_b'))lib/shoryuken/active_job/current_attributes.rb (1)
49-55: Key naming is inconsistent depending on call pattern.The key generation logic produces different keys depending on how
persistis called:
persist(A)thenpersist(B)→{cattr: A, cattr_0: B}persist(A, B)→{cattr: A, cattr_1: B}This doesn't break functionality but is inconsistent. Consider also adding idempotency to skip already-registered classes.
def persist(*klasses) @cattrs ||= {} - klasses.flatten.each_with_index do |klass, idx| - key = @cattrs.empty? ? 'cattr' : "cattr_#{idx}" + klasses.flatten.each do |klass| + klass_name = klass.to_s + next if @cattrs.value?(klass_name) # Skip if already registered + + key = "cattr_#{@cattrs.size}" @cattrs[key] = klass.to_s endspec/integration/active_job/current_attributes/partial_context_spec.rb (1)
53-57: Consider usingassert_nilfor nil assertions.The assertions are correct, but
assert_nilis more idiomatic thanassert(...nil?):-assert(result[:tenant_id].nil?) -assert(result[:request_id].nil?) +assert_nil(result[:tenant_id]) +assert_nil(result[:request_id]) assert_equal('fr-FR', result[:locale]) -assert(result[:timezone].nil?) +assert_nil(result[:timezone])lib/active_job/queue_adapters/shoryuken_adapter.rb (1)
101-106: Consider handling negative delays for past timestamps.If
timestampis in the past,delaywill be negative. While SQS likely handles this gracefully (treating it as 0), an explicit check would be clearer.def calculate_delay(timestamp) delay = (timestamp - Time.current.to_f).round raise 'The maximum allowed delay is 15 minutes' if delay > 15.minutes - delay + [delay, 0].max endspec/integration/active_job/scheduled/scheduled_spec.rb (2)
40-67: Timing assertions are sensible; consider whether you want stricter upper bounds or more defensive helpersThe current timing checks (immediate
< 10s, delayed jobs>= 2s/>= 4s) are a reasonable compromise between catching obvious regressions and avoiding flakes from LocalStack/CI variance. Likewise,enqueue_timewill raise quickly if a timestamp label is missing, which is often desirable in tests.If you later see intermittent slowness or want more diagnostics, you could:
- Add soft upper bounds for delayed jobs as well (e.g.
<= 15s) to detect large regressions, and/or- Make
enqueue_timefail with a clearer message when a label is missing (e.g.tapon the result andraisewith the label).Given this is an internal integration spec, leaving it as-is is also fine.
44-53: String literal style: optional RuboCop clean‑upRuboCop is flagging a few of these assertion messages for
Style/StringLiterals(preferring single quotes when not interpolating):assert_equal(3, DT[:executions].size, "Expected 3 job executions") assert(immediate_job, "Immediate job should have executed") assert(delayed_3s_job, "3s delayed job should have executed") assert(delayed_5s_job, "5s delayed job should have executed") # ... "Immediate job should execute before 3s delayed job" "3s delayed job should execute before 5s delayed job"If you want this file fully RuboCop‑clean and consistent with the rest of the suite, you can switch these to single quotes (leaving the interpolated messages as double‑quoted).
Also applies to: 71-75
- Specs job runs unit tests without LocalStack - Integrations job runs integration tests with LocalStack - Both run in parallel for faster CI feedback
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/specs.yml (1)
6-23: Add explicit permissions block to specs job.The job lacks an explicit
permissionsblock for GITHUB_TOKEN, which is a security best practice per GitHub's advanced security scanning. This was previously flagged by CodeQL analysis.Apply this diff to add minimal read-only permissions:
specs: name: Specs + permissions: + contents: read strategy: matrix:Alternatively, if you prefer to set permissions at the workflow level (covering all jobs), add this after the
on:block at the top of the file:on: - push - pull_request +permissions: + contents: read jobs:
🧹 Nitpick comments (2)
.github/workflows/specs.yml (2)
34-44: Clarify LocalStack health check timeout behavior.The health check uses a 30-second timeout before proceeding. If the timeout is exceeded, the workflow will fail. This is appropriate for CI, but the error message could be clearer.
Consider improving the error clarity and robustness:
- name: Wait for LocalStack run: | timeout 30s bash -c ' until curl -s http://localhost:4566/_localstack/health | grep -q "\"sqs\": \"available\""; do echo "Waiting for LocalStack..." sleep 2 done ' + shell: bash + if: success()Alternatively, if failures should not stop the workflow, add
continue-on-error: true. However, the current behavior (fail fast) is generally preferred for CI health checks.
24-52: Add explicit permissions block to integrations job.The job should include an explicit
permissionsblock for the GITHUB_TOKEN to follow the principle of least privilege. While the default token has sufficient permissions for this workflow, explicitly declaring minimal read-only permissions improves security posture.Apply this diff:
integrations: name: Integrations + permissions: + contents: read strategy: matrix:The
spec:integrationRake task is properly defined in the Rakefile and requires no changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/specs.yml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/specs.yml
[warning] 7-24: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
[warning] 25-54: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
- Fix project root path in bin/scenario (was ../.. should be ..) - Handle partial failures in enqueue_all by checking SQS response - Remove orphaned lib/shoryuken/extensions/active_job_adapter.rb - Add permissions block to workflow for security best practices
This is a major reorganization that improves code structure and follows Rails conventions. Changes are atomic and must ship together.
Test Infrastructure Changes:
ActiveJob Adapter Changes:
Configuration Updates:
This reorganization provides:
Breaking Changes:
Test Results: 512 examples, 0 failures, 90.43% coverage
Summary by CodeRabbit
New Features
Breaking Changes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.