Fix adding config hooks to existing example groups #2280
Conversation
|
Can you rebase this against master? #2279 fixes the build failures you are seeing here. |
| @@ -2033,6 +2032,12 @@ def on_existing_matching_groups(meta) | |||
| end | |||
| end | |||
|
|
|||
| def add_hook_to_existing_groups(prepend_or_append, position, *meta, &block) | |||
There was a problem hiding this comment.
Instead of adding a hook-specific method (which in turn delegates to some new logic added to the Hooks module for this case), is there a way to make on_existing_matching_groups handle nested groups properly? It would be nice if the generic logic in configuration for applying metadata-based config to existing groups worked with nested groups so we don't have to solve issues like this in an ad-hoc manner for each metadata-based config thing.
There was a problem hiding this comment.
I thought the behaviour of helper modules and hooks were too different to use the same logic - helper modules are currently applied to every existing matching group, whereas hooks need to only be applied to the highest level matching groups.
However it looks like when creating a new example group, helper modules work that way too: https://github.com/rspec/rspec-core/blob/v3.5.0/lib/rspec/core/configuration.rb#L2044
I pushed a new commit that updates the behaviour of on_existing_matching_groups, and uses it to apply both helper modules and hooks. I'm not happy with some things about the implementation - I'll comment inline where I suspect there might be a better way to do something.
If we want to go in this direction, I think we should fix the behaviour of helper modules separately, and then rebase this change to just fix the issues with hooks afterwards.
When an example group is created, the existing config hooks are added to it if their metadata filters match and none of the new example group's parent groups already have the hook added. The current implementation adds new config hooks to all existing example groups, which means they can be run multiple times if several nested example groups all match their metadata filters, which doesn't happen if the config hook is defined first. To get the same behaviour regardless of the order in which hooks and groups are defined, we can walk the inheritence heirarchy starting from the top level example groups, and only add the new hook to the first matching group on each branch.
eugeneius
force-pushed
the
fix_adding_hooks_to_existing_groups
branch
2 times, most recently
from
639c720 to
93b16b2
Compare
For the behaviour when apploying helper modules to existing example groups to be fully consistent with the behaviour for new groups, we shouldn't apply the module if any of the example group's parent groups already have the module applied. Also refactor how we add hooks to existing groups to use the same code.
eugeneius
force-pushed
the
fix_adding_hooks_to_existing_groups
branch
from
93b16b2 to
7e1799d
Compare
| until groups.empty? | ||
| group = groups.shift | ||
|
|
||
| if scope == :example || scope == :each || scope.nil? || |
There was a problem hiding this comment.
This duplicates some very specific hook-related logic:
:examplehooks are always added, even if they don't match the group's metadata: https://github.com/rspec/rspec-core/blob/v3.5.0/lib/rspec/core/hooks.rb#L519:eachis an alias for:example: https://github.com/rspec/rspec-core/blob/v3.5.0/lib/rspec/core/hooks.rb#L478- when no scope is passed,
:exampleis implied: https://github.com/rspec/rspec-core/blob/v3.5.0/lib/rspec/core/hooks.rb#L586
In the previous implementation this logic were still duplicated, but at least it was still in the Hooks module.
|
Thanks for pushing your updates, @eugeneius! I'm going to try to review it tomorrow. |
|
I ran into the same problem today and I can confirm this branch fixes the issue FWIW |
|
Great work, @eugeneius! |
|
Thanks for the review @myronmarston! 🙏 |
…ting_groups Fix adding config hooks to existing example groups
This commit was imported from rspec/rspec-core@71c835b.
Followup to #2189.
When an example group is created, the existing config hooks are added to it if their metadata filters match and none of the new example group's parent groups already have the hook added.
The current implementation adds new config hooks to all existing example groups, which means they can be run multiple times if several nested example groups all match their metadata filters, which doesn't happen if the config hook is defined first.
To get the same behaviour regardless of the order in which hooks and groups are defined, we can walk the inheritance hierarchy starting from the top level example groups, and only add the new hook to the first matching group on each branch.