Randomization is difficult to reproduce

[ToadJamb]

The implementation of randomizing tests is such that locking down other randomizations is difficult (i.e. shuffling, calling rand, etc. do not yield the same results for a given seed).

The only way I've found to do it is to add something like:

RSpec.configure do |config|
  config.before(:each) do
    srand RSpec.configuration.seed
  end
end

This is not ideal because now the randomization is reset before each and every test, making the randomizations over the course of an entire suite much less... 'random'.

There are two potential solutions:

Do not use srand for randomization. Do something like:

def order(items)
  @used = true
  strategy = Random.new(@configuration.seed)
  items.shuffle(random: strategy)
end

Use srand, but let RSpec's seed be used for everything after that. Like so:

def order(items)
  @used = true
  Kernel.srand @configuration.seed
  ordering = items.shuffle
  #Kernel.srand # reset random generation - Remove this line.
  ordering
end 

The first option still requires the consumer to write code to call srand RSpec.configuration.seed, but it can be done once in a support test file or before(:suite), or before(:all).

I prefer the second option, as it means that I do nothing as an RSpec consumer in order to get repeatable randomization (I swear RSpec worked this way once upon a time). Keep in mind that this can be 'turned off' the same way it is 'turned on', so it simply requires calling srand with no arguments.

I am working on a patch to fix this, but would like feedback on the preferred approach.

If the second choice is preferrable, it is probably worth taking the time to ensure that it only gets set once. It appears that the ordering code gets called multiple times - based on sample test suites that I used to play with this. Ideally, srand would only be called once with the seed for that run.

[myronmarston]

I haven't had a problem with this, and in my experience shuffle produces the same results for a given seed. Can you show us an example of what's not working as you expect?

[ToadJamb]

This repo contains a sample that consistently fails the included Cucumber feature for me. The feature records the output from the first RSpec run and checks it against a second run.

https://bitbucket.org/ToadJamb/ruby-rspec-randomization-example

Assuming the Cucumber features fail for others, there is a third option that I like better than either of the two I mentioned above. It would be to implement the first option above, but provide a seed regardless so that randomization can be reproduced for a given test run (regardless of whether the specs are run in a random order). This is a potential problem regardless of the ordering strategy.

[JonRowe]

@myronmarston he's talking about randomisation in his code, not in our code. E.g. We are not locking down the randomisation across the board, which would seem legit.

[myronmarston]

@ToadJamb -- I'm not a mercurial user (I've never even installed it) and don't have any desire to spend the time learning now. I tried git-remote-hg to allow me to clone your repo using git, but it gave me an error. Can you push that to a git repository so I can clone it? Thanks!

[ToadJamb]

Sure. Try this: https://github.com/ToadJamb/ruby-rspec-randomization-example

[JonRowe]

@myronmarston FYI I downloaded the repo (via a zip from bitbucket) and it does randomise differently, but this isn't internal RSpec code, is our seed supposed to leak out into a developers implementation?

[ToadJamb]

You're right. This isn't internal RSpec code.

But there are two approaches RSpec can take to randomization.

1. The framework can help the developer reproduce errors due to randomization by seeding randomization for the course of a test run.
2. It can take a completely hands-off approach and leave randomization seeding/reproduction to the developer.

Right now, RSpec is attempting to take a hands-off approach, but isn't. The call to srand in RSpec::Core::Ordering::Random#order attempts to reset randomization, which a developer may have already set in an attempt to reproduce the results of a previous run. If a developer calls srand RSpec.configuration.seed from a file in spec/support or at the end of spec_helper, this seeding gets clobbered by RSpec's call to srand. The fix for this is trivial, so there is no reason to not address this (at the minimum).

Ideally, I would like to see RSpec pursue the first option, as I think it is the most useful to developers. I didn't realize that I needed to handle seeding randomization myself until my failures were not reproduced when passing in the seed from the failed test run. I think most people would fall into this category and it's rather frustrating to find out at a point that is too late to help.

[JonRowe]

The support for randomisation that exists at the moment is for ordering our specs in a reproducible random order only. AFAIK the code is not attempting to support your own randomisation. Note that our call to srand occurs before your code is executed.

[ToadJamb]

"Note that our call to srand occurs before your code is executed."

In the case that I have rspec order my tests randomly, this is not true at all. Adding srand RSpec.configuration.seed at the end of spec_helper or in a file in spec/support will not result in predictable results with a given seed. With random ordering turned on, the only thing that works is to put that call in a before(:each), which is far from ideal.

It would be trivial (and seems quite obviously better) to do this:

def order(items)
  @used = true
  strategy = Random.new(@configuration.seed)
  items.shuffle(random: strategy)
end

instead of

def order(items)
  @used = true
  Kernel.srand @configuration.seed
  ordering = items.shuffle
  Kernel.srand # reset random generation
  ordering
