Allow collecting coverage in parallel using DRb #400
Conversation
|
Nice! Thanks for the PR, Richard. I will take a look in the next days and will see if I can test it. Thanks for your interest in this project and the work on the issue and this PR. |
Of course! Thank you so much for creating and maintaining it 🙏 |
| # is wrapped in a CoveredResponse data object. | ||
| tracker&.track_response( | ||
| oad.key, | ||
| CoveredResponse.new( |
| plan = Plan.for(oad, skip_response:, skip_route:) | ||
| [oad.key, plan] | ||
| end | ||
| tracker = Tracker.new(Test.definitions, skip_response:, skip_route:) |
There was a problem hiding this comment.
I am looking for code to distinguish the main process from "remote" process… but that is not needed here, because Test.start only run in the main process?
There was a problem hiding this comment.
Yes I assume here that Coverage.start (or Test.setup) is called in the main process. The issue is I'm not sure how to differentiate between main and worker process without storing the main process PID somewhere (which requires that some code is expected to run in the main process anyway).
Do you consider the Coverage module to be public API? In the Test module we at least safe-guard against multiple calls to setup which also prevents multiple calls to Coverage.start.
There was a problem hiding this comment.
Do you consider the Coverage module to be public API?
Good question. I think I started out with Test::Coverage being public API, but added Test.setup after that. So I think Coverage should be considered private. …which could make some class methods on Coverage obsolete.
There was a problem hiding this comment.
If you want to we can add something like Test.in_main_process? and guard Coverage.start with this. However, this would still require that Test.setup is called in the main process, which would then set the PID. What do you think? Alternatively we could also return early from Coverage.start if @drb_uri is non-nil?
…which could make some class methods on Coverage obsolete.
I thought about this as well while exchanging the current_run Hash with the Tracker class. For example, the current_run method is currently only used in specs. Same for the plans or the reset method.
There was a problem hiding this comment.
If you want to we can add something like Test.in_main_process? and guard […] What do you think? Alternatively we could also return early from Coverage.start if @drb_uri is non-nil?
Hey @richardboehme, I think both approaches are fine. I don't have a strong opinion on this things right now. Maybe the second variant is simpler and respects the lines between Test and Coverage? Do you want to add that guard in this PR?
I thought about this as well while exchanging the current_run Hash with the Tracker class.
I will have removed Coverage.current_run, .plans, .install, .uninstall in another PR. #401
There was a problem hiding this comment.
Okay I added a small guard on top of the Coverage.start method!
|
I think is this great and would like to merge it. Just 👋🏼 whenever you think this is ready to go. 👍🏼 |
richardboehme
force-pushed
the
drb-parallel
branch
from
a7ab7b3 to
09b9f37
Compare
Thank you, I think this is ready to merge now! |
richardboehme
force-pushed
the
drb-parallel
branch
from
09b9f37 to
9642847
Compare
| end | ||
|
|
||
| def start(skip_response: nil, skip_route: nil) | ||
| return if @drb_uri |
2 participants
This is a first shot at allowing parallel coverage collection. The advantage of the DRb-based approach is that we do not need to detect if coverage is collected in parallel or not, it should just work theoretically. This also means that this should run with all tools that run tests in parallel on the same machine, but I only tested it with the framework that should not be named here.
There are probably more sophisticated ways of doing this so please let me know if we should adjust anything! I'm looking forward to any feedback on this one!
Closes #394