use_oneshot_lines_coverage causes no implicit conversion of Symbol into Integer (TypeError)
dbackeus opened this issue · 10 comments
Describe the bug
I just added coverband to a Rails app and set use_oneshot_lines_coverage = true for performance reasons. However with this enabled I did not end up getting any data reported and found the following errors logged instead:
ERROR: coverage failed to store
ERROR: Coverband Error: #<TypeError: no implicit conversion of Symbol into Integer> no implicit conversion of Symbol into Integer
I ran bundle open coverband and ensured to re-raise the error to get more data and got the following stacktrace at boot time:
coverband-5.2.3/lib/coverband/collectors/delta.rb:91:in `block in transform_oneshot_lines_results'
coverband-5.2.3/lib/coverband/collectors/delta.rb:79:in `each'
coverband-5.2.3/lib/coverband/collectors/delta.rb:79:in `each_with_object'
coverband-5.2.3/lib/coverband/collectors/delta.rb:79:in `transform_oneshot_lines_results'
coverband-5.2.3/lib/coverband/collectors/delta.rb:32:in `results'
coverband-5.2.3/lib/coverband/collectors/delta.rb:27:in `results'
coverband-5.2.3/lib/coverband/collectors/coverage.rb:61:in `block in report_coverage'
coverband-5.2.3/lib/coverband/collectors/coverage.rb:58:in `synchronize'
coverband-5.2.3/lib/coverband/collectors/coverage.rb:58:in `report_coverage'
coverband-5.2.3/lib/coverband.rb:66:in `report_coverage'
coverband-5.2.3/lib/coverband/utils/railtie.rb:26:in `block in <class:Railtie>'
The error is triggered by coverage[:oneshot_lines] at
coverage is an Array but is treated as a Hash. I didn't find any other reference to the :oneshot_lines symbol in the code-base which was puzzling. This is as far as I spelunked...
I've confirmed that setting use_oneshot_lines_coverage to false allows me to boot just fine since it doesn't hit this code path.
To Reproduce
I'm assuming this error will occur on any Rails app. I haven't done anything special to arrive in this situation.
Expected behavior
Coverband should manage to report when the use_oneshot_lines_coverage configuration is set to true.
Desktop (please complete the following information):
- OS: MacOS (latest version) using Ruby 3.1.0 and Rails 7.0
OK, thanks for the report, I will check it out and reproduce it, I know that the response of coverage has changed from time to time and we have had to detect response and adjust. Especially if branch coverage or anything else is enabled.
Ok, so were have had reports like this from time to time and I feel like I always forget it and we don't give the best errors when it is happening.
This situation occurs when Coverband is set to be in oneshot mode, and is expecting to receive the one-shot format from coverage, but it instead receives the legacy array format. You can see that this format occurs if Coverage.start is called with no options. Coverband will call Coverage.start correctly UNLESS something else has already called Coverage.start see here:
So this mean some other tool or your app code itself has already started Coverage and we are trying to just use whatever was setup... As you can see in the code the most common thing is SimpleCov... If you use that it is the most likely conflict... You can disable it in non-test mode, disable coverband in test... or ensure SimpleCov will use oneshot mode... most of the time folks DO NOT want SimpleCov running in anything other than test mode. Moving it into the test group in your gemfile for example might fix the issue.
I could do something like detect if it is an array and just kind of work with it... but it isn't ideal, now it slows down both modes and doesn't give the performance boost one is hoping for... I feel like perhaps printing a warning and turning off coverband is an OK idea... or I could detect this at bootup and give a better error message and disable coverband as it is in a incorrectly configured state.
transformed_line_counts = if coverage.is_a?(Array)
# NOTE: This means while coverband was set into oneshot mode, something else already started
# ruby coverage in traditional non-oneshot mode, so we are receiving the old standard array coverage format.
coverage
else
coverage[:oneshot_lines].each_with_object(@@stubs[file].dup) { |line_number, line_counts|
line_counts[line_number - 1] = 1
}
end
Take a look at this and let me know if it makes sense and if you have suggestions for what you think would be the best experience.
Thanks for getting back on this 🙏
This is not related to SimpleCov though. I did some more testing and basically the issue is that requiring coverband will immediately start coverage, before the configuration has been done.
I added a puts statement just before
puts "Starting with: #{Coverband.configuration.use_oneshot_lines_coverage}"Then added require: false for coverband to my Gemfile to be able to explicitly control requiring.
Then added the following config/initializers/coverband.rb initializer:
require "coverband"
puts "configuring"
Coverband.configure do |config|
config.use_oneshot_lines_coverage = true
config.track_views = true
endThe output when booting rails becomes:
Starting with: false
configuring
So it seems oneshot mode can only be used in conjunction with COVERBAND_DISABLE_AUTO_START?
Or perhaps a work-around would be to manually run Coverage.start(oneshot_lines: true) before requiring coverband.
Part of the issue here is perhaps that there is no documentation on how to use this feature. The only hint that the feature even exists in the README is in the jRuby section where it is recommended that oneshot mode should be used but with no guidance as to how.
Hmmm interesting, I will look closer, I didn't seem to be able to reproduce the way you are describing but that gives me something to go on...
Good point on documentation, we never really pushed the feature that much. To be honest, it has never shown up as a very significant performance issue for me, in most webapps, but we had a few folks using it outside of webapps where the impact was more significant. I prefer to get the total hit count so you can see a "heat map" of code use frequency, so I don't personally use oneshot.
I guess are you hitting performance issues and reaching for oneshot or just noticed that comment in the JRuby section? I could perhaps add an advanced section on performance tuning, the most common perf impact has been the reporting frequency and the Redis stampede issue of all servers reporting around the same time, those we solve in most production systems with these options
# reduce the CPU and Redis overhead, we don't need reporting every 30s
config.background_reporting_sleep_seconds = 500
# add a wiggle to avoid cache stampede and flatten out Redis CPU
config.reporting_wiggle = 60I will dig in a bit more to your load ordering issue later, but might not get to it until tomorrow.
Hmm looking at this more, I don't see how you are getting the coverage start call before the config. If you setup coverband to have config/coverband.rb it should load that before it ever calls start... I noticed in your last comment you mentioned having a config/initializers/coverband.rb which is NOT recommended, as the gem has a railtie that does initialization, I have documented in the README to use config/coverband.rb so that the gem itself can find and configure during it's railtie code.
If you don't do gem require false, but move your file to config/coverband.rb it should load in the proper order...
with the file in config/coverband.rb
bundle exec rails s
calling coverband configure
******************************************************************************************
coverage initialize
=> Booting Puma
=> Rails 6.1.6 application starting in developmentIf I put the same coverband configuration file in config/initializers/coverband.rb, I get
bundle exec rails s
D, [2022-06-05T14:15:54.837797 #14189] DEBUG -- : using default configuration
******************************************************************************************
coverage initialize
=> Booting Puma
=> Rails 6.1.6 application starting in development
=> Run `bin/rails server --help` for more startup options
calling coverband configureSee in the logs in the second case there are two things to note, the first is using default configuration which is logged when no config/coverband.rb is found (or optionally another file can be set via ENV["COVERBAND_CONFIG"]. See the code on loading the config. Then the logs show that if you have an intializer it ends up calling coverband configure AFTER coverage has been initialized...
I am thinking I could do a couple things...
- I could log a warning if coverband was already configured, which should make it more clear that the default configuration was used as the custom one wasn't found
- I could try to detect the current
Coverageoptions if set and if they don't match the configuration log an error.
What do you think would be the best developer experience.
I will also update the readme to try to make this more clear, and to give an example on how to use oneshot.
perhaps I should also update the log line using default configuration to say Coverband: using default configuration
anyways overall let me know if this fixes the issue for you and what you think is the best clear path forward to help others get stuck like this @dbackeus
Looks like I made a mistake in missing that the config file wasn't supposed to be an initializer! I probably just skimmed through the README too fast saw "config" and "coverband.rb" and assumed it was an initializer as that tends to be the case with most of ruby gem configuration in the context of rails apps. Sorry about that.
A way of improving the developer experience around this that comes to mind could be to add a rake coverband:install task (same convention as action_cable:install / stimulus:install / active_storage:install etc) which generates the configuration file (eg. with all configuration options visible and explained but commented out) and encourage users to install using that command in the README.
thanks the install task is a good idea.
closing this out, but will keep the install task and other simplifications in mind.