Fix failing/flaky test#35
Conversation
There was a problem hiding this comment.
Pull request overview
Addresses a flaky test caused by leaked per-thread HTTP response state, by switching response storage from Thread.current to ActiveSupport::CurrentAttributes and adjusting the test to force query execution.
Changes:
- Replace
Thread.current-backed HTTP response storage withActiveSupport::CurrentAttributes. - Reset the shared “current” response state in test setup to avoid cross-test leakage.
- Update the flaky test to execute the
Status.allquery (to_a) before asserting on the stored response.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/active_resource_response/connection.rb |
Stores HTTP responses via ActiveSupport::CurrentAttributes instead of Thread.current. |
test/active_resource_response_test.rb |
Resets current response state in setup; forces query execution in flaky test. |
activeresource-response.gemspec |
Adds an explicit activesupport dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class Current < ActiveSupport::CurrentAttributes | ||
| attribute :http_responses, default: {} | ||
|
|
There was a problem hiding this comment.
ActiveSupport::CurrentAttributes is referenced here, but the gem doesn’t explicitly require it anywhere. Depending on what activeresource loads, this can raise NameError: uninitialized constant ActiveSupport::CurrentAttributes when requiring active_resource_response. Consider adding an explicit require 'active_support/current_attributes' (or require 'active_support') before this class is defined so the dependency is reliably loaded.
|
@patrickarnett Thanks for contribution ! Will review soon, please rebase if possible |
|
@Fivell Thanks! |
test_get_headers_from_find_when_404_custom_prefixis currently flaking. It fails fairly consistently, but for inconsistent reasons. Sometimes the actual http response code is 200, sometimes it's 422, sometimes the test raises a NoMethodError.The inconsistency in failure reason is due to the use of
Thread.currentwithout clearing it, causing response state to leak between tests.I'm of the opinion that this is a bug, not just a test flake, because state will also leak between requests and jobs in Rails, so I've implemented ActiveSupport::CurrentAttributes. Rails will clear it automatically between requests/jobs in production, and ActiveSupport is already a transient dependency.
With that out of the way, the test still fails with a NoMethodError. This is resolved by coercing to an array to execute the query.