Add example parameter to extra_failure_content #878

Closed
wants to merge 1 commit into from

4 participants

@gshakhn

Some custom formatters may want to add data based on the example metadata, but there isn't a good way to grab that from just the failure AFAIK.

If there is a better way to do this, please let me know. This was the simplest change I could think of.

Thanks,
George

@gshakhn gshakhn Add example parameter to extra_failure_content.
Some custom formatters may want to add data based on the example metadata,
 but there isn't a good way to grab that from just the failure AFAIK.
76e9b72
@samphippen
RSpec member

Hi @gshakhn, firstly thanks for sending us a pull!

This looks like it could be considered to be a breaking change to me. This is because we're changing the protocol that people subclass on the HTML formatter. This isn't necessarily a problem, but it means we can't get it in until rspec 3.0.0.

I like having more information available to the formatters in general. The other rspec superfriends @alindeman @JonRowe @soulcutter and @myronmarston may be able to suggest a workaround in the meantime and give other feedback.

@gshakhn

@samphippen You're right. I didn't think about the fact that existing formatters would need to update their override.

If you guys think of any workarounds until 3.0.0 comes out, I'd love to use them. I don't follow rspec development too closely, so I'm not sure what the timeline for 3.0.0 is.

More info on our use case in case I'm approaching the problem from the wrong angle:

We currently use formatter that is a hack of a hack (https://github.com/gshakhn/rspec-webdriver). It overrides example_failed, which broke when the base implementation changed in 2.12. I figured I should rewrite it so it wasn't tightly coupled to the base implementation and overrode the right method (extra_failure_content), but need more data passed to that method. Primarily, we save an image and video from a Selenium WebDriver test and name the file based on some example metadata. That metadata isn't available in extra_failure_content currently, hence my pull request. :)

@samphippen
RSpec member

@JonRowe can we close this because of #884 ?

@JonRowe
RSpec member

@samphippen no, this is awaiting me finishing refactoring the formatters then adding this functionality

@JonRowe
RSpec member

Re-examining this post #1258, I'm realising this change never affected a method we intended to expose as public. It's an internal part of this formatter, so I'm on the fence wether to go ahead with this. /cc @myronmarston

@myronmarston
RSpec member

Can the new notification object be used in place of the current argument (or the two arguments, as this PR changes it to)? That will create a nice stable interface (e.g. we can always keep it to that one arg) while making it more flexible.

@JonRowe
RSpec member

@myronmarston it's not part of the reporter cycle, so it's specific to this formatter, that said I have no problem passing the notification down if we choose to make this part of the interface for this formatter.

@gshakhn
@JonRowe
RSpec member

@gshakhn they weren't designed for extending in this way, would it help if we extracted this into the notification for failed_example?

@gshakhn

If the existing formatter isn't not meant to be extended in this way, guess I'll have to do something gross like copy/paste most of it. :(

Given that i would be "rewriting" parts of it, having a notification object with more data would potentially make that easier.

@JonRowe JonRowe closed this in ab5cc8a Mar 15, 2014
@JonRowe
RSpec member

hey @gshakhn I just merged a PR that makes the data you're after accessible via another means, hope that helps!

@gshakhn

Thanks! Is this planned to be in 3.0.0?

@JonRowe
RSpec member

It's in 3.x master

@gshakhn

Great. Looking forward to testing it out when it's released. We'll have a fun time upgrading to 3.x. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment