Avoid using RSpec.configuration from the bisect coordinator.
#2507
Conversation
Use a simple `Notifier` instead of the full-fledged `Reporter` (which required the use of config). Avoiding config is necessary for us to be able to use the new forking runner for bisect.
myronmarston
force-pushed
the
myron/bisect-formatter-refactor
branch
from
bac15c4 to
925bc20
Compare
| :bisect_failed, :bisect_aborted, | ||
| :bisect_round_ignoring_ids, :bisect_round_detected_multiple_culprits, | ||
| :bisect_dependency_check_started, :bisect_dependency_check_passed, | ||
| :bisect_dependency_check_failed |
There was a problem hiding this comment.
not sure I follow why all this goes away?
There was a problem hiding this comment.
I think you answered your own question above, but for posterity: previously, this was a "real" formatter that used with the RSpec::Core::Reporter, which required we register the formatter with all the events it subscribes to.
Now that we are no longer using the reporter to publish events to the formatter during bisection runs, we don't have to register all these events. Instead, the new Notifier I wrote just checks respond_to? as a way to see if it can safely publish an event.
In general, we've learned that respond_to? isn't a good approach with 3rd-party formatters (particularly when paired with common event names like start or close that could be meant for some entirely different purpose, or monkey patched onto all objects by a 3rd-party gem, etc...), but when we entirely control the formatter, as we do here, respond_to? is simple and efficient.
| end | ||
|
|
||
| def bisect | ||
| @configuration.add_formatter @formatter |
There was a problem hiding this comment.
so we only used the config to add a formatter. Instead now we just call the formatter directly, right?
So that probably explains why we don't need to register all the events below. This also implies that previously I would have been able to hook in my own formatter to bisect events, where as now I ... can't? Guess it depends where the formatter comes from that's passed into the initializer.
All LGTM, I figure you have this in your head, just trying to see if I'm following along :)
There was a problem hiding this comment.
so we only used the config to add a formatter. Instead now we just call the formatter directly, right?
We used the config for 2 things:
- To add the formatter
- To access the
reporter, which we used to publish events to the formatter
Now we just publish events to the formatter directly, without config and the reporter mediating access.
So that probably explains why we don't need to register all the events below.
Yep!
This also implies that previously I would have been able to hook in my own formatter to bisect events, where as now I ... can't?
In theory, yes, you could have, but we did not provide any way for users to specify a custom bisect formatter, so in practice, a user would have had to monkey patch RSpec to make it use their custom formatter for bisect events. So we aren't breaking any public APIs here.
There was a problem hiding this comment.
thank you for extra explanation!
Avoid using `RSpec.configuration` from the bisect coordinator.
Use a simple
Notifierinstead of the full-fledgedReporter(which required the use of config). Avoiding config is necessary
for us to be able to use the new forking runner for bisect.
This builds on top #2504.