Outputting seed before loading files creates reporter too early

[grk]

It looks like fb0f06e (merging #676) created a regression: by calling @configuration.reporter.notify before @configuration.load_spec_files (and thus memoizing the reporter too early), it breaks adding formatters in configuration block in spec_helper.rb.

A simple fix would be to add another reporter for this message, or revert this merge until a better solution is found.

I'd like to fix it, but maybe someone has a proper solution?

[dchelimsky]

@grk I think it should be part of the "run options" output, but that requires some refactoring. I actually started to do this but haven't gotten very far so you're welcome to give it a shot. Just comment here that you're working on it so we don't duplicate the effort.

[grk]

@dchelimsky that looks like a bit too much for me atm. I'll just keep an eye on this issue and monkeypatch what I need from head to a stable release :)

[grk]

Perhaps a quick workaround would be to just puts the seed instead of using the reporter?

[dchelimsky]

That's not a legit option because output can go to a file.

[myronmarston]

Three other possible solutions that come to mind:

• Have the seed printed to the reporter, but then allow RSpec.configure blocks to change the formatter. Problem is, a totally different formatter could be chosen, and it would be weird to use the "wrong" one for printing the seed.
• Don't print the seed at the start; instead, include the seed in the terminal output if an error occurs before the specs start running. Essentially, rescue errors when loading the spec files, and append a notice about the seed to the error message before re-raising it.
• Don't bother printing the seed at all (except for the existing print that happens as part of the end-of-run summary). Instead, write the seed out to .last_rspec_seed (or something similar) at the start of every spec run so the user has a way to always get the seed even if things blew up while loading.

Of these options, I think I like #2 the best. Thoughts from others? @mscottford, it'd be good to have you weigh in as well since you came up with the original change. Which of the above would meet your needs?

[dchelimsky]

I think this belongs in the run options, e.g.

Run options:
  order: random (seed 1234)
  include {:focus=>true}
  exclude {:ruby=>#<Proc:./spec/spec_helper.rb:109>}

[mscottford]

Of these options, I think I like #2 the best. Thoughts from others? @mscottford, it'd be good to have you weigh in as well since you came up with the original change. Which of the above would meet your needs?

1. I think that option 1 could break formatters that use a structured output, so that's probably not a good idea. (I'm thinking about junit xml format here, but I'm not sure about the inner workings of that formatter. It might not be an issue.)
2. I think that option 2 could work, but we'd have to make sure that any exception that's raised during loading will output the seed. I'm nervous that there would still be cases were strange order dependency errors, which random ordering is meant to prevent, will still cause failures without being able to tell the order that did so. Outputting the seed early skips that issue.
3. Option 3 would not work for my scenario, because these failures have been mostly happening on a hosted CI service with a transient file system.

Is there an option 4 that conditionally (controlled by a command line parameter) outputs the seed to STDERR before the run?

[mscottford]

I think this belongs in the run options:

Run options:
  order: random (seed 1234)
  include {:focus=>true}
  exclude {:ruby=>#<Proc:./spec/spec_helper.rb:109>}

@dchelimsky Sorry, I'm not familiar enough with the run options to see how that solves the problem. Do you mind being a little more pedantic?

[dchelimsky]

@mscottford it's actually orthogonal to the problem (may or may not fix it automatically), but however the issue is solved, there is a place to display run options and that should include order as well as filters. Does that make sense?

[mscottford]

@dchelimsky Got it. As long as the run options are displayed before files are loaded, then that's probably a better approach than what I originally did...

I'll go take a peek. [imagine that time has passed...]

I don't think that'll fix it all by itself. See: https://github.com/rspec/rspec-core/blob/master/lib/rspec/core/command_line.rb#L27

To solve the issue that I ran into, we would need to call @world.announce_filters before @configuration.load_files, but since World#announce_filters outputs via the reporter (See: https://github.com/rspec/rspec-core/blob/master/lib/rspec/core/world.rb#L72), we'll still have the issue that was reported here.

Thoughts?

[myronmarston]

@dchelimsky -- putting it in the run options output makes sense, but that's not going to address @mscottford's need. The run options are printed after all spec files have been loaded, so if an error occurs while loading the files, it will exit before printing the run options. The run options (as currently exists) can't be printed before loading the spec files because the output depends on what has been loaded; for example, it can print things like "All examples were filtered out".

@mscottford -- we talked a bit about this in your original PR, but I want to revisit it:

Also, did you see #660? That fixes the order that files are loaded (previously it was whatever order the OS returned the files in), and may solve the problems you were experiencing.

Thanks for pointing me to #660. That's not the problem we've been running into. We have some ordering dependency bugs, and we've turned on random order to assist with hunting them down. However, we seem to have one that's causing an exception to be thrown when the files are loaded, but we don't have a way to figure out what ordering caused this to happen. Thus this pull request.

In the current gem release of rspec, the order files get loaded is essentially unspecified (it delegates to the operating system's order). With the change in #660, the load order in the next minor release will be fixed at alphabetical order -- so if an error occurs while loading the files, it'll be consistently reproducible without knowing the random seed, in fact, the random seed won't have done anything yet at that point. Printing the random seed off at the start isn't going to help you troubleshoot.

Maybe I'm missing something, but I don't see how printing the random seed at the start will actually help you troubleshoot the sorts of load-time issues you mentioned when you opened the PR. In fact, printing the random seed at the start may actually be unhelpful because it suggests the randomization has done something at that point, and it hasn't.

So...I'm considering reverting the change from #660, but I want to make sure I'm not misunderstanding your problem first.

Regardless, I think printing the random seed in the run options (as @dchelimsky suggests) at the start of the spec run makes sense, but that can be a separate improvement entirely.

[mscottford]

In the current gem release of rspec, the order files get loaded is essentially unspecified

I was perhaps making an incorrect assumption. I thought the order the files were loaded was also randomized by that seed. I thought I had seen code to that affect, but I can't find it now...

[myronmarston]

The random seed is used for two things:

• To randomize the order example groups are run in
• To randomize the order examples and nested groups run in

The order the spec files are loaded is a separate thing.

[mscottford]

Okay. Then it looks like I solved a problem that did not exist. My apologies. In that case, please keep #660 around, or provide another way to control the file ordering.

[myronmarston]

I reverted the changes from #676 in 3301d30.

This should solve the problem reported by @grk, so I'm going to close this.