Skip to content

Commit

Permalink
A race condiiton could occur when setting the redis for the view tracker
Browse files Browse the repository at this point in the history
This commit moves the creation of the view tracker and subscription to active support notifications until after we have run the users custom config block... previously the view tracker would run before a users custom config could set the store `config.store` with a redis URL... So it would default to REDIS_URL from the env, even if the user set it to say COVERBAND_REDIS_URL in their custom config... This bug could only effect folks that had multiple redises and were trying to run Coverband on a Redis that wasn't associated with the REDIS_URL env.

When this bug occered we would save view_tracker information tot he REDIS_URL redis instance and then report them from the on configured by the users custom config.
  • Loading branch information
danmayer committed Sep 7, 2020
1 parent db6969b commit 08726b5
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 10 deletions.
23 changes: 14 additions & 9 deletions lib/coverband/utils/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,21 @@ class Railtie < Rails::Railtie
initializer "coverband.configure" do |app|
begin
app.middleware.use Coverband::BackgroundMiddleware
rescue Redis::CannotConnectError => error
Coverband.configuration.logger.info "Redis is not available (#{error}), Coverband not configured"
Coverband.configuration.logger.info "If this is a setup task like assets:precompile feel free to ignore"
end
end

config.after_initialize do
unless Coverband.tasks_to_ignore?
Coverband.configure
Coverband.eager_loading_coverage!
Coverband.report_coverage
Coverband.runtime_coverage!
end

begin
if Coverband.configuration.track_views
COVERBAND_VIEW_TRACKER = if Coverband.coverband_service?
Coverband::Collectors::ViewTrackerService.new
Expand All @@ -33,15 +47,6 @@ class Railtie < Rails::Railtie
end
end

config.after_initialize do
unless Coverband.tasks_to_ignore?
Coverband.configure
Coverband.eager_loading_coverage!
Coverband.report_coverage
Coverband.runtime_coverage!
end
end

config.before_configuration do
unless ENV["COVERBAND_DISABLE_AUTO_START"]
begin
Expand Down
2 changes: 1 addition & 1 deletion lib/coverband/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@
# use format '4.2.1.rc.1' ~> 4.2.1.rc to prerelease versions like v4.2.1.rc.2 and v4.2.1.rc.3
###
module Coverband
VERSION = "5.0.0"
VERSION = "5.0.1.rc.1"
end

2 comments on commit 08726b5

@kbaum
Copy link
Collaborator

@kbaum kbaum commented on 08726b5 Sep 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! Would be great to have a way to expose this type of initialization issue in our tests. Something that initializes coverband in a bunch of different possible ways and ensures coverage is getting captured. For that we would need the test to be forking a new process each time.

@danmayer
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I couldn't really figure out a good way to test this... I am thinking perhaps we could ensure nothing tries to save or send any coverage without first having called configure... That is probably something we could test even without forking and having to kick everything off and ensure coverage was collected... This was definitely a subtle and hard thing to figure out so trying to find a better way to test for it would be good

Please sign in to comment.