Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

merge with rspec-repeat? #46

Open
michaelglass opened this issue Oct 5, 2015 · 9 comments
Open

merge with rspec-repeat? #46

michaelglass opened this issue Oct 5, 2015 · 9 comments

Comments

@michaelglass
Copy link
Contributor

@dwbutler @rstacruz

thoughts on merging with rspec-repeat? I think rspec-retry has better SEO / name recognition but rspec-repeat has a cleaner codebase. From that perspective: Would be happy to either

  • bring on @rstacruz as a maintainer and release rspec-repeat as the next big-version-number version of rspec-retry.
    or
  • deprecate rspec-retry and point people towards rspec-repeat in the readme.

My requirements would be

  • ensure feature parity (which might be done)
  • ensure adequate test coverage
  • ensure open issues are migrated to the appropriate repository

thoughts, you two?

@rstacruz
Copy link

rstacruz commented Oct 5, 2015

i'm all for it, but have you also considered:

a) let them exist side by side?
b) deprecate rspec-repeat instead?
c) making rpsec-repeat a dependency of rspec-retry?

@michaelglass
Copy link
Contributor Author

I think they're so similar that maybe deprecating one is better than keeping them side by side. I think your code base is better than ours (minus our much more comprehensive test suite). We could certainly use rspec-repeat as a dependency, however I think once we pull out the rspec-retry guts and replace them with rspec-retry, most of what we'd have is a test suite.

@rstacruz
Copy link

rstacruz commented Oct 5, 2015

I'm happy with rspec-retry absorbing rspec-repeat in whatever way that probably means, even if it's replacing all of lib/.

I just request that you make repeating all of type: features still possible somehow — it's the primary use case of my team (and with people who's written to me about rspec-repeat) :)

@michaelglass
Copy link
Contributor Author

@dwbutler thoughts on this? Then will move forward / give you access

@dwbutler
Copy link
Collaborator

dwbutler commented Oct 6, 2015

@michaelglass I like @rstacruz's implementation of rspec-repeat. I would be in favor of giving him contributor access so he can merge in his changes in favor of mine.

The only difference is that rspec-repeat is opt-in - you have to set up an around hook in order to get any retry behavior. Whereas, rspec-retry automatically sets up the around hook and relies on configuration. We'll have to find a way to resolve the behavior difference, or document it.

The right approach might be to do a major version bump, move ahead with breaking changes, and document them.

@michaelglass
Copy link
Contributor Author

@dwbutler thanks so much! @rstacruz I'm going to give you contributor access who's going to do the work WRT pulling rescue-retry guts?

@michaelglass
Copy link
Contributor Author

would like to stay syntax-compatible (still with a major version bump) but I think we can have rspec-repeat do the heavy lifting

@rstacruz
Copy link

rstacruz commented Oct 6, 2015

How about having both available — the repeat method, and the retry: 3 metadata?

That would be approximately:

config.around :each, :retry do |ex|
  RSpec::Repeat.repeat ex.metadata[:retry], ex.metadata
end
it 'eventually works', retry: 3, verbose: true do
  expect(rand(2)).to eq 0
end

also: gotta decide on method names at some point (retry as a method name is reserved)

@michaelglass
Copy link
Contributor Author

yeah exactly

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

No branches or pull requests

3 participants