Fix define_derived_metadata so that it supports cascades.

[myronmarston]

For example:

RSpec.configure do |c|
  c.define_derived_metadata(:elasticsearch) do |meta|
    meta[:vcr] = true
  end

  c.define_derived_metadata(:vcr) do |meta|
    meta[:retries] = 2
  end
end

With this configuration, an example or group tagged with :elasticsearch
should get tagged with :vcr as well, which in turn should add
retries: 2 metadata to the example or group. Before this change,
this did not work properly, because we did look to see if additional
metadata blocks should apply after applying them once.

[JonRowe]

These are being treated as hashes, of course, but we still support 1.8.7, so we need the hash rocket syntax :)

[myronmarston]

Thanks. Fixed.

[JonRowe]

I see how this demonstrates the cascade, but not the re-applied logic...

[myronmarston]

It demonstrates that the blocks are not re-applied by asserting that the values of each of foo* is 1. The blocks each increment the value, so if any of the define_derived_metadata blocks are run multiple times, then the valule would be something greater than 1 and the expectation below would fail.

How can I make this more clear?

[JonRowe]

I wouldn't expect in this scenario the blocks to be run more than once though, but this would demonstrate the issue more clearly:

c.define_derived_metadata(:foo1) { |m| m[:foo2] = (m[:foo2] || 0) + 1 }
c.define_derived_metadata(:foo2) { |m| m[:foo3] = (m[:foo3] || 0) + 1 }
c.define_derived_metadata(:foo3) { |m| m[:foo1] = (m[:foo1] || 0) + 1 }

group = RSpec.describe("bar", :foo1 => 1)
expect(group.metadata).to include(:foo1 => 2, :foo2 => 1, :foo3 => 1)

[myronmarston]

I wouldn't expect in this scenario the blocks to be run more than once though

They would run more than once if it wasn't for the unless skip block bit on the block.call(metadata) unless skip_block line.

Anyhow, I've changed to:

        RSpec.configure do |c|
          c.define_derived_metadata(:foo1) { |m| m[:foo2] = (m[:foo2] || 0) + 1 }
          c.define_derived_metadata(:foo2) { |m| m[:foo3] = (m[:foo3] || 0) + 1 }
          c.define_derived_metadata(:foo3) { |m| m[:foo1] += 1 }
        end

        group = RSpec.describe("bar", :foo1 => 0)
        expect(group.metadata).to include(:foo1 => 1, :foo2 => 1, :foo3 => 1)

I think it's a bit easier to see that each block runs only once if the values are all 1 (if one value is 2 it makes it seem like a block ran twice I think), so we can start foo1 at 0 to achieve this.

[JonRowe]

Isn't there potential here for an infinite loop? Can we add a "break" that limits it to something even if it's rather high?

[myronmarston]

As long as the user is not defining new derived_metadata_blocks within a derived metadata block, I think it is guaranteed to terminate. Here's my reasoning:

• Assume that @derived_metadata_blocks is not changing (I'll discuss that case separately).
• On a given loop iteration, one of two things will happen:
  • @derived_metadata_blocks.items_for(metadata) will return 1 or more blocks that have not previously been returned on a prior loop iteration
  • @derived_metadata_blocks.items_for(metadata) will return the same set of block that have been previously returned
• If it's the latter case, the all? will return true (because already_run_blocks.include?(block) will be true for all blocks), and the loop will break.
• If it's the former case, the newly returned blocks will be called and added to already_run_blocks, and the loop will iterate again. However, we will be closer to having run all of the blocks in @derived_metadata_blocks, and there is no way for the former case to happen indefinitely. At some point the latter case much occur because @derived_metadata_blocks has a finite number of blocks.

The assumption I stated above could be violated with some weird code like:

RSpec.configure do |c|
  c.define_derived_metadata(a: 0) do |meta|
    met[:a] += 1
    c.define_derived_metadata(a: meta[:a] + 1) do
      # ...
    end
  end
end

e.g. where the user is adding new blocks each time the block is triggered, and updating the metadata to match it. I don't think this is a case any user will actually try, though, and if they do, well it's no different than a user writing a let that is self-referential and therefore recurses indefinitely. Infinite recursion is possible with many constructs in Ruby and RSpec, and I don't think there's a viable way for us to guard against that without also preventing potentially valid use cases.

We could add a hard limit, but any limit would be arbitrary, and for any limit we can come up with, a valid use case exists for it happening once more. Plus, it would be very odd to abort at an arbitrary point; it would be like a ruby method that recurses on itself arbitrarily stopping after some number of iterations.

[JonRowe]

it would be like a ruby method that recurses on itself arbitrarily stopping after some number of iterations.

Which is exactly what a StackLevelTooDeep error is... and this would hang indefinitely instead

[myronmarston]

Good point; raising SystemStackError is reasonable. I thought you were suggesting that we silently stop after a certain number of iterations.

I've updated this to raise SystemStackError after 200 iterations.

[JonRowe]

Apologies for the confusion, I was advocating failing noisily if a limit was hit, I agree a stack error seems appropriate. I ponder if this could be refactored to be a recursive method to trigger the error organically at the same limit as the Ruby vm? Not a blocker however.

[myronmarston]

I wondered the same thing, but can't think of a way to refactor it that way.

[myronmarston]

Can you stamp this, @JonRowe?

[JonRowe]

Rubocop is complaining about shadowing here.

[myronmarston]

Thanks, fixed.

[JonRowe]

Thanks @myronmarston!