end

The most obvious thing here is that you are making a global call to srand that is absolutely not necessary and is having side effects that are clearly not intended. Is there a reason not to fix this? Is there a problem with the code I'm suggesting?

For the record, I'm more than willing to write the tests and submit a patch.

[JonRowe]

Ruby 1.8.7 doesn't support your suggestion.

[JonRowe]

However I'm still not sure what the problem is. We aren't preventing you from reseeding for your own needs, we are not trying to make your randomisation predictable, only our suite ordering.

[JonRowe]

If you want predictable ordering of course you have to reset the seed to a known value before each test, how else would it work.

[ToadJamb]

I wasn't sure if you were still supporting 1.8.7. If so, then that is certainly a fair argument against my suggestion.

At the least, I would like to be able to set it in a before(:suite) or before(:all), but even better would be to not have to worry about it at all and have RSpec seed randomization with the seed it generates.

[JonRowe]

I wasn't sure if you were still supporting 1.8.7.

As a testing tool we kind of have to, we can't force people to upgrade and there are a lot of gems which support 1.8.7 with test suites written in RSpec.

At the least, I would like to be able to set it in a before(:suite) or before(:all), but even better would be to not have to worry about it at all and have RSpec seed randomization with the seed it generates.

We are not trying to enable predictable randomisation for your code. If you want two examples to display the same psuedorandom pattern then the seed must be reset before each example, there is no other alternative. I am heavily against that, it's an overhead most users won't need, you can easily do it yourself.

If you want two examples to display different randomisation then you can leave it as is.

[ToadJamb]

While I don't entirely agree, I certainly respect your thoughts. My final argument for RSpec seeding randomization goes something like this...

NOT setting a seed for randomization violates the principle of least surprise. Most developers won't care about how randomization is seeded until there is a test failure/error due to randomization. At that point, it will be too late for them to reproduce the failure/error.

This is exactly what happened to me - except that I do care and made the incorrect assumption that RSpec was handling this for me.

[myronmarston]

@ToadJamb -- First off, thanks for the very clear code example that shows the issue!

@JonRowe is right that RSpec's --seed option was not intended to lock down other things that use randomization. However, I have wished that it would on multiple occasions, but just haven't taken the time to do anything about it.

I agree that RSpec's current way of managing the seed actively gets in the way of users because it's basically impossible for users to set the random seed once and have it "stick" since RSpec resets it. So yes, at a minimum we should fix that, and more than that, I think I'd like to see --seed locking down other randomizations. One can imagine using a library like faker to generate random test fixture data, and having a weird interaction between the order the tests run and the data that generates such that you'd need both to be locked down to a consistent seed to repro and solve the issue.

@ToadJamb -- want to take a stab at fixing this?

[ToadJamb]

@myronmarston I'll be happy to take a shot at this.

[ToadJamb]

I prefer to know that I'm not breaking existing/expected functionality before I go hacking around in code. To that end, I was looking for existing Cucumber features around ordering and seeding but didn't find any, so I started writing some. What is the best way to discuss this code?

I'm not opposed to submitting a pull request, but I'd just like to have a discussion around what I'm doing to make sure it's ok. I'm not looking to have it pulled in just yet. Ideally, I would like to be able to do this as I go along, so I don't spend time going too far down paths that don't make sense.

[JonRowe]

There are ones surrounding ordering.

[myronmarston]

https://github.com/rspec/rspec-core/blob/master/spec/command_line/order_spec.rb

[ToadJamb]

Sweet. I mistakenly assumed those specs would be in Cucumber.

Is the behavior of rspec --order defined --seed 123 defined?

It looks like doing that currently throws the ordering into random, but I think it makes sense (with this change) that it would leave the specs unordered and only use the given seed for seeding randomization.

[ToadJamb]

I prefer to take things in small, iterative steps. Here is my outline for this issue (each item will ultimately be a pull request):

1. Decouple ordering and seeding (i.e. just because I specify a seed doesn't mean I want to run my specs in a random order, maybe I just need to reproduce randomness, which will not be reproduced if my tests are now running in a completely different order). This pull request has already been sent. :)
2. Always display the seed. This is to support seeding randomness when using an ordering strategy other than random. You can't pass the seed in to the next run if you don't know what it was on the current run.
3. Seed randomization.
4. Move the randomization seeding and spec ordering (since these need to be tightly coupled). The first call to Random#order should be made prior to all before hooks and the last call should be made after all after hooks.

[myronmarston]

rspec --order defined --seed 123

FWIW, prior to now, --seed 123 implied "--order random". As a result, currently, rspec --order defined --seed 123 runs specs in random order but rspec --seed 123 --order defined runs specs in defined order.

However, I think with the direction we're talking about taking this, decoupling them makes sense.

[myronmarston]

Decouple ordering and seeding (i.e. just because I specify a seed doesn't mean I want to run my specs in a random order, maybe I just need to reproduce randomness, which will not be reproduced if my tests are now running in a completely different order). This pull request has already been sent. :)

This sounds fine, but I'd rather just get one PR for the entire thing. IMO, this change as as whole makes sense, but doing this first step w/o the rest doesn't make much sense: as long as long as the seed is only usable for RSpec's random orderer, having --seed not set order would be odd. We're trying to get ready to release the first RSpec 3 alpha later in October, and I don't want only part of this to get merged. Thus, doing it all in one PR is important.

Always display the seed. This is to support seeding randomness when using an ordering strategy other than random. You can't pass the seed in to the next run if you don't know what it was on the current run.

👍

That also takes care of the complexity we've discussed here :).

Seed randomization.

Huh? The seed is already randomized. I don't follow...

Move the randomization seeding and spec ordering (since these need to be tightly coupled). The first call to Random#order should be made prior to all before hooks and the last call should be made after all after hooks.

Not sure I follow this, either. Why is doing it in that order important?

[myronmarston]

On a side note, if you want to use the Random class from 1.9.2 to help with this (e.g. to have an isolated random number generator used by rspec) that's fine -- we'll just have to pull in the backports version of it for 1.8.7. As @JonRowe said, we're still supporting 1.8.7 (primarily so that we're not forcing gem authors to choose between dropping support for 1.8.7 before they are ready or not upgrading to RSpec 3), but 1.8.7 is definitely legacy at this point, so we'll definitely be dropping support for it. If using the Random class makes the implementation easier or more robust then I don't want us not to use just because we support 1.8.7, given that there is a way to backport Random to that version.

[ToadJamb]

This sounds fine, but I'd rather just get one PR for the entire thing.

Agreed. My primary goal with that was to get feedback as early as possible, but you're right, merging those in individually doesn't really make sense.

if you want to use the Random class from 1.9.2 to help with this (e.g. to have an isolated random number generator used by rspec) that's fine

Awesome! I was attempting to find a solution that would not require new dependencies, but bringing in backports would make things a lot easier.

So now, I'm thinking of the work like this:

1. Decouple ordering and seeding as command line parameters. (There is already a commit for this).

2. Always display the seed.

3. Decouple ordering and seeding functionality.

   Right now, resetting the seed has a potential impact beyond RSpec. Use backports and Kernel::Random to prevent this leakage. Randomization of spec ordering stays where it is in the order of operations.

4. Seed randomization.

   While it's true that a seed is generated, this seed is not actually used to seed randomization - or rather - it is, but ONLY in the case of randomly ordered examples and even then, it is reset to use an unknown seed after the examples are shuffled.

   I am also proposing that we seed randomization prior to all before hooks, and even better would be before spec_helper and/or any support files.

   Ideally, if an application developer chooses to put srand 123 in any before hook, I think it should override the seed that RSpec sets for them. Even better would be if srand 123 in a support file or spec_helper.rb overrides the seed that RSpec sets. With random spec ordering tightly coupled with seeding (i.e. without backports), allowing overrides in spec_helper.rb would not be possible, but using backports allows us to consider this option, which I think makes the most sense, assuming there is not anything else in the way. (I was a bit surprised when putting srand 123 in my spec_helper.rb got overridden when I first started looking into this).

[myronmarston]

Awesome! I was attempting to find a solution that would not require new dependencies, but bringing in backports would make things a lot easier.

I don't want to add backports as a dependency for just the random stuff. Instead, we'd probably port its code over to rspec, like so:

module RSpec
  module Core
    if defined?(::Random)
      Random = ::Random
    else
      require 'rspec/core/random' # which defines RSpec::Core::Random based on backports version
    end
  end
end

The reason for this is that besides the added dependency, we don't want RSpec to add a 1.9 stdlib class to the top level namespace. Reason being, a user testing a gem on 1.8.7 and 1.9 could use Random w/o realizing it's not available on 1.8.7, and if RSpec used backports (which defines a top-level Random), the user's specs would pass, even though the gem is broken on 1.8.7. Thus, we want to namespace it under RSpec::Core, so that we can use it w/o affecting the top level namespace on 1.8.7.

[ToadJamb]

we don't want RSpec to add a 1.9 stdlib class to the top level namespace

That makes sense. I just hadn't taken the time to process the implications.

It may be worth noting that Array#shuffle doesn't accept a RNG until ruby 1.9.3. Given the concern around compatibility, I think the best option would be to write a simple algorithm for ordering instead of monkey patching Array#shuffle (as the method is available in 1.8.7, but doesn't accept params until 1.9.3). Backports has a ruby implementation of Array#shuffle that we could take inspiration from.

[myronmarston]

Closing since this was addressed by #1120.