Reporting load time to formatters #841

Closed
wants to merge 6 commits into
from

Projects

None yet

4 participants

@JonRowe
Member
JonRowe commented Mar 23, 2013

This is the start of an implementation of measuring load time and reporting it via formatters. I wanted to get feedback on roughly how this works before I went much further along with it. Basically the idea is to store the start time as soon as possible in config (which allows it to be overridden by spork or anything else), and then compare that to the start time on start.

At the moment this is a breaking change (because of the extra argument to start). I weighed this up against having an extra method, or adding in if's to work around it, but I think in the longer run it'd be better just to do this even if it means waiting until RSpec 3.

Thoughts? If this is generally agreeable I'll continue and start on the formatters.

This is in reference to #489

@myronmarston myronmarston commented on an outdated diff Mar 23, 2013
lib/rspec/core/formatters/base_formatter.rb
@@ -39,9 +39,10 @@ def initialize(output)
# is {#example_group_started}.
#
# @param example_count
- def start(example_count)
+ def start(example_count,load_time)
@myronmarston
myronmarston Mar 23, 2013 Member

Spaces,please.

(andbelowaswell)

@samphippen
Member

This implementation seems fine, but I don't see any way to get output, is there a way to do this that I'm missing?

@JonRowe
Member
JonRowe commented Apr 6, 2013

Nope, wanted an OK on this before implementing it in the formatters.

@samphippen
Member

@JonRowe I took a look through this. It looks good, can you add the formatters so we can try it out?

@JonRowe
Member
JonRowe commented Apr 21, 2013

I've rebased this and added an example of how load time could be reported to the base text formatter. As it stands this is a breaking change because of the whole protocol change on formatters. But I have an idea, rather than adding in additional paramters, if we changed the reporter to check for implementation before sending notifications:

def notify(method, *args, &block)
  @formatters.each do |formatter|
    formatter.send(method, *args, &block) if formatter.respond_to? method
  end
end

Then we could implement these extra details as new notifications, and thus make the protocol slightly more flexible at the cost of including another conditional... Thought? @samphippen @myronmarston ?

@samphippen
Member

oooooh, I'm not sure if this is necessary. I like this because it means we can add new notifications as and when we want to as it not being a breaking change. My other thought would be to have formatters take a details hash and add keys as necessary. I don't have strong thoughts either way though.

@JonRowe
Member
JonRowe commented Apr 21, 2013

I prefer sending messages over using an arguments hash, because I feel we'd have similar issues with changing the hash keys required/desired, also we'd still have to make a breaking change to switch to the hash.

@soulcutter
Member

New notifications rather than adding parameters to an existing notification... I don't know about this. I feel notifications should be event-based rather than content-based. Still, you can add a new notification without breaking existing formatters if you add that new notification to BaseFormatter as a no-op... the only people that would break for are those not inheriting from BaseFormatter -- are there actually people doing this?

What can a formatter do to be compatible with adding params? def start(example_count, *extra_args) extra_args here would capture any junk you don't care about.

While it would be a breaking change, the more I think about it, the more I think that any notification should receive one argument, which would be an event object. This event object could be a Struct or Hash or some derivative, but that object would give us a place where we could put deprecation warnings when we do want to change things.

But that will be a massive breaking change, you say? I am thinking that if you made it so that any formatter uses new syntax that registers it to receive specific notifications could receive event objects. That could be the migration path, essentially. Then any formatter that does not register itself for events will get a deprecation message, but get the old variable-number-of-params methods invoked.

@JonRowe
Member
JonRowe commented Apr 23, 2013

I would like the option of not having to inherit when writing formatters, I'm in favour of making a breaking chance in RSpec 3 surrounding formatters to make it a more event based system, but I'd still like to get the couple of PR's currently existing out for the next RSpec release (given as RSpec 3 could be a way off) it just seems a more community orientated approach to say 'lets do this non breaking now, and then refactor into a better way for 3'

@soulcutter
Member

I think I can code up a compromise/alternative in the next couple of days.

@samphippen
Member

@JonRowe did the formatter api changes get merged, can this be updated for that now?

@JonRowe
Member
JonRowe commented Jun 11, 2013

Not yet, don't worry, it's on my to do list :P

@JonRowe JonRowe closed this Jul 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment