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

Make glob order alphabetical, not file-system dependent #660

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
Contributor

joliss commented Aug 13, 2012

Dir[] and FileList[] have undefined order, so sorting them is strictly
better than not. Sorting should not have a performance impact in any
realistic use case: Sorting 100k shuffled elements is
near-instantaneous, and sorting 1M elements takes ~3 seconds, so file
system access time will always dominate.

One particular reason why this is useful is that undefined ordering is a
major source of flickering test suite failures.

Make glob order alphabetical, not file-system dependent
Dir[] and FileList[] have undefined order, so sorting them is strictly
better than not. Sorting should not have a performance impact in any
realistic use case: Sorting 100k shuffled elements is
near-instantaneous, and sorting 1M elements takes ~3 seconds, so file
system access time will always dominate.

One particular reason why this is useful is that undefined ordering is a
major source of flickering test suite failures.

This pull request passes (merged 6441c85 into 07be957).

Owner

myronmarston commented Aug 13, 2012

I'm on the fence about this. I can see that this helps make things more consistent, but if your test suite fails intermittently based on the order the spec files get loaded then I think this is just a bandaid that covers over a deeper problem. Since RSpec 2.8 there have been options to control the ordering RSpec runs your examples and example groups; I think it'd be cool to apply those same options here so that you can control the load order of spec files in the same way. For folks who want to just use alphabetic ordering they can, and for folks who want files loaded in a random order (with a printed seed that enables you to reproduce when it exposes an issue), they can do that as well.

Would that meet your needs? Want to take a stab at it?

Contributor

alindeman commented Aug 17, 2012

+1 for a config.order = "alphabetical" or similar.

Owner

myronmarston commented Aug 17, 2012

@alindeman -- there's already config options for ordering stuff. I'd rather just re-use the existing options than adding new ones.

That said, there's a bit of a chicken-and-egg problem here: if you specify the config option in your RSpec.configure block, the block won't be run until a spec file is loaded and either configures rspec or requires spec_helper which configures rspec--but we want the config option to determine which spec file gets loaded first.

So I think the existing config option would only affect this when it is specified in .rspec or as a CLI option.

Contributor

alindeman commented Aug 17, 2012

Makes sense. I should have said --order.

Contributor

joliss commented Aug 17, 2012

I'm on the fence about this. I can see that this helps make things more consistent, but if your test suite fails intermittently based on the order the spec files get loaded then I think this is just a bandaid that covers over a deeper problem.

To clarify, I'm not asking for RSpec to make any guarantees about the ordering. If my tests are order-dependent, I should fix that, rather than forcing an order as a bandaid.

This patch solves a slightly different problem: If some order-dependency has snuck in, one of three things might happen:

  1. Things fail and I get alerted.
  2. Things pass silently.
  3. I get a flickering test on my build server.

Of these options, a flickering test is by far the worst. I'd rather have my tests pass silently, and deal with the order dependency when it comes up, than spend an hour tracking down a flickering failure.

In fact, in larger projects my experience is that flickering tests can easily become the biggest time sink in the entire test suite.

A major source of flickeringness seems to be the system-specific globbing order. So this patch avoids it by making the order consistent.

Again, I'm not saying that RSpec should make any promises about "alphabetical" test ordering, and I don't think an --order alphabetical option is even needed. If we ever want to change the order in which we collect spec files, we can still do so at any time.

Owner

myronmarston commented Aug 17, 2012

I've frequently seen issues with tests failing due to running in a different order (e.g. because one test sets up some persisted state another depends on, for example), but I can't think of a time I've seen a flickering tests due to the spec files loading in a different order. Can you explain one of these cases? I'd just like to understand more about the underlying problem you're trying to solve.

Anyhow, after thinking about this some more, I think it makes sense to make the file-load order consistent by default (similar to how the specs are run in a consistent order by default), and support loading the files in a random (but seeded) order if --order rand is passed. The randomly ordered loading can be added later, as you point out.

Can you add some specs for this? If we're going to make consistent load order a feature of rspec then it should be spec'd as such.

Contributor

joliss commented Aug 23, 2012

I've frequently seen issues with tests failing due to running in a different order (e.g. because one test sets up some persisted state another depends on, for example), but I can't think of a time I've seen a flickering tests due to the spec files loading in a different order. Can you explain one of these cases?

Hm, the different test order comes from the different file load order, no? Just to make sure we're talking about the same thing: When I say "flickering", I mean that to include "it works reliably on my machine but fails reliably on another machine".

Circle CI have these file ordering issues at the top of their FAQ, so I assume they're common for other people too: https://circleci.com/docs/common-problems

