Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Determining the rspec context in which a helper method is run #2893

Closed
odinhb opened this issue Jun 9, 2021 · 12 comments · Fixed by #2895
Closed

Determining the rspec context in which a helper method is run #2893

odinhb opened this issue Jun 9, 2021 · 12 comments · Fixed by #2895

Comments

@odinhb
Copy link
Contributor

odinhb commented Jun 9, 2021

Subject of the issue

I'm writing a small helper function for my test suite, and I'd like to protect both future me and my colleagues from using said method inside rspec's before(:all) hook (or extensions that run in the same context like let_it_be/before_all from test-prof). It just doesn't make sense to run it in that way.

The current way of detecting this context relies on the string output of self.inspect when running said method.
Alternatively, you can check the @__inspect_output instance variable.
The check looks like this:

# this lives in a module, included in the example group
def my_helper
  # self is an example group here
  if inspect.include?("before(:context)")
    raise friendly_error
  end

  # do stuff
end

You can see test-prof relying on the same hack here, and rspec-rails relying on it here.

Could we get an official way of detecting this context? It's easy to imagine this string changing w/o warning.

Your environment

  • Ruby version: ruby 2.6.6p146 (2020-03-31 revision 67876) [x86_64-linux]
  • rspec-core version: 3.10.0
@pirj
Copy link
Member

pirj commented Jun 9, 2021

Sounds like a reasonable addition. Would you like to take a stab at it?

@odinhb
Copy link
Contributor Author

odinhb commented Jun 9, 2021

Would you like to take a stab at it?

Indeed. Although I'm not sure what exactly the design should be.

# return a symbol?
self.context # => :example/:group

# or return something introspective?
self.context.example? # => true/false
self.context.example_group? # => true/false ?

I'll find some time to tinker with it. Maybe I'll come up with more ideas then?

@pirj
Copy link
Member

pirj commented Jun 9, 2021

The usage of context is ambiguous here. I believe scope is more appropriate.

I might be mistaken, but it might be that the before(:context) returned by inspect originates from here:

# lib/rspec/core/example_group.rb
run_before_context_hooks(new('before(:context) hook')) if should_run_context_hooks

There's yet another dimension, though before/after:

run_after_context_hooks(new('after(:context) hook')) if should_run_context_hooks

however, it's of no use to the consumers that you've mentioned.

The usefulness of the new indicator would be focused around providing information if something was called from inside before(:context) or in the examples scope. Where "something" is a helper method defined by let_it_be or a fixture accessor. Please correct me if I'm mistaken.
I'm on the fence to advise for a generic solution that would provide a rich interface used by no consumers except for one method, or a more specific one that would look odd (before_context_scope?) but will do the job.

@odinhb
Copy link
Contributor Author

odinhb commented Jun 10, 2021

For the helper I implemented it also doesn't make sense to run it in an after(:context) hook, so It would be useful to detect that in some way. I just forgot/didn't think about it. I deemed it too unlikely that someone would use the method in that way.

I get it though. It doesn't make sense to implement a complicated interface nobody will use.

@pirj
Copy link
Member

pirj commented Jun 10, 2021

This new thing will be easy to use in rspec-rails. Say, when Rails hits 7.0, rspec-rails hits 6.0 and will depend on the rest of RSpec projects to be 4.0+.

There's still time to push this to rspec-core 4.0.0, or even 3.11.0.
It's a bit more complicated with test-prof though. It has just hit 1.0 recently, and I'm not confident if they would like to adopt a minor code improvement sacrificing the compatibility with RSpec < 3.10. Eventually, this will happen, but probably not before test-prof 2.0.

@JonRowe
Copy link
Member

JonRowe commented Jun 10, 2021

For implementation, this needs to be thought through a bit, suite hooks are executed by the runner, example group hooks at the class level for that class, and for examples its the instance themselves.

Thus for a consistent api we would have to have potentially 3 places to access the current "context status", which is tricky for the example itself, because we want that API to be super low api surface so as not to conflict with developer code. (For example, a method named type would be a big no no as thats a super common let name).

Potentially metadata could be used for this, although I'm wary of abusing that too, but at least that is an RSpec controlled space.

@odinhb
Copy link
Contributor Author

odinhb commented Jun 11, 2021

Not sure how easy it is to implement (haven't touched the code yet), but I have an idea:

RSpec.current_scope # => :example_group/:example/:config/etc etc

That way there's no way anybody has defined a variable/let block/method that conflicts w/ the api.

@odinhb
Copy link
Contributor Author

odinhb commented Jun 15, 2021

So I found some code in example_group.rb. Which already kinda implements this. It's 5 years old.

f8e555d

Problem with this method is that it raises you a WrongScopeError about not being available outside an example group both when inside an example and when inside before/after hooks.

I'm partial to deleting this code and reimplementing it on the RSpec constant instead. My only question is how I or anyone else was supposed to discover that this method even existed and start using it. Was it for lack of documentation or the impracticality of dealing with an exception?

@pirj
Copy link
Member

pirj commented Jun 15, 2021

Nice find!
Do you think if it won't raise if you implement an instance method with the same name? I believe it could always return false.

I'm not certain if implementing it under RSpec is a good idea. We're thinking of making RSpec thread-safe, and depending on how you'd implement it, would share the state between threads.

@odinhb
Copy link
Contributor Author

odinhb commented Jun 15, 2021

Do you think if it won't raise if you implement an instance method with the same name? I believe it could always return false.

I could give that a shot, but I'm not a big fan of the api. Do you think anybody is relying on the existing method?

I'm not certain if implementing it under RSpec is a good idea. We're thinking of making RSpec thread-safe, and depending on how you'd implement it, would share the state between threads.

So far I've implemented it on RSpec::Support.thread_local_data[:current_scope] which I found by virtue of the similar RSpec.current_example accessor. Wouldn't this be thread-safe?

@pirj
Copy link
Member

pirj commented Jun 15, 2021

thread_local_data. Wouldn't this be thread-safe?

Should be perfectly fine.

Do you think anybody is relying on the existing method?

Quite unlikely, but still possible.
It's not marked explicitly as a part of the private API, so our obligation is to keep it intact in between major versions. If we happen to decide to remove it, we still have time to do so before RSpec 4 is out.

not a big fan of the api

Is it the fact that the method is available in both scopes, but would be implemented differently for them concerning you?

So far I've implemented

👍
Do you mind sending a PR, so it's easier to reason about the current and the new implementation?

@odinhb
Copy link
Contributor Author

odinhb commented Jun 15, 2021

Is it the fact that the method is available in both scopes, but would be implemented differently for them concerning you?

If someone needs to differentiate between before/after or before(:all)/before(:each) the api is not extensible without adding yet more cruft to the public api of examples and example groups.

Do you mind sending a PR, so it's easier to reason about the current and the new implementation?

I'll send a PR soon. The implementation was really more of an experiment, but I'll give you something concrete to look at now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants