Fix noisy log error

[sj26]

We've been seeing this error in our test logs:

Could not log "render_template.action_view" event.
NoMethodError: undefined method `example_group' for nil:NilClass
.../rspec-rails-3.6.0/lib/rspec/rails/view_rendering.rb:67:in `current_example_group'
.../rspec-rails-3.6.0/lib/rspec/rails/view_rendering.rb:71:in `render_template'
...

This is while running an rspec rails feature spec using capybara, so this is a separate thread running webrick. This was also during a before block. In any case, this should probably fail gracefully when there is no example.

While updating this code I got an "ExampleGroup doesn't respond to render_views?", so built in that case too.

[xaviershay]

Thank you for the patch!

This is while running an rspec rails feature spec using capybara

I'm a little confused as to what expected behaviour is here. With your change, it will log the "views not rendered" message, but seems like that would be just as confusing in your use case?

[mikegee]

This is related to #1800, but doesn't link to it. Now it does.

[sj26]

@xaviershay Sorry, I think this might be tricky with some double negatives! With this change in my case it no longer logs a stack trace, and does not log the "views not rendered" message because it returns early because the example is nil so #prevent_view_rendering? is false.

This is correct in my case. I'm running a feature spec which does indeed render views, hooked up to capybara and poltergeist.

I inverted the name and logic of that method so that it describes whether the noteworthy behavior change is in effect, allowing us to shortcut an answer in the cases where an example or group etc is missing, or is not configured for view suppression at all. That may have made it more confusing!

I also didn't want to rock the boat and change this too much, or try mocking especially internals in a way which might interfere with running the test suite itself, either. But we could try this if it wouldn't be a problem and would illustrate and exercise the cases better?

[xaviershay]

oh yes you are right, I mis-read it :)

LGTM!

[JonRowe]

As per #1827 I think this fix masks the problem (in the case of no render_views? for sure, as the log subscriber should never be attached to such a spec)

[sj26]

@JonRowe yeah, it did seem like the global state was a bit fragile, but I didn't want to rock the boat too much. I did take a look, though. Afaict there's no ActiveSupport::Subscriber.detach_from or equivalent. There is ActiveSupport::LogSubscriber::TestHelpers but that seems more about the testing of log subscribers instead of limited or conditional application of them, and I don't think we'd want to interfere with consumers of rspec-rails' potential testing of log subscribers. I'm also not sure about the breadth of support for these pieces across the various supported rails versions.

So in lieu of that rabbit hole I opted for fixing the immediate error and leaving the caveats as-is. Perhaps there should be a follow up issue/PR?

[sj26]

So I did look into this a little more. As an alternative we could just throw away the subscriber and instead log within the empty template renderer:

sj26/rspec-rails@fix-log-subscriber-outside-current-example...sj26:or-just-log-it-here

I have no idea how to test this in the current rspec-rails test suite. The current template renderer has no unit tests in the spec suite, it all seems integration tested with Cucumber. It would be easiest with a verifying proxy or something in these integration tests, but I don't want to litter the nice clean relish docs with logger mocks. Or if there were a step for asserting log output? Not sure.

[JonRowe]

So I did look into this a little more. As an alternative we could just throw away the subscriber and instead log within the empty template renderer:

I'd be ok with that solution, we don't test this log currently as far as I can tell so I'm unsure what to do about that.

[sj26]

I've pulled that commit onto this branch. I've verified manually that this behaves as expected by running an example from the cucumber suite and then removing the "render_views" and ran it again and made sure the logs worked as expected:

There are no tests for the log output at the moment and again I'm not sure how to add them. Maybe this is enough?

[sj26]

This does seem to log more often than master currently does because it now logs for every individual template render (the interior template and the layout) instead of for every controller render call, but I think that's acceptable:

[JonRowe]

Thanks @sj26 I'm happy with it as is, if this solves the problem and doesn't fail the build its better in my eyes.

[sj26]

Thanks @JonRowe, @xaviershay!

[robd]

We have this problem in our suite. I was wondering - is there a way I can use this commit without having to monkey patch our codebase? I tried adding rspec-rails to our Gemfile specifying this ref (ac759a3) but I see the error

Could not find gem 'rspec-mocks (= 3.7.0.pre)', which is required by gem 'rspec-rails', in any of the sources.

I'm not too familiar with the rspec ecosystem versioning policy, but is there any chance this PR could be released as a 3.6 maintenance release? eg 3.6.1

Thanks!

[JonRowe]

In the mean time if you add master as the branch in your Gemfile, and do the same for the other rspec gems, you'll be able to use this.

[robd]

@JonRowe OK I see - thank you.

[yujinakayama]

@JonRowe Is there any reason not to release a patch version for this?

[JonRowe]

It's too minor to release on its own so no ones had time to do it, happy for you to do it if you have the time Yuji

[yujinakayama]

@JonRowe I think this issue is not so minor, since the noisy errors really annoy users when checking the log (actually I'm experiencing it). Could you give me the release permission on rubygems?

[robd]

@yujinakayama This would be fantastic if you have time to do a patch release. As another data point, in our case it has caused the log file to go from 6MB to 96MB for the test tranche I was looking at. We have about 40 test tranches per build, although I'm not sure if they are all affected as severely.

[JonRowe]

@yujinakayama you should be able to release it now

[ianks]

The problem with trying to use master in the Gemfile is that it resolves to 3.7.0.pre, which is incompatible versioning for some gems that I am using. A patch release would be super nice!

[robd]

@ianks Hey - there is now a 3.6 maintenance branch (https://github.com/rspec/rspec-rails/tree/3-6-maintenance) you can reference in your gemfile which includes this fix, but doesn't depend on 3.7.0.pre. Thanks @yujinakayama!

[yujinakayama]

I've just released rspec-rails 3.6.1 👍