Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Example groups can override the default ordering #1025

Merged
merged 31 commits into from Oct 1, 2013

Conversation

@alindeman
Copy link
Contributor

alindeman commented Jul 28, 2013

So ... is this even a good solution and good way to design it? 馃槃

There's a decent bit of weirdness around how ordering works right now
that we're trying not to break. If you see some weirdness in our code,
feel free to ask about it ... and we might decide to simply break it for
RSpec 3 to allow the solution to be cleaner.

Still outstanding if so:

  • OrderingRegistry has some structural duplication between
    getting/setting ordering for examples vs. groups
  • We have not yet written rdoc/yard for the public interface for
    OrderingRegistry. Some of these methods might be @private.

@samphippen and I paired on this

@coveralls
Copy link

coveralls commented Jul 28, 2013

Coverage Status

Coverage increased (+0%) when pulling 7d3b260 on overriding-ordering-in-example-groups into a04fb17 on master.

true
end
end
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

Given ruby 1.9's well-known perf problems with require, I think it's better to not split things into such small files. What do you think about defining each of these ordering strategies in-line in the ordering_registry.rb file?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jul 31, 2013

Member

Why don't we require the file only when it's set as the strategy, I mean we're not going to need all of them at once right?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

Why don't we require the file only when it's set as the strategy, I mean we're not going to need all of them at once right?

I think that would complicate things in the registry, where it registers the strategy by class -- it would need to always load a couple of files to be able to register the built-in orderings. Seems simpler to just put them all in the same file as the registry. There's no rule that says we have to limit a file to one class :).

@coveralls
Copy link

coveralls commented Jul 31, 2013

Coverage Status

Coverage increased (+0%) when pulling 2845392 on overriding-ordering-in-example-groups into a04fb17 on master.


it 'resets random number generation' do
allow(Kernel).to receive(:srand)
expect(Kernel).to receive(:srand).with(no_args)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

Same here...

This comment has been minimized.

Copy link
@penelopezone

penelopezone Jul 31, 2013

Member

I feel like I'm missing some context here.... (Same here as what?)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

Weird, I posted a comment on the example above and github ate it or something.

My comment was: why use allow and expect? Seems like you could use just expect...

This comment has been minimized.

Copy link
@JonRowe

JonRowe Jul 31, 2013

Member

Yeah you don't need the allow on top of the expect

@myronmarston
Copy link
Member

myronmarston commented Jul 31, 2013

There's a decent bit of weirdness around how ordering works right now
that we're trying not to break. If you see some weirdness in our code,
feel free to ask about it ... and we might decide to simply break it for
RSpec 3 to allow the solution to be cleaner.

What weirdness are you referring to in particular? IMO, the main weirdness we had before was the use of an ordering module to extend onto arrays. You've gotten rid of that and I heartily approve :).

singleton_class.send(:define_method, :order) { args.last[:order] }
elsif parent && parent.order
singleton_class.send(:define_method, :order) { parent.order }
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

Rather than defining singleton methods like this, this feels like something that could be delegated to metadata (similar to described_class and file_path:

delegate_to_metadata :described_class, :file_path

Thoughts?

Also, the singleton_class.send(:define_method, :order) { parent.order } bit seems very odd to me; won't ruby's method dispatch cause it to use the parent class's implementation already, w/o a need to explicitly define it to delegate to the parent?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

Actually, perusing the code, I don't even see a place where this method gets used. Can you direct me to where this gets used? Maybe it's not even needed...

This comment has been minimized.

Copy link
@penelopezone

penelopezone Aug 1, 2013

Member

all this code should go it's a leftover from my original solution that somehow made its way into this PR.

@penelopezone
Copy link
Member

penelopezone commented Jul 31, 2013

The mismatch between groups and examples, also a single @order on configuration which is set depending on which of groups and examples had their ordering system set last.

@@ -405,9 +416,25 @@ def self.run(reporter)
end
end

# :order => MyStrategy.new
#
# :order => :my_stragegy

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

It's not clear what these :order => ... lines are supposed to mean, given that this method takes no args.

Also, s/stragegy/strategy/

This comment has been minimized.

Copy link
@JonRowe

JonRowe Aug 1, 2013

Member

Yeah I think this is in the wrong place

# @private
def self.ordering_strategy
strategy_name = metadata[:order]
RSpec.configuration.ordering_registry.resolve_example_ordering(strategy_name)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

If you do move some of this to Metadata, perhaps it could have an ordering_strategy method that looks up the strategy?

Also resolve_example_ordering seems like it could just be named #[] -- to me, the registry has similar semantics to a hash and I'd expect to be able to look up an ordering strategy using the square bracket method.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

I'm noticing that multiple things call ordering_strategy, causing the lookup to be done multiple times. Is there a way we could memoize it?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

Also resolve_example_ordering seems like it could just be named #[] -- to me, the registry has similar semantics to a hash and I'd expect to be able to look up an ordering strategy using the square bracket method.

I just realized you've named it as such because of the separate example and group ordering. A suggestion: consider using a separate OrderingRegistry instance for examples and a separate one for groups. To me, the example/group split fits better as a separate instances and not as part of the interface of the registry itself. If you move in that direction, then it would become natural to use #[] as the lookup method.

Thoughts?

@@ -0,0 +1,14 @@
module RSpec::Core::Ordering
class RandomOrdering
def order(items, configuration = RSpec.configuration)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Jul 31, 2013

Member

Having a second configuration arg seems kinda odd to me as an interface. It looks like you're just using it as a form of dependency injection for the tests, right? Part of what makes it seem weird is that it's leaked into all the other orderers where they don't use configuration at all.

Instead, what do you think about accepting the configuration in initialize (and having it default to RSpec.configuration in the same way)? To me, the config feels like a bit of state the instance should hold onto rather than being given each time order is called. It also means the other orderers wouldn't need to take the odd second argument.

Thoughts?

@myronmarston
Copy link
Member

myronmarston commented Jul 31, 2013

The mismatch between groups and examples, also a single @order on configuration which is set depending on which of groups and examples had their ordering system set last.

What is this in reply to, @samphippen? It's just floating there in the PR discussion view but I don't understand the context...

@penelopezone
Copy link
Member

penelopezone commented Aug 1, 2013

@myronmarston I know you're AFK but when you get back, my floating comment was in reply to:

There's a decent bit of weirdness around how ordering works right now
that we're trying not to break. If you see some weirdness in our code,
feel free to ask about it ... and we might decide to simply break it for
RSpec 3 to allow the solution to be cleaner.
What weirdness are you referring to in particular? IMO, the main weirdness we had before was the use of an ordering module to extend onto arrays. You've gotten rid of that and I heartily approve :).```
@myronmarston
Copy link
Member

myronmarston commented Sep 6, 2013

I'm going to try to rebase this and get this ready to merge, FWIW. I'll be force pushing the rebase shortly and then taking care of addressing the open issues above after that.

@coveralls
Copy link

coveralls commented Sep 7, 2013

Coverage Status

Coverage increased (+14.39%) when pulling f4acb57 on overriding-ordering-in-example-groups into 5f0e7a0 on master.

@coveralls
Copy link

coveralls commented Sep 7, 2013

Coverage Status

Coverage increased (+14.39%) when pulling ca4d4e8 on overriding-ordering-in-example-groups into 5f0e7a0 on master.

@myronmarston
Copy link
Member

myronmarston commented Sep 7, 2013

BTW, I have more changes coming for this. Just wanted to push what I had so far :).

@myronmarston
Copy link
Member

myronmarston commented Sep 9, 2013

I push some further updates. I expect the build to fail but wanted to push it to ensure it's backed up. I'll keep working on this this week.

@coveralls
Copy link

coveralls commented Sep 9, 2013

Coverage Status

Coverage increased (+14.39%) when pulling 560a5ea on overriding-ordering-in-example-groups into 5f0e7a0 on master.

@coveralls
Copy link

coveralls commented Sep 12, 2013

Coverage Status

Coverage increased (+14.39%) when pulling c59466b on overriding-ordering-in-example-groups into 5f0e7a0 on master.

@coveralls
Copy link

coveralls commented Sep 14, 2013

Coverage Status

Coverage decreased (-0.07%) when pulling 1f887ec on overriding-ordering-in-example-groups into 3900da7 on master.

@coveralls
Copy link

coveralls commented Sep 16, 2013

Coverage Status

Coverage increased (+14.42%) when pulling 727a0be on overriding-ordering-in-example-groups into 4c51980 on master.

@coveralls
Copy link

coveralls commented Sep 16, 2013

Coverage Status

Coverage decreased (-0.0%) when pulling d490c75 on overriding-ordering-in-example-groups into 4c51980 on master.

@coveralls
Copy link

coveralls commented Sep 16, 2013

Coverage Status

Coverage decreased (-0.04%) when pulling 9d4a982 on overriding-ordering-in-example-groups into 4c51980 on master.

@coveralls
Copy link

coveralls commented Sep 17, 2013

Coverage Status

Coverage increased (+0.02%) when pulling 954d16d on overriding-ordering-in-example-groups into 4c51980 on master.

@coveralls
Copy link

coveralls commented Sep 18, 2013

Coverage Status

Coverage increased (+14.42%) when pulling abdf355 on overriding-ordering-in-example-groups into 4c51980 on master.

@coveralls
Copy link

coveralls commented Sep 18, 2013

Coverage Status

Coverage increased (+14.42%) when pulling 8b87d9b on overriding-ordering-in-example-groups into 4c51980 on master.

@myronmarston
Copy link
Member

myronmarston commented Sep 18, 2013

I've gotten this to a point that's (almost) ready to merge and I'd like feedback from others. There's still a few open issues/questions I have, though:

  • The config API now has order_groups/register_group_ordering, order_examples/register_example_ordering and order_groups_and_examples/register_group_and_example_ordering. For each pair, the former one is the existing API from RSpec 2 that allows users to set custom global orderings. The latter is new and allows users to register named orderings that can be used with :order => <name> metadata. I'm concerned with the size of this API and would like to simplify it if we can but I'm not sure how. Also, if we do stick with this API, it's not totally clear what the difference between all these APIs are. Is there a simpler way to do it?
  • What additional cukes (if any) do we want?
  • I've removed some methods from the config API (like #order and #randomize?). We should deprecate these in 2.99 (plus anything else we rename as discussed above).

/cc @dchelimsky @alindeman @JonRowe @samphippen @soulcutter

@coveralls
Copy link

coveralls commented Sep 18, 2013

Coverage Status

Coverage increased (+14.42%) when pulling 76395cb on overriding-ordering-in-example-groups into 4c51980 on master.

@JonRowe
Copy link
Member

JonRowe commented Sep 22, 2013

  • The config API now has order_groups/register_group_ordering, order_examples/register_example_ordering and order_groups_and_examples/register_group_and_example_ordering. For each pair, the former one is the existing API from RSpec 2 that allows users to set custom global orderings. The latter is new and allows users to register named orderings that can be used with :order => <name> metadata. I'm concerned with the size of this API and would like to simplify it if we can but I'm not sure how. Also, if we do stick with this API, it's not totally clear what the difference between all these APIs are. Is there a simpler way to do it?

What is the difference, I'm not totally clued up on this. If there's no difference should we streamline them? Deprecate the old ones? Or will they be in widespread use?

  • What additional cukes (if any) do we want?

If we're allowing 'custom global orderings' then we should have a cuke outlining how that works (in addition to the one for using a standard ordering).

Edit Yup, we should cuke the custom block ordering, and possibly the identify one too?

module Ordering
# @private
# The default ordering (defined order).
class Identity

This comment has been minimized.

Copy link
@JonRowe

JonRowe Sep 22, 2013

Member

From the name of this it isn't clear to me how it's ordered.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Sep 22, 2013

Member

What's a better name then? I like Identity as it's a well-known mathematical concept for a function that returns the argument it is given...which is what this does.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Sep 22, 2013

Member

So it's the identity ordering, as in it leaves the thing in the same order. We were also thing about InPlace if you like that better. Thoughts on the back of a github comment 鉂わ笍

This comment has been minimized.

Copy link
@xaviershay

xaviershay Sep 23, 2013

Member

I like Identity for exactly what @myronmarston said. It's what I would have called this class.

This comment has been minimized.

Copy link
@JonRowe

JonRowe Sep 23, 2013

Member

My mathematical concepts must be rusty :P, I prefer @samphippen's InPlace suggestion.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Sep 23, 2013

Member

I like Identity much better than InPlace, so let's stick with that. Honestly, it doesn't matter that much since this is an internal class that users won't need to deal with directly.

@coveralls
Copy link

coveralls commented Sep 27, 2013

Coverage Status

Coverage increased (+14.48%) when pulling e7eaa57 on overriding-ordering-in-example-groups into 3091424 on master.

鈥ather than having special `set_global_ordering`
and `global_ordering` methods.

Also, simplify `Ordering::Registry#fetch`:

- No need to handle `nil`; the caller can pass
  :global.
- No need to handle callables; that's a bit of
  complexity we don't need. Callables can be
  registered.
- No need to convert the ordering name `to_sym`;
  it wasn't spec'd and I'd rather require folks
  pass symbols.
- Don't have separate `groups`, `examples` and
  `groups_and_examples` methods. There are very
  few use cases for having separate logic for
  groups vs examples and it doesn't justify the
  added complexity to have separate APIs. If you
  need to order them separately, put a conditional
  in your ordering strategy that branches based on
  the type of the first item in the list.
- Remove legacy `order_xyz` methods. These set the
  global ordering but didn't make that clear. Instead,
  the global ordering can be set by `register_ordering :global`.
`--default` was never a good name. (Consider that
if we had made `--random` the default, `--default`
would have been even more confusing).
@coveralls
Copy link

coveralls commented Sep 27, 2013

Coverage Status

Coverage increased (+14.48%) when pulling 57ab90a on overriding-ordering-in-example-groups into 3091424 on master.

@myronmarston
Copy link
Member

myronmarston commented Sep 30, 2013

Circling back around on this:

First, I want to 馃憤--order defined. I feel it's more obvious/expressive than default.

Done, in 57ab90a.

As a side thought, what about: ...

@shepmaster I like the idea of it accepting an ordering strategy object, so I added support for that in 69c12db. However, having separate example vs group methods is similar to what we already have and are trying to get away from--plus it seems inconsistent with the block ordering form. So I decided to make everything (examples and groups) go through one API (see 10aca90). Besides the fact that this radically simplifies the API and code in rspec-core, I think that @xaviershay is right that it's very rare that these will need to be treated differently. I'm also hoping that for the cases where people actually do need to call methods on the example groups or examples, it'll put positive design pressure on us to have a unified API for them so that for the user here they can effectively be treated uniformly. Keeping them separate does not put that kind of design pressure on us.

@xaviershay I went back and forth between making the API register_ordering(:default) or register_ordering(:global) but ultimately decided on :global for a few reasons:

  • default is a very overloaded term. There's the use in 2.x which is now called defined. There's the use we started using it on this PR (the ordering strategy that is used by groups that don't have explicit :order metadata). But then there's the concept of what RSpec's default for that strategy is. All these uses of "default" create confusion.
  • I need to add the appropriate deprecation warnings to 2.99. Given that 2.x supports --order default, I think it would be confusing to have a second concept for "default" in that release.

Thus, I thought that :global was a better choice. Thoughts?

Anyhow, here's what I think is left for this feature:

  • Add some additional cukes (as part of this PR -- I'll try to take care of that later today or this week).
  • Add deprecation warnings to 2.99 for the changed APIs (as a separate PR).
@xaviershay
Copy link
Member

xaviershay commented Sep 30, 2013

Agree re global.

@coveralls
Copy link

coveralls commented Oct 1, 2013

Coverage Status

Coverage increased (+14.48%) when pulling c5979da on overriding-ordering-in-example-groups into 3091424 on master.

@myronmarston
Copy link
Member

myronmarston commented Oct 1, 2013

OK, I think this is ready to merge. See my last commit for the added cukes. I opened #1092 to track the work to add the necessary deprecations to 2.99.

@myronmarston
Copy link
Member

myronmarston commented Oct 1, 2013

Anyone want to do a final code review before this is merged?


* Nested groups are always run from top-level to bottom-level in order to avoid
executing `before(:all)` and `after(:all)` hooks more than once, but the order
of groups at each level is randomized.

You can also specify a seed
You can also specify a seed:

This comment has been minimized.

Copy link
@JonRowe

JonRowe Oct 1, 2013

Member

Why the indentation change here?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 1, 2013

Member

I didn't mean to or realize I did. I'll fix that.

@@ -224,8 +205,8 @@ def treat_symbols_as_metadata_keys_with_true_values=(value)
add_setting :expecting_with_rspec
# @private
attr_accessor :filter_manager

attr_reader :backtrace_formatter
# @private

This comment has been minimized.

Copy link
@JonRowe

JonRowe Oct 1, 2013

Member

Why are we marking these as @private, is backtrace_formatter not a public configuration option?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 1, 2013

Member

backtrace_formatter is an internal object rspec-core uses for code organization that arose out of recent refactorings. There are other public configuration APIs off of the configuration object (which delegate to backtrace_formatter internally) but backtrace_formatter isn't meant to be used by end users. It's an implementation detail.

@@ -19,6 +19,10 @@ class ExampleGroup
include Pending
extend SharedExampleGroup

def self.order
nil
end

This comment has been minimized.

Copy link
@JonRowe

JonRowe Oct 1, 2013

Member

What's this for?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Oct 1, 2013

Member

No idea. It's from @alindeman's original commit:

a755aa6#diff-89a77987791b99dfc9d2e744919d0c42R22

Well, from when I rebased it. I'll try getting rid of it.

@JonRowe
Copy link
Member

JonRowe commented Oct 1, 2013

Looks mostly good to me, although I'd like to see a better solution for checking wether the seed is used rather than passing configuration into the reporter. Especially as I think we use Reporter in some of our other specs with no args.

@myronmarston
Copy link
Member

myronmarston commented Oct 1, 2013

Thanks for doing the review, @JonRowe.

Looks mostly good to me, although I'd like to see a better solution for checking wether the seed is used rather than passing configuration into the reporter. Especially as I think we use Reporter in some of our other specs with no args.

I agree the "seed_used?" solution could be better but I think there are more important things for me to work on. I'll merge this PR as-is once it's green (now that I addressed your other two comments) and you're welcome to open another PR with the improvements you have in mind there.

@JonRowe
Copy link
Member

JonRowe commented Oct 1, 2013

Yeah thats fair

@coveralls
Copy link

coveralls commented Oct 1, 2013

Coverage Status

Coverage decreased (-0.03%) when pulling df880f0 on overriding-ordering-in-example-groups into 3091424 on master.

myronmarston added a commit that referenced this pull request Oct 1, 2013
鈥ups

Example groups can override the default ordering
@myronmarston myronmarston merged commit 2ecfec8 into master Oct 1, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@myronmarston myronmarston deleted the overriding-ordering-in-example-groups branch Oct 1, 2013
@myronmarston
Copy link
Member

myronmarston commented Oct 1, 2013

Merged :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can鈥檛 perform that action at this time.