Apply module/shared context inclusion to individual examples #1749
Conversation
myronmarston
force-pushed
the
more-powerful-include
branch
4 times, most recently
from
ccc524d to
a58a8ed
Compare
|
Any thoughts on this, @rspec/rspec? |
|
I'd like to see the benchmarks on this, and I'd ideally want the |
I've been leaning that way as well. That leads to another question, as well....should we do something to make this work? RSpec.configure do |config|
config.before(:context, :some_tag) do
end
end
RSpec.describe "Group" do
it "does stuff", :some_tag do
end
end |
|
Hmm. I'm leaning towards yes, because I'd be surprised it didn't work... |
myronmarston
force-pushed
the
more-powerful-include
branch
2 times, most recently
from
e1ef028 to
331ca53
Compare
myronmarston
force-pushed
the
more-powerful-include
branch
2 times, most recently
from
76a1414 to
5c0b6d2
Compare
|
OK, I've made some more progress on this. I feel like it's become a giant yak shaving exercise, unfortunately :(. A few things to note:
I'll keep chipping away on this but someone else can feel free to look it over and give feedback. |
|
So after implementing #1796 I realized that there's no way to keep the spec I added there green with how I've changed the implementation of filtered hooks in this PR. Specifically, if we make I think I'm going to have to roll back many of my later commits in this PR and take a different approach. |
myronmarston
force-pushed
the
more-powerful-include
branch
3 times, most recently
from
c1947ba to
31ed2c2
Compare
|
OK, I added benchmarks. It's a bit of a mixed bag. The benchmarks define and run 8400 examples at various nesting levels with metadata, and then there various benchmarks for:
The latter 3 were run with all 8400 examples having matching metadata and then with none of the examples having matching metadata. Takeaways:
As things currently stand, I'm not comfortable even with the perf to merge this as-is. But maybe we can find some low hanging fruit to optimize? In particular, I care the most about trying to get the perf of the case where examples don't match (i.e. test suites that don't leverage this feature) as close to the perf we have w/o these changes as possible. I'm far less concerned about the perf when this feature is being used, although it'd be nice to optimize that, too. @xaviershay, would you want to pair on some perf optimizations in these areas some time? I'm curious what others think about the perf cost vs the benefit of this change, too. /cc @rspec/rspec |
|
As a heavy user of these features I really don't want a big performance hit here... This will also have a knock on effect for users of |
|
I'm travelling in AU without a schedule for the next month, so might be hard to find a good time to pair. Will try to hit you up though, and if not I'll just have a poke at it anyways. |
|
Gonna be in Sydney at all? Or just Melbs? |
|
Just Melbourne. |
|
@JonRowe, by "these features" are you referring to the |
|
BTW, I've got an idea for how we can covert the linear "find matching modules/hooks/etc" checks into (practically) constant lookups, which I think should help considerably. I'm going to give that a shot. |
I am :) |
|
So I just pushed a WIP commit for my idea. See cb245d3. It shows a modest improvement over previous perf. I plan to keep playing with it but wanted to put it out there to get some early feedback if anyone wants to look at it. |
|
I extracted some of the refactoring changes (and other minor fixes) I had done as part of this PR (either in the original version of the PR or in what there is now) into #1805. The perf improvement I started playing with in the most recent commit is something that I think I'll work into its own PR as it should improve perf when applied to master, w/o the changes here (although, I think the perf diff is more dramatic with the changes here). I am still interested in other perf improvements offered by others... |
They are optimized for different cases.
- Config should use the QueryOptimized one because config usually sets up global hooks, inclusions, etc once at the start of the process (e.g. in `spec_helper`) and then is repeatedly queried as each example or group is defined. - Examples and groups should use the UpdateOptimized one as they are not queried by other examples or groups the way config is; instead, they can be updated multiple times based on having multiple global config hooks to process. Cutting out the additional update processing done by the QueryOptimized implementation greatly improves the performance of the `with_config_hooks` benchmark.
The call to `RSpec::CallerFilter.first_non_rspec_line` took most of the time. I originally planned to leverage rspec/rspec-support#155 but realized we could avoid calling that entirely by re-using the location from the group’s metadata. I re-ran all the benchmarks and updated them. They look much, much better.
myronmarston
force-pushed
the
more-powerful-include
branch
from
293dda7 to
a1a3dc4
Compare
|
I rebased against master now that #1848 has been merged so hopefully this will pass now. |
|
And it is indeed now green (including on AppVeyor). @JonRowe, please let me know if what's here is insufficient. If so, I can put some more time into profiling today, and I may choose to cherry-pick the last 4 commits to a new PR targeting master since they are general perf improvements that will help outside of this PR. |
|
I'll rerun these later today to find out! |
Did you get a chance to do this? |
|
Doing it now |
|
|
|
Much better, it's still slower but I guess I'm ok with it if you are. |
Apply module/shared context inclusion to individual examples
Somehow I forgot to add this before... [ci skip]
Somehow I forgot to add this before... [ci skip]
This regressed in #1749. The changes there affected code like this: ``` ruby RSpec.configure do |c| c.include Mod, :example_group => { :file_path => /./ } end ``` In RSpec 2, and in RSpec 3 before #1749, this snippet would cause `Mod` to be included in every example group (since `/./` matches all file paths). After #1749, it would _not_ be included in any example groups. Instead, it was included in the singleton class of every example. In practice, this usually worked OK, as the methods from `Mod` were available to be called very every example, but it had subtle problems: * If the same method was defined in multiple included modules, the version of the method defined in a module included in this fashion would always get precedence due to the singleton class getting "first dibs" during method dispatch over the example group class. (This is the broken example given in #2232). * I believe this broke `c.extend Mod, :example_group => {...}` since we don't extend onto singleton classes. The reason is that the optimizations included in #1749 considered only metadata keys that the groups and examples have in their metadata hashes. In a group hash, `:example_group` is not a "real" metadata key--instead it is created lazily on first access using a `default_proc`. But example hashes still have it (it is a reference to the metadata of their group), so it allowed example filtering to work correctly, which in turn hid the bug. Fixes #2232.
This regressed in #1749. The changes there affected code like this: ``` ruby RSpec.configure do |c| c.include Mod, :example_group => { :file_path => /./ } end ``` In RSpec 2, and in RSpec 3 before #1749, this snippet would cause `Mod` to be included in every example group (since `/./` matches all file paths). After #1749, it would _not_ be included in any example groups. Instead, it was included in the singleton class of every example. In practice, this usually worked OK, as the methods from `Mod` were available to be called very every example, but it had subtle problems: * If the same method was defined in multiple included modules, the version of the method defined in a module included in this fashion would always get precedence due to the singleton class getting "first dibs" during method dispatch over the example group class. (This is the broken example given in #2232). * I believe this broke `c.extend Mod, :example_group => {...}` since we don't extend onto singleton classes. The reason is that the optimizations included in #1749 considered only metadata keys that the groups and examples have in their metadata hashes. In a group hash, `:example_group` is not a "real" metadata key--instead it is created lazily on first access using a `default_proc`. But example hashes still have it (it is a reference to the metadata of their group), so it allowed example filtering to work correctly, which in turn hid the bug. Fixes #2232.
Apply module/shared context inclusion to individual examples
Somehow I forgot to add this before... [ci skip]
This regressed in rspec#1749. The changes there affected code like this: ``` ruby RSpec.configure do |c| c.include Mod, :example_group => { :file_path => /./ } end ``` In RSpec 2, and in RSpec 3 before rspec#1749, this snippet would cause `Mod` to be included in every example group (since `/./` matches all file paths). After rspec#1749, it would _not_ be included in any example groups. Instead, it was included in the singleton class of every example. In practice, this usually worked OK, as the methods from `Mod` were available to be called very every example, but it had subtle problems: * If the same method was defined in multiple included modules, the version of the method defined in a module included in this fashion would always get precedence due to the singleton class getting "first dibs" during method dispatch over the example group class. (This is the broken example given in rspec#2232). * I believe this broke `c.extend Mod, :example_group => {...}` since we don't extend onto singleton classes. The reason is that the optimizations included in rspec#1749 considered only metadata keys that the groups and examples have in their metadata hashes. In a group hash, `:example_group` is not a "real" metadata key--instead it is created lazily on first access using a `default_proc`. But example hashes still have it (it is a reference to the metadata of their group), so it allowed example filtering to work correctly, which in turn hid the bug. Fixes rspec#2232.
Somehow I forgot to add this before... [ci skip] --- This commit was imported from rspec/rspec-core@8bba91a.
This is an idea I'm toying with. I lean towards including it but I'm not totally sold on the idea yet.
### The problem
We have two different mechanisms for defining metadata-filtered shared context. On the configuration object:
...and as a shared context:
The latter is generally more useful because
shared_contextsupports all rspec-core constructs, includinglet, helper methods, etc...where as the config object only provides the means to define hooks that way. So I'd consider the latter form to be generally "better" but it has a downside:Both forms work fine in the first example (where the metadata has been applied to the group), but only the config-based approach (
:db_1) works in the second example. The problem is that theshared_contextis included (or not) in example groups, not individual examples, whereas the config can apply just fine. A user might reasonably expect the 2nd example to work, but as things currently stand, it won't. I myself have run into this a few times, where I had something that I had defined as ashared_contextbut thought I had defined onconfig, and I tagged an example and it didn't work.Solution
I realized recently that ruby's object model (particularly the
singleton_class) provides a means to very simply address this issue: we can include modules/shared contexts directly in the singleton class of the example group instance owned by an individual example. This allows the contents of the module or shared context to apply to one example, but not others in the group. I'm pleased with how simple the implementation was.However, I'm not 100% sold on this, for a few reasons:
before(:context)/after(:context)hooks that are defined in the shared context. As currently implemented, when the context is applied to one example, these do not run. However, they potentially setup and teardown some state that other parts of the context (such as anaroundhook) use, so not running them could cause problems. I'm on the fence about whether or not they should be run.config.extendmodules aren't treated the same way. I didn't think it made sense to apply those modules the same, since they generally define class-level helper methods (e.g. macros) that are called from within adescribeorcontextblock. (But can anyone thing of a need or use to treat them the same way?)TODO:
:contexthooks.Feedback wanted.
/cc @rspec/rspec