Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Reload hooks when metadata changes #1089

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
3 participants
Owner

JonRowe commented Sep 30, 2013

This is a trivial take at solving rspec/rspec-rails#829 but simply watching for metadata changes
and then applying hooks again when they change. Hooks shouldn't be included twice due to the
checks in the existing call.

Simply moving the call to register the hooks didn't work due to ordering on the hooks.

Thoughts? /cc @myronmarston

@JonRowe JonRowe added a commit to rspec/rspec-rails that referenced this pull request Sep 30, 2013

@JonRowe JonRowe cleanup and remove own solution (depends on rspec/rspec-core#1089) e8475dc
Owner

myronmarston commented Sep 30, 2013

I think I'd rather see the hook registration delayed until they are actually needed, as I suggested in the rspec-rails issue:

One idea is to move the hooks.register_globals call out of set_it_up and into run, so that the hooks are registered at the last possible moment.

Any reason that won't work? Seems far less complex, and would perform better.

Owner

JonRowe commented Sep 30, 2013

It completely buggers the ordering, this is actually a far simpler change.

Owner

myronmarston commented Sep 30, 2013

It completely buggers the ordering, this is actually a far simpler change.

Can you expand on that a bit? I'm not following...

Owner

JonRowe commented Sep 30, 2013

Moving the call to run means that the hooks from the configuration will run after local hooks (not desirable).

So I attempted to solve this by changing how the hooks are added to the array but then local hooks are just intermingled with the configuration hooks, so I moved the registration call to the hook methods so it's a "JIT" registration of hooks, however this messes with the ordering of hooks in nested contexts.

Owner

JonRowe commented Sep 30, 2013

So at this point the best place to load the configuration hooks is actually in the existing place, and then load in any extra hooks on metadata changes, or rewrite the hook system so we have more control over the ordering. The later is more complexity and more overhead.

@myronmarston myronmarston and 2 others commented on an outdated diff Oct 1, 2013

lib/rspec/core/metadata.rb
@@ -50,6 +50,16 @@ def self.build_hash_from(args)
hash
end
+ # @private
+ def on_change &block
+ @callback = block
+ end
+
+ def []=(key, value)
+ super
+ @callback.call if defined?(@callback)
+ end
@myronmarston

myronmarston Oct 1, 2013

Owner

There are other ways to change an metadata hash, right? Such as store, or any of the xyz! methods. Should the on_change hook be triggered for them?

@JonRowe

JonRowe Oct 1, 2013

Owner

This is the only one we use AFAIK but yes they should all follow this pattern, I'll go over the enum spec and add them all.

@JonRowe

JonRowe Oct 1, 2013

Owner

I've updated this for the ones that set metadata, this doesn't deregister hooks at the moment (that would require more changes, do we want to support that?)

@myronmarston

myronmarston Oct 1, 2013

Owner

I don't think we need to deregister hooks. on_change is private, anyway.

I've updated this for the ones that set metadata,

What about delete, delete_if, select! and reject!?

@JonRowe

JonRowe Oct 1, 2013

Owner

They don't set metadata, but remove it, hence my comment

this doesn't deregister hooks at the moment (that would require more changes, do we want to support that?

@myronmarston

myronmarston Oct 2, 2013

Owner

Ah, when you said "deregister hooks" I thought you meant provide a way to deregister from receiving on_change notification. I didn't realize you meant rspec's before/around/after hooks.

Hmm....I'm a bit on the fence about this. I feel like that if we're going to support mutating the metadata after-the-fact as a public API, we should fully support it. OTOH, I'm OK stating that it's not intended to be a public API but is just intended for use by rspec-rails and is subject to change in a minor release.

Thoughts from others?

/cc @alindeman @xaviershay @samphippen @soulcutter

@JonRowe

JonRowe Oct 2, 2013

Owner

I'm ok with that too, or even just publicly stating we only support adding to metadata. If we stopped inheriting from hash and delegating to an internal hash we could control the api and reject those methods totally.

@myronmarston

myronmarston Oct 10, 2013

Owner

It'd be good to merge this PR soon, but I'd still like others to weight in (I'm still very much on the fence). @xaviershay? @samphippen? @soulcutter?

@xaviershay

xaviershay Nov 2, 2013

Member

I'm OK stating that it's not intended to be a public API but is just intended for use by rspec-rails and is subject to change in a minor release.

This. We should collect stronger use-cases before exposing this publicly.

@xaviershay

xaviershay Nov 2, 2013

Member

Reading this more, seems like we're between a rock and a hard place. We've exposed a full Hash as our interface, so are at the mercy of future versions of ruby to keep the method signatures that we're overriding for dirty tracking the same.

Seems like since we already have an iteration over methods in the latest version, it would be straight forward to just add the extra methods there.

@myronmarston myronmarston and 1 other commented on an outdated diff Oct 1, 2013

lib/rspec/core/metadata.rb
@@ -50,6 +50,16 @@ def self.build_hash_from(args)
hash
end
+ # @private
+ def on_change &block
+ @callback = block
@myronmarston

myronmarston Oct 1, 2013

Owner

@callback is a pretty generic name. Perhaps this should be @on_change_callback.

Also, I prefer non-Seattle style method defs (e.g. def on_change(&block)).

@JonRowe

JonRowe Oct 1, 2013

Owner

Sorry, force of habit slaps wrist, I'll rename it as well

@myronmarston myronmarston commented on an outdated diff Oct 1, 2013

spec/rspec/core/example_group_spec.rb
@@ -52,6 +52,19 @@ def metadata_hash(*args)
expect(examples_run.count).to eq(1)
end
+ it "runs before hooks when configured with metadata" do
@myronmarston

myronmarston Oct 1, 2013

Owner

I don't think this doc string really describes the issue very well. Maybe this instead?

context "when metadata is added later" do
  it "still runs the matching config hooks" do
  end
end
Owner

myronmarston commented Oct 1, 2013

Thanks, @JonRowe, after hearing you explain that this definitely seems like the right direction. Left a few comments.

Owner

JonRowe commented Oct 1, 2013

@myronmarston I've addressed the issues commented about

@JonRowe JonRowe referenced this pull request in rspec/rspec-rails Oct 1, 2013

Closed

Run before(:all) hooks for rails magic example groups #829

Owner

myronmarston commented Oct 1, 2013

Looks good...seems like you are missing a few mutation methods, though (see above).

@myronmarston myronmarston commented on an outdated diff Oct 1, 2013

lib/rspec/core/metadata.rb
@@ -50,6 +50,18 @@ def self.build_hash_from(args)
hash
end
+ # @private
+ def on_change &block
+ @update_after_change = block
+ end
+
+ [:[]=, :store, :merge!].each do |name|
+ define_method(name) do |*args|
+ super(*args)
@myronmarston

myronmarston Oct 1, 2013

Owner

merge! accepts a block:

http://www.ruby-doc.org/core-2.0.0/Hash.html#method-i-merge-21

I think that it gets forwarded automatically via super, but it would be good to spec that.

Owner

JonRowe commented Oct 1, 2013

Looks good...seems like you are missing a few mutation methods, though (see above).

The other mutation methods only remove things from the hash, there's currently nothing to remove hooks when you remove metadata anyway. I left off the other mutation methods to make this more apparent. (See above)

@xaviershay xaviershay referenced this pull request Nov 2, 2013

Closed

Release 2.99 and 3.0.0 ALPHA #1136

13 of 13 tasks complete

@myronmarston myronmarston commented on an outdated diff Nov 2, 2013

spec/rspec/core/example_group_spec.rb
@@ -156,6 +156,19 @@ def ascending_numbers
expect(examples_run.count).to eq(1)
end
+ it "still runs matching config hooks when metadta is added later" do
@myronmarston

myronmarston Nov 2, 2013

Owner

s/metadta/metadata/

Member

xaviershay commented Nov 2, 2013

Rebased and added extra mutation methods. Note that overriding update doesn't work like the other ones (totally breaks), haven't figured out why yet.

Member

xaviershay commented Nov 2, 2013

oop, forgot about compat for old versions again :/

Member

xaviershay commented Nov 3, 2013

Given that this is not a new feature, and that Jon is away all weekend, I'm going to argue that this should not be a blocker for 3.0.0 pre. Any objections?

Owner

JonRowe commented Nov 4, 2013

Thanks for adding the extra mutation methods @xaviershay, any ideas on how to actually make this work in those situations? (e.g. actually remove now unnecessary hooks?)

Member

xaviershay commented Nov 5, 2013

Ah I see, I didn't understand the problem. Going to think about this after 3-pre. If you wanted to mark as private and merge without the extra mutation methods I'd be ok with that.

Owner

JonRowe commented Nov 6, 2013

WDYT @myronmarston? Merge without the mutation methods? Or at least with the mutation methods marked as private...?

Owner

myronmarston commented Nov 6, 2013

IIRC, this is fixing a bug in rspec-rails 2.14, right? Let's merge this in some form ASAP. I'm not sure what "with the mutation methods marks as private" means -- the metadata object is a hash, and the mutation methods are part of a hash's public interface. What do you mean by that?

Owner

JonRowe commented Nov 6, 2013

Following on from @xaviershay's suggestion, they're public for Hash we don't have to make them public for MetaData

Owner

myronmarston commented Nov 6, 2013

Following on from @xaviershay's suggestion, they're public for Hash we don't have to make them public for MetaData

Hmmm...I don't think we can do that in RSpec 2.x -- it would violate semver. We can do it in RSpec 3, but I'm wondering if there's a better approach we can do in 3.0? Can we change how rspec-rails does it's inclusion (maybe along the lines of rspec/rspec-rails#662) so that this is unnecessary?

Owner

JonRowe commented Nov 14, 2013

@myronmarston However we solve rspec-rails (I still think this is the easiest way) we should either fix this or make metadata immutable. As it stands it's confusing IMO.

@JonRowe JonRowe referenced this pull request Mar 2, 2014

Closed

Stop subclassing Hash #1231

Owner

myronmarston commented Mar 15, 2014

@JonRowe -- clearly this solution isn't going to work with the metadata hash just being a raw hash...we should figure out an alternate solution. Keeping open for now until we figure that out.

Owner

JonRowe commented Mar 15, 2014

I think we need an api to mutate the hash, that triggers the hook inclusion et al, then rspec-rails can call that instead

Owner

myronmarston commented Mar 15, 2014

I think we need an api to mutate the hash, that triggers the hook inclusion et al, then rspec-rails can call that instead

What kind of API do you have in mind? I'm having trouble coming up with a non-clunky API for that.

Here's an alternate idea I presented in #1231:

RSpec.configure do |rspec|
  rspec.define_inferred_metadata do |metadata|
    metadata[:type] = type_for(metadata[:file_path])
  end
end

Basically, rather than officially supporting all metadata features when you mutate metadata after the fact, provide an API to allow inferred metadata entries (which rspec-rails currently mutates the metadata to achieve), and rspec-core will ensure the inferred entries are set before it does anything with the metadata.

Owner

JonRowe commented Mar 15, 2014

Here's an alternate idea I presented in #1231:

👍

Owner

myronmarston commented Mar 20, 2014

@JonRowe -- whatever we come up with should address #969 as well.

Owner

myronmarston commented Apr 21, 2014

Closing in favor of #1496

@JonRowe JonRowe deleted the reload_hooks_on_metadata_change branch Apr 21, 2014

@myronmarston myronmarston added a commit that referenced this pull request Apr 22, 2014

@myronmarston myronmarston Add `config.define_derived_metadata`.
Fixes #969 and supercedes #1089.
ebf9c80

@myronmarston myronmarston added a commit that referenced this pull request Dec 17, 2014

@myronmarston myronmarston Remove spec for behavior we will no longer be supporting.
Fully supporting metadata mutation is fraught with difficulties
see (rspec/rspec-rails#829 and the attempted fix in #1089).
For RSpec 3, we decided to expose `define_derived_metadata`
as an officially supported way to accomplish the sorts of
things metadata mutation was used for in RSpec 2. This spec
got left behind (since it was passing at the time, it wasn't
noticed) but isn’t something we want to support going forward
since module inclusion shouldn't mutate metadata. It’s also
getting in the way of a perf optimization I'm implementing.
620269e

@myronmarston myronmarston added a commit that referenced this pull request Dec 18, 2014

@myronmarston myronmarston Remove spec for behavior we will no longer be supporting.
Fully supporting metadata mutation is fraught with difficulties
see (rspec/rspec-rails#829 and the attempted fix in #1089).
For RSpec 3, we decided to expose `define_derived_metadata`
as an officially supported way to accomplish the sorts of
things metadata mutation was used for in RSpec 2. This spec
got left behind (since it was passing at the time, it wasn't
noticed) but isn’t something we want to support going forward
since module inclusion shouldn't mutate metadata. It’s also
getting in the way of a perf optimization I'm implementing.
9062f3a

@myronmarston myronmarston added a commit that referenced this pull request Dec 19, 2014

@myronmarston myronmarston Remove spec for behavior we will no longer be supporting.
Fully supporting metadata mutation is fraught with difficulties
see (rspec/rspec-rails#829 and the attempted fix in #1089).
For RSpec 3, we decided to expose `define_derived_metadata`
as an officially supported way to accomplish the sorts of
things metadata mutation was used for in RSpec 2. This spec
got left behind (since it was passing at the time, it wasn't
noticed) but isn’t something we want to support going forward
since module inclusion shouldn't mutate metadata. It’s also
getting in the way of a perf optimization I'm implementing.
fa61873
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment