Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

Add additional rdoc example for ordering #2248

Merged
merged 1 commit into from
Jun 1, 2016
Merged

Add additional rdoc example for ordering #2248

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions lib/rspec/core/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1389,8 +1389,12 @@ def self.delegate_to_ordering_manager(*methods)

# @macro delegate_to_ordering_manager
#
# Sets the default global order and, if order is `'rand:<seed>'`, also
# sets the seed.
# Sets the default global ordering strategy. By default this can be one
# of `:defined`, `:random`, but is customizable through the
# `register_ordering` API. If order is set to `'rand:<seed>'`,
# the seed will also be set.
#
# @see #register_ordering
delegate_to_ordering_manager :order=

# @macro delegate_to_ordering_manager
Expand All @@ -1412,13 +1416,32 @@ def self.delegate_to_ordering_manager(*methods)
# end
# end
#
# describe MyClass, :order => :reverse do
# RSpec.describe 'MyClass', :order => :reverse do
# # ...
# end
#
# @note Pass the symbol `:global` to set the ordering strategy that
# will be used to order the top-level example groups and any example
# groups that do not have declared `:order` metadata.
#
# @example
# RSpec.configure do |rspec|
# rspec.register_ordering :global do |examples|
# acceptance, other = examples.partition do |example|
# example.metadata[:type] == :acceptance
Copy link
Contributor

@e2 e2 May 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually use 3 types: :unit, :acceptance and everything else. (The order would be: [:unit, everything else, :acceptance]).

Makes the order more explicit, it's also more practical and easier to adapt to real life scenarios. (And without losing on readability).

Ordering is a bit hard to "test" if you're a user, so the less tweaking/adapting an example needs in real life, the better.

It's also possible to botch something and accidentally "filter out" a chunk of the examples.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only an example however

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Internet there isn't much difference between "example" and code copied directly into production ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst that's sadly true, over complicating examples make it much more likely people won't use them at all and to be honest the most use of RSpec is for Rails for which neither example makes sense.

Copy link
Contributor

@e2 e2 May 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, though this is a special case.

First, Rails users likely won't use ordering anyway, except for random/seed. A "trivial" ordering example isn't practical. A practical example won't be trivial. Custom ordering in RSpec simply is a bit tricky right now.

To prove the point about complex ordering being non-trivial, consider this "simple example":

examples.partition { |example| example.metadata[:type] != :acceptance }.flatten(1)

It's one line, simple and readable yet ... not a good example.

Sometimes a longer and more elaborate example is much simpler to understand and use. I think this is exactly the case here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is for ordering examples, you are given examples and are free to order them as you wish. This PR is about improving the documentation of the fact you may order examples as you wish.

If you wish to expand upon the available ordering strategies may I suggest you work on an extension gem that adds whatever complex ordering you desire, if any of those show enough public adoption then we may consider bringing them into rspec-core, however for now please can you limit discussion on this topic to useful feedback about the documentation. I am not going to expand this example further at this time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If complexity is problem, the API has to change.

I don't agree. RSpec has never had the goal of providing APIs to make anything a user might want to do easy. That's fool's errand and we have neither the time nor desire to pursue that.

We do want to provide APIs that are sufficiently flexible that anything a user might want to do is possible without brittle monkey patches. That's what the ordering API is about: it provides the flexibility for users to order things with any arbitrary logic. How simple or complex the logic is is up to the user. You as a user can define the logic in a custom class using a <=> operator, and write your block to delegate to that if you prefer. I also don't think it makes sense to provide a CLI option to select a custom ordering strategy. The options supported from the CLI are intentionally limited. We don't want rspec --help to print off a long wall of text that makes it intimidating or hard for a user to find the options they care about.

Copy link
Contributor

@e2 e2 May 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR - Summary: I guess it makes more sense to just create an opinionated "default RSpec config" which can be included into projects. A custom "sorting strategy" rspec "plugin" alone just takes up too much configuring already.

RSpec has never had the goal of providing APIs to make anything a user might want to do easy. That's fool's errand and we have neither the time nor desire to pursue that.

I agree. It's just that in this case only, configuring the order seems more complex, brittle and exposes more internals than needed. If Ruby's sort was implemented this way it would only cause grief for everyone, especially maintainers. Letting me shoot myself in the foot is one thing, but expecting me to walk through a mine field to just sort an array ... that's the other extreme of "flexible".

It's so counter-intuitive to register a :global sorting strategy (whatever that is - ok, I know it means "example groups") and then remembering to set it, and then expecting me to remember the same sorting is used within an example group, and then expecting me to be careful with accidentally removing tests from the list and then expecting me to define my own class, somehow include it in RSpec, without the ability to simply share some state between calls (it would be different if I could pass a class instance instead).

It just gets hairier every minute. Just imagine all the code it would currently take for a plugin/gem to set this up automatically. I'm not saying this needs a whole api like custom formatters have. On the contrary - it shouldn't.

I understand the intention to preserve the randomizing seed as much as possible.

We do want to provide APIs that are sufficiently flexible that anything a user might want to do is possible without brittle monkey patches. That's what the ordering API is about: it provides the flexibility for users to order things with any arbitrary logic.

Yes! And the fact that @JonRowe made a mistake here is case-in-point. He's not a newbie by any stretch. On my first shot I accidentally left out the types of tests I was trying to separate and have running last. My only indication that I screwed up was: the last tests ran too fast.

I also don't think it makes sense to provide a CLI option to select a custom ordering strategy.

Without digging in the codebase, I'm assuming that I could right now specify --order global and expect my custom strategy to kick in.

I'm not complaining here or pushing you guys for any work - just sharing feedback and my impression.

All I really wanted to do was run spec/acceptance* before everything else. I just realized that having an --order reversed would've done it for me. Even if that's just a hack assuming acceptance is <= everything else. That would've been a single line of code instead of copying and pasting a large unmaintainable block of code between projects.

I just need 'reversed" alphabetical sorting for example groups and "defined order" for examples within a group (so I can just reorder by reordering methods in classes). I can leave order: random up to the CI. That's not an exotic or complex use case.

I just want to know if it's worth putting time into creating and maintaining a separate plugin ... or if this will be simplified into a <=> sort-like API (which would render my own plugin obsolete).

I understand all this now - thank you. Since I understand, I don't really need any changes here. Right now I'm just wondering how I could've made things easier for other RSpec users.

I'm just giving feedback. If my feedback isn't valuable, then I've wasted my time and yours - I'm sorry.

I've already wasted time digging into RSpec wondering why it was sorting files even if I set the order to :defined. As a user, I've already messed up ordering until I've figured out the internals.

If I can help others avoid wasting time without increasing the burden for you guys, that would be great.

Writing takes more time than reading, btw., so it's an investment too.

Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@e2 -- your feedback is very valuable and I thank you for it. But I think comments on a PR that is focused only on improving the documentation of the existing API is not the best place for it: API changes are out of scope for this PR (as it's just about documenting what's already there) and will result in these comments no longer being surfaced in an open issue or PR once this PR is merged, essentially causing your valuable comments to be "lost". (Not really lost, but no longer visible on an open thing).

In addition, engaging on these issues, and considering how it interacts with the rest of RSpec's features, takes a lot of time, and while I can't speak for @JonRowe, for me personally, my time working on RSpec is quite limited and I open a small PR like this one expecting it to breeze through code review and get merged without much back and forth. When instead a whole conversation is opened up that requires a lot of time to adequately respond to, well, that's time I often don't have and it can be a bit frustrating to see a small PR get side tracked on a larger discussion that's really not in scope for the PR. At the same time, I don't want to ignore valuable feedback like yours, so I make time to respond but it turns what was initially a quick drive-by contribution into a larger thing that I didn't really have time for this week.

Instead, if you opened an issue with this feedback, I think it would be much more productive for all involved. It wouldn't sidetrack this small PR, and I'd participate knowing it's a longer conversation that's going to take some time. It would also ensure this feedback is not "lost" when this PR is merged.

So I'd say please do continue giving your valuable feedback, but please try to find the most appropriate place for it (often an issue if it's not in scope for a specific PR).

For this specific case, before you open an issue, I'll provide you with a few thoughts of mine on your feedback.

  • We used to have a larger ordering API (with separate methods to order groups and examples) in RSpec 2. However, the consensus was that it was unnecessarily confusing and it was simpler to consolidate to one method. See Example groups can override the default ordering #1025: there are extended conversations there about many of the issues you've mentioned. Ordering api deprecations #1146 contains the deprecations of the old APIs (which should give you an idea of what the API used to be like).
  • It's not clear to me how we could support the <=> API like Ruby's sort does. I'd like to see a concrete proposal for how you would like the API to work differently.
  • You mention that you might create a plugin but that API improvements on our end could render your plugin obselete. It might actually make sense to first implement your ideas as a plugin, with the plan to later integrate it into RSpec (that's often how we've done things), although if you propose something that we're ready to support immediately and you're willing to do the implementation work we can probably bypass the plugin. It all depends on what API you have in mind, which I really have no idea about.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myronmarston - thanks! That really clears things up. I'm glad to know how to approach such topics without the side-tracking. I moved the discussion here.

Again, sorry for the noise.

# end
# other + acceptance
# end
# end
#
# RSpec.describe 'MyClass', :type => :acceptance do
# # will run last
# end
#
# RSpec.describe 'MyClass' do
# # will run first
# end
#
delegate_to_ordering_manager :register_ordering

# @private
Expand Down