Can you add some specs for this?

I'm not sure what a meaningful spec would look like that would not itself be prone to flickering failures or use excessive mocking. I'd prefer going without a spec.

If we're going to make consistent load order a feature of rspec then it should be spec'd as such.

Well, we're not promising any specific load order, are we?

I basically just follow the rule that whenever I have Dir[], I add a .sort. It's not really something I'd want to test - similar to how you have security best practices (e.g. escape stuff before you dump it into HTML) that you don't necessarily test for.

Owner

dchelimsky commented Aug 24, 2012

I'm generally in favor of @joliss's approach to just sorting and not making it an API, however I would prefer to have this tested than not to prevent future regressions.

myronmarston added a commit that referenced this pull request Sep 8, 2012

myronmarston added a commit that referenced this pull request Sep 8, 2012

Owner

myronmarston commented Sep 8, 2012

I merged it manually and added specs:

279a6d6

Contributor

joliss commented Sep 13, 2012

Thanks for adding specs, Myron. Those are much nicer than what I would've come up with!

myronmarston added a commit that referenced this pull request Oct 3, 2012

Don't output the seed before loading spec files.
This sets the formatter before users have a chance to
configure it in a `RSpec.configure` block in a loaded
spec file, and it turns out we don't need it--#660
fixes the order files are loaded to a consistent order,
so the seed isn't involved at file load time.

This reverts the following commits:

* "Changelog for #676."
  (824119e)
* "Ensures that error commands always throw an error"
  (dbee8b6)
* "Outputs random seed before loading files"
  (f6565c7)

Closes #676.

I disagree with the statement that Dir and FileList have unspecified order.

# The example below has a specific order:
FileList["spec/unit/**/stubs/*_spec.rb", "spec/unit/*/*_spec.rb"]  

If I specify an order via pattern then I except the the spec files to be run in that order. Sorting messes this up.

I have a special case where I redefine a class. For accurate code coverage I need to have specific order of my spec files. In earlier versions of Rspec I just set the pattern and I am good. However with this merge I no longer have control over the order.

Contributor

shepmaster commented Jan 4, 2013

Here is my use case that doesn't fit well with this change.

I have a fun test-ordering bug that is triggered with a particular random seed. However, I do not know which preceding test is interacting poorly to cause my failure. To figure it out, I'd like to run all the tests in a user-specified order. I could then binary-chip the initial set of tests to find the one that is causing me grief.

An example may help me explain:

I run my tests in random order, and it picks test files A B C D E F. There is a order-related failure in E. I would like to then run A B C D E (fails), then A B E (no failure), then C D E (fails), then C E (fails). At this point I start commenting out tests inside of C to figure out which of them (or whatever) is causing the problem.

With this change, RSpec will always run E first if its name is "aaa_spec.rb". This means I am unable to force an order of files to debug a particular ordering.

Owner

myronmarston commented Jan 4, 2013

I disagree with the statement that Dir and FileList have unspecified order.

Sure, you can define an ordered listing of patterns, but within those patterns, the order of the matched files themselves is operating system dependent.

With this change, RSpec will always run E first if its name is "aaa_spec.rb". This means I am unable to force an order of files to debug a particular ordering.

Maybe I'm misunderstanding what you're saying, but I don't think that's true. This change just affects the order RSpec loads spec files, not the order specs are run in.

In fact, this change (along with d91410f) greatly improved things in regards to doing the kind of order-dependent troubleshooting you're referring to, at least in my experience. Before these changes, if you ran a subset of the examples by passing one or more paths to the rspec command, and passed the same seed as was used in the prior full-suite run, it was not guaranteed that the subset would run in the same order that that subset ran in within the full suite run. It depended upon the you passing the file paths to the rspec command in the same order as the files were loaded for the full suite run, which depends on your OS, and on some OSs is not really specified (i.e. on linux I have no idea what the order is).

Besides, there's now a well-defined public API for ordering your examples and example based on any criteria you want...so you can explicitly run things in the exact order you want now with that API, rather than relying upon the order the files are loaded in (which should be considered a separate thing from the order the examples are run in).

@shadabahmed -- Would an ordering strategy work to solve your problem as well?

@myronmarston Yes an option to set sorting strategy would be better than forcing a sort even when not required

Owner

myronmarston commented Jan 22, 2013

@myronmarston Yes an option to set sorting strategy would be better than forcing a sort even when not required

RSpec already supports a way to set an ordering strategy, which determines the order the example groups and examples are run in:

http://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration:order_groups_and_examples

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment