Update template #501

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
@sferik
Contributor

sferik commented Feb 6, 2012

Two changes to the generated .rspec file.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Feb 6, 2012

Member

I don't think random order should be the default. @myronmarston, @justinko, any thoughts on that?

Member

dchelimsky commented Feb 6, 2012

I don't think random order should be the default. @myronmarston, @justinko, any thoughts on that?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Feb 6, 2012

Member

I think that random ordering is an awesome feature that I plan to use on all new projects. It's far easier to prevent order-dependent bugs when you start a project with that turned on than when you go back later and add it--so I like the idea of it being a default.

My one suggestion here is that it is potentially confusing behavior for users that don't know about the randomization feature (and the --seed option), and it's a bit hidden in the .rspec file. Maybe it would be better to put it in the spec_helper.rb file with a comment explaining it? That gives it more visibility.

Member

myronmarston commented Feb 6, 2012

I think that random ordering is an awesome feature that I plan to use on all new projects. It's far easier to prevent order-dependent bugs when you start a project with that turned on than when you go back later and add it--so I like the idea of it being a default.

My one suggestion here is that it is potentially confusing behavior for users that don't know about the randomization feature (and the --seed option), and it's a bit hidden in the .rspec file. Maybe it would be better to put it in the spec_helper.rb file with a comment explaining it? That gives it more visibility.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Feb 6, 2012

Member

I would be OK with that. @sferik - wanna redo the patch so it does what @myronmarston said?

Member

dchelimsky commented Feb 6, 2012

I would be OK with that. @sferik - wanna redo the patch so it does what @myronmarston said?

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Feb 7, 2012

Contributor

@dchelimsky @myronmarston How does that look?

Contributor

sferik commented Feb 7, 2012

@dchelimsky @myronmarston How does that look?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Feb 7, 2012

Member

Looks great! The comments are really good.

Member

myronmarston commented Feb 7, 2012

Looks great! The comments are really good.

@justinko

This comment has been minimized.

Show comment
Hide comment
@justinko

justinko Feb 7, 2012

Contributor

I'm pretty sure minitest has random ordering as the default -- people seem to be okay with that.

Contributor

justinko commented Feb 7, 2012

I'm pretty sure minitest has random ordering as the default -- people seem to be okay with that.

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Feb 7, 2012

Member

As/is, this breaks cukes which depend on load order, so that's gotta get addressed before this gets merged.

Also, while I'm happy to change colour to color, the rationale statement in the commit message is not only entirely unnecessary, but it welcomes a useless debate about the issue, so I'm not going to include it. Feel free to submit a new pull w/o it, or I'll just cherry pick what I want from the pull.

Member

dchelimsky commented Feb 7, 2012

As/is, this breaks cukes which depend on load order, so that's gotta get addressed before this gets merged.

Also, while I'm happy to change colour to color, the rationale statement in the commit message is not only entirely unnecessary, but it welcomes a useless debate about the issue, so I'm not going to include it. Feel free to submit a new pull w/o it, or I'll just cherry pick what I want from the pull.

@sferik

This comment has been minimized.

Show comment
Hide comment
@sferik

sferik Feb 7, 2012

Contributor

I've reworded the commit message to remove the controversial part.

I think everyone can agree — prima facie — that the American spelling is superior, so a longwinded justification is not necessary. ;)

Contributor

sferik commented Feb 7, 2012

I've reworded the commit message to remove the controversial part.

I think everyone can agree — prima facie — that the American spelling is superior, so a longwinded justification is not necessary. ;)

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Feb 7, 2012

Member

@sferik agreed, thanks. That still leaves the problem that there are some cukes failing intermittently now because the examples within them depend on being run in order (e.g. https://github.com/rspec/rspec-rails/blob/master/features/model_specs/transactional_examples.feature#L58-85). Need to either find all those and specify --order default in each scenario, or implicitly generate a new .rspec file with same in each scenario. I think I prefer the former, as it's explicit, but it's going to be a lot of work.

Member

dchelimsky commented Feb 7, 2012

@sferik agreed, thanks. That still leaves the problem that there are some cukes failing intermittently now because the examples within them depend on being run in order (e.g. https://github.com/rspec/rspec-rails/blob/master/features/model_specs/transactional_examples.feature#L58-85). Need to either find all those and specify --order default in each scenario, or implicitly generate a new .rspec file with same in each scenario. I think I prefer the former, as it's explicit, but it's going to be a lot of work.

@ghost ghost assigned alindeman Jun 2, 2012

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jun 2, 2012

Contributor

Assigning to myself. I think I may have found all of the places default ordering is required for now.

Contributor

alindeman commented Jun 2, 2012

Assigning to myself. I think I may have found all of the places default ordering is required for now.

@alindeman alindeman closed this in 0ad477a Jun 2, 2012

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