Skip context all blocks #2442

devonestes · 2017-07-12T12:06:54Z

This skips any hooks for contexts that are entirely skipped. Since I don't know much about how all this stuff interacts, I'd be happy to go at this again if this looks like it will cause problems.

Resolves #2436

devonestes · 2017-07-12T14:21:46Z

So I'm getting a strange failure in CI with JRuby 9000 (as well as some other transient JRuby failures). Here are some examples of the past CI runs:

https://travis-ci.org/rspec/rspec-core/builds/252781795
https://travis-ci.org/rspec/rspec-core/builds/252802216

I'm not 100% sure what's happening here: https://travis-ci.org/rspec/rspec-core/jobs/252781808, so if anyone has any input, that would be much appreciated! I'll keep searching on my own, but I just wanted to ask in case someone had a quick answer on hand.

devonestes · 2017-07-12T14:56:01Z

Ok, nevermind, it finished now? There appears to be some flakiness in those JRuby builds.

myronmarston

Thanks for putting together your own PR to solve your problem!

I left a comment on your implementation. Beyond that, I have a larger concern: this doesn't work for all cases where an entire example group is skipped. Consider a case like this:

RSpec.describe "Outer" do
  before(:context) { ... }

  xcontext "nested" do
    # ...
  end
end

Here, the example group containing the before(:context) hook is not being skipped, but the nested group is (which is the only group or example it contains). I don't believe either of our implementations would fix this.

Another example:

RSpec.describe "Group" do
  before(:context) { ... }

  xit "does one thing" do
    # ...
  end

  xit "does another thing" do
    # ...
  end
end

Here all the examples in the group are individually skipped.

Ideally, the :context hook would be skipped in both cases. However, to implement that, I believe it would require us to recursively check the examples in the group (and in any nested groups) to see if there are any non-skipped examples or not. Given that skipping a :context hook for this case is really just an optimization, I don't think I want RSpec to incur the cost of doing that recursive check, as it could have a noticeable impact on perf for a relatively small optimization.

I think the simple direct check of the example group's :skip metadata is the right trade off to make, as long as we're all aware and OK with the fact that it doesn't provide this optimization for all cases.

myronmarston · 2017-07-14T09:09:32Z

lib/rspec/core/hooks.rb

@@ -503,6 +503,8 @@ def all_hooks_for(position, scope)
        end

        def run_owned_hooks_for(position, scope, example_or_group)
+          return if example_or_group.class.respond_to?(:metadata) &&
+                      example_or_group.class.metadata.keys.include?(:skip)


There's a few things about this that are a bit sub-optimal:

metadata.keys.include?(:skip) is a linear operation, scanning an array of metadata keys. It's more efficient to use Hash#key? by doing metadata.key?(:skip) as that's a constant time check.

Checking respond_to? is sub-optimal, but I see why it's necessary here, because this is a central place called by both example groups and examples. Instead, I think you can move this check to the call site of this method for the :context hook case. Here's a diff that does that:

diff --git a/lib/rspec/core/hooks.rb b/lib/rspec/core/hooks.rb index 4a69b76..0f77064 100644 --- a/lib/rspec/core/hooks.rb +++ b/lib/rspec/core/hooks.rb @@ -456,7 +456,9 @@ module RSpec return if RSpec.configuration.dry_run? if scope == :context - run_owned_hooks_for(position, :context, example_or_group) + unless example_or_group.class.metadata.key?(:skip) + run_owned_hooks_for(position, :context, example_or_group) + end else case position when :before then run_example_hooks_for(example_or_group, :before, :reverse_each) @@ -503,8 +505,6 @@ module RSpec end def run_owned_hooks_for(position, scope, example_or_group) - return if example_or_group.class.respond_to?(:metadata) && - example_or_group.class.metadata.keys.include?(:skip) matching_hooks_for(position, scope, example_or_group).each do |hook| hook.run(example_or_group) end

Notice that there's no need to check respond_to? in this case.

Actually, one other thing I just realized: the check needs to be example_or_group.class.metadata[:skip] because if the user tags a group with :skip => false it should not be skipped, so checking for the presence of the :skip key is insufficient.

Thank you for such kind, detailed feedback! It's always so helpful!!

I'll revise accordingly and push something back up in a second.

myronmarston · 2017-07-14T09:20:48Z

Regarding the JRuby build flakiness: this has been an ongoing problem ever since we added the bisect feature cukes (so it's been going on for a couple years now). None of the RSpec core devs has had the time or inclination to dig into it and fix it, though. (I don't even know if any of the core devs are active JRuby users to know where to begin). If you (or anyone reading this) wanted to dig into fixing that, it would be a huge help! Also, the JRuby build is quite slow compared to MRI--about 10x slower, sadly. Again, we'd welcome some help here!

devonestes · 2017-07-14T11:30:26Z

Funny enough, in the examples you give above, I would actually find that to be confusing behavior. That said, I can 100% see how others would see it as totally expected and normal behavior.

I guess the way I think about those before(:context) and after(:context) blocks is that they're completely independent of anything in an inner scope. As a user, I would expect those to run even if a given context had no examples, like this:

RSpec.describe "Outer" do
  before(:context) { ... }
end

Since we're still execting that block passed to describe, I would think that everything in that block should be run. It's only when we're skipping that block using xdescribe (or adding skip: true) did it seem to me like we shouldn't be evaluating anything in that block, including those before and after hooks.

I would totally think that in the cases you mentioned, and in the one I mentioned above, that skipping the execution of those blocks would be a nice optimization, but I agree that it would probably be a net negative for 99.99% of users!

myronmarston

One more change needed, then we can merge this.

myronmarston · 2017-07-14T12:04:13Z

lib/rspec/core/hooks.rb

@@ -455,7 +455,7 @@ def register(prepend_or_append, position, *args, &block)
        def run(position, scope, example_or_group)
          return if RSpec.configuration.dry_run?

-          if scope == :context
+          if scope == :context && !example_or_group.class.metadata[:skip]


I believe there is a bug here. Consider what happened before your change when scope == :context: it never executed the else clause code. Consider what happens now when the group is not tagged with :skip: the 2nd part of the conditional evaluates to false, and the else clause code will get executed. That will potentially result in before(:example) hooks running an extra time (when RSpec is intending to run before(:context) hooks). So I think this needs to be:

if scope == :context unless example_or_group.class.metadata[:skip] # ... end else # ... end

Ah, yeah, sorry. I guess I was relying on the tests a little too heavily to do my thinking for me! When they passed I figured I was good. Fixed now!

Well, that goes to show that our test suite has some holes in it. Would you be up for adding a test to cover that case?

Yeah, sure thing!

PragTob · 2017-07-14T17:28:18Z

re JRuby build flakiness, I'm 99% sure it's just a timeout due to big startup time. I'll give it a quick look and see if I can adjust a screw or 2.

Startup time is still very much a problem with JRuby, hence feature type aruba cukes that invoke rspec a lot of times are still problematic. The bisect specs do this to the extreme, resulting in long spec run times. Locally a single scenario takes 36 seconds to run for me, on a 4-core desktop computer. An overloaded CI might take longer, hence just double the timeout and hope it's enough. Nothing much else we can do. ref: rspec#2442

myronmarston

Thanks for your quick changes, @devonestes! I was about to hit the big green merge button but noticed a couple minor things I didn't notice before--mind addressing them before we merge?

myronmarston · 2017-07-15T00:54:44Z

spec/rspec/core/hooks_filtering_spec.rb

+          c.after(:context) { filters << "after context in config"}
+        end
+        group = RSpec.xdescribe("skipped describe") do
+          xcontext("skipped context") do


I just noticed that you are skipping both the nested group and the outer group. Is that necessary to trigger this behavior? It seems like it shouldn't be, but if it's not, it begs the question about why the spec is skipping at two levels.

myronmarston · 2017-07-15T00:55:51Z

spec/rspec/core/hooks_filtering_spec.rb

+          xcontext("skipped context") do
+            it("is skipped") {}
+          end
+        end


I just realized that this spec is only covering global config hooks, but does not cover :context hooks declared in the skipped example group itself. Given that in this case we want all :context hooks to be skipped, regardless of where they are declared, it would be good to cover that in this spec. Mind improving the spec before we merge?

Startup time is still very much a problem with JRuby, hence feature type aruba cukes that invoke rspec a lot of times are still problematic. The bisect specs do this to the extreme, resulting in long spec run times. Locally a single scenario takes 36 seconds to run for me, on a 4-core desktop computer. An overloaded CI might take longer, hence just double the timeout and hope it's enough. Nothing much else we can do, but try using --dev see rspec/rspec-dev#179 ref: rspec#2442

This adds a test that I think should represent the behavior we want. I did my best to keep the tests consistent with the other specs in that file.

This makes that previous test pass, and the test suite is still green, so I'm _assuming_ that this works and doesn't break other stuff! However, I'd be happy to make any alterations if this doesn't look like it's the right way of tackling this problem.

devonestes · 2017-07-17T13:15:57Z

I made those additions to that test, as well as adding a test explicitly describing the expected behavior when all examples in a context are skipped.

For the life of me I tried adding a failing test for that time I had accidentally introduced that bug, but everything kept passing ❓ I think it's because the visible behavior won't actually change since when we hit that else clause we end up traversing the tree of parent groups and running the hooks owned by those groups, but it would have affected run time for sure because we would essentially be doing those tree traversals twice for every :each|:around hook instead of once, even though we'd only be executing those blocks once.

I'll keep thinking about how I could tighten that up, but if something comes to me I can add it in another PR later.

myronmarston · 2017-07-18T11:01:13Z

Thanks, @devonestes!

[ci skip]

Startup time is still very much a problem with JRuby, hence feature type aruba cukes that invoke rspec a lot of times are still problematic. The bisect specs do this to the extreme, resulting in long spec run times. Locally a single scenario takes 36 seconds to run for me, on a 4-core desktop computer. An overloaded CI might take longer, hence just double the timeout and hope it's enough. Nothing much else we can do, but try using --dev see rspec/rspec-dev#179 ref: rspec#2442

Skip context all blocks

[ci skip]

devonestes force-pushed the skip-context-all-blocks branch 4 times, most recently from a53c46f to 3dab6d1 Compare July 12, 2017 13:43

myronmarston suggested changes Jul 14, 2017

View reviewed changes

devonestes force-pushed the skip-context-all-blocks branch from 3dab6d1 to 1de4389 Compare July 14, 2017 11:20

myronmarston suggested changes Jul 14, 2017

View reviewed changes

devonestes force-pushed the skip-context-all-blocks branch from 1de4389 to f8d8ef4 Compare July 14, 2017 12:15

PragTob mentioned this pull request Jul 14, 2017

Reduce JRuby flakyness by giving it more time to finish #2444

Merged

myronmarston reviewed Jul 15, 2017

View reviewed changes

devonestes force-pushed the skip-context-all-blocks branch 3 times, most recently from ae6596e to f35d5c4 Compare July 17, 2017 13:10

devonestes added 2 commits July 17, 2017 15:12

Add failing test for new feature

ff70bc9

This adds a test that I think should represent the behavior we want. I did my best to keep the tests consistent with the other specs in that file.

devonestes force-pushed the skip-context-all-blocks branch from f35d5c4 to 3735f30 Compare July 17, 2017 13:12

myronmarston approved these changes Jul 18, 2017

View reviewed changes

myronmarston merged commit acd503f into rspec:master Jul 18, 2017

devonestes deleted the skip-context-all-blocks branch July 18, 2017 11:01

myronmarston added a commit that referenced this pull request Jul 18, 2017

Add changelog for #2442.

bcf7d9e

[ci skip]

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Merge pull request rspec#2442 from devonestes/skip-context-all-blocks

bd76d10

Skip context all blocks

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Add changelog for rspec#2442.

56dd2cf

[ci skip]

Uh oh!

Skip context all blocks #2442

Skip context all blocks #2442

Uh oh!

Conversation

devonestes commented Jul 12, 2017

Uh oh!

devonestes commented Jul 12, 2017

Uh oh!

devonestes commented Jul 12, 2017

Uh oh!

myronmarston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Jul 14, 2017

Uh oh!

devonestes commented Jul 14, 2017

Uh oh!

myronmarston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PragTob commented Jul 14, 2017

Uh oh!

myronmarston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devonestes commented Jul 17, 2017

Uh oh!

myronmarston commented Jul 18, 2017

Uh oh!

Uh oh!