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

Use RSpec's when_first_matching_example_defined if available #1496

Merged
merged 2 commits into from
Jan 29, 2023

Conversation

mcls
Copy link
Contributor

@mcls mcls commented Dec 14, 2022

This RSpec method was added in v3.5.0 to with expensive setup logic. It results in similar behavior as just using before(:example, ...), but it will only run once instead of for each example. So the behavior remains the same, but with the small optimization of not having to check for compiled assets on every example.

Related documentation:


This change is Reviewable

@justin808
Copy link
Member

@mcls

  1. What is the difference between doing this and a before(:suite)?
  2. Assuming this is needed, please update the CHANGELOG.md and update the docs, https://github.com/shakacode/react_on_rails/blob/master/docs/guides/rspec-configuration.md, to show how to use this change.

This RSpec method was added in v3.5.0 to with expensive setup logic. It
results in similar behavior as just using `before(:example, ...)`, but
it will only run once instead of for each example.
@mcls
Copy link
Contributor Author

mcls commented Dec 26, 2022

Hi Justin,

  1. What is the difference between doing this and a before(:suite)?

before(:suite) does not support metadata filtering and thus doesn't support the same conditional running of setup code like when_first_matching_example_defined does. I was initially confused by this too, but you can find some additional context here in the original RSpec PR which introduces the feature: rspec/rspec-core#2175

  1. Assuming this is needed, please update the CHANGELOG.md and update the docs, https://github.com/shakacode/react_on_rails/blob/master/docs/guides/rspec-configuration.md, to show how to use this change.

I have updated the CHANGELOG.md and pushed an updated commit. Regarding the docs, I just checked but I don't think they need to be updated because this is just an internal change to ReactOnRails::TestHelper.configure_rspec_to_compile_assets.

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine.

Just want to check that somebody tested this with the versions where when_first_matching_example_defined is not available.

And we need to resolve conflict in the changelog.

@justin808
Copy link
Member

@Judahmeek or @ahangarha Please merge this after resolving the conflict.

@ahangarha
Copy link
Contributor

@justin808 I don't have write access to this repo so I cannot resolve the conflict and merge.

@justin808
Copy link
Member

@mcls @ahangarha can I get confirmation:

Just want to check that somebody tested this with the versions where when_first_matching_example_defined is not available.

and then I'll merge.

@justin808 justin808 merged commit 78c37d9 into shakacode:master Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants