search

wspurgin / rspec-sidekiq

RSpec for Sidekiq
https://github.com/wspurgin/rspec-sidekiq
Other
663 stars 132 forks source link

Adding support for latest ruby and Rails 5 #156

Closed coding-red-panda closed 4 years ago

coding-red-panda commented 6 years ago

This pull-request adds support for the following:

Updated the matrix to run all combinations for maximum support, and removed a deprecation warning as well as invalid RSpec usage pertaining raise_error.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.8%) to 97.772% when pulling 23acb46f2bd81c06992e787301702d9846485c8a on coding-bunny:upgrade into 12135312a7868353a51586bf97489dd2c3f0846d on philostler:develop.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+18.3%) to 97.778% when pulling 89d837a8066aead14a8d4946f34dff7a85fb88d3 on coding-bunny:upgrade into 2cd15b0fe2b172243e8002c4aefa39696c42d52f on philostler:develop.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.8%) to 97.772% when pulling 23acb46f2bd81c06992e787301702d9846485c8a on coding-bunny:upgrade into 12135312a7868353a51586bf97489dd2c3f0846d on philostler:develop.

coding-red-panda commented 6 years ago

@packrat386 or @wpolicarpo is this project still being maintained?

packrat386 commented 6 years ago

It certainly isn't deprecated or anything. I can't guarantee how often I have time to look through PRs/Issues but I do periodically.

coding-red-panda commented 4 years ago

Wondering if this can be merged down? Been quite some time since I made this pull-request.

coding-red-panda commented 4 years ago

I've implemented the changes, but now I have specs failing because of GlobalID. The matcher ones only actually, and I'm not sure how to get these fixed :/

cgunther commented 4 years ago

Part of the GlobalID failure is due to: https://github.com/rails/globalid/commit/8b3af4c39977812d782319e6d2ad310f96819425

To fix that, I think you'd want to remove attr_reader :global_id and the initialize method in the TestResource class, then memoize the result of the id method (ie @id ||= rand(36**10).to_s 36 ) so we get a consistent result on calls to the same object.

Furthermore, when you assign GlobalID.app the second time after all the jobs are enqueued, that makes it so the app identifier we expect to be in the queue doesn't match the app identifier that's actually in the queue, so I think this line is unnecessary: https://github.com/philostler/rspec-sidekiq/pull/156/files#diff-4b213564ae539e591aeb48cb440f1c77R20

I also think wrapping the expected arguments in an array is unnecessary and removing them should result in a passing test: https://github.com/philostler/rspec-sidekiq/pull/156/files#diff-4b213564ae539e591aeb48cb440f1c77R78

I think that should get you to a passing test suite.

coding-red-panda commented 4 years ago

Sorry it has taken this long, I've applied the suggestions you posted.

coding-red-panda commented 4 years ago

and all green, at least those that are supposed to pass :)

coding-red-panda commented 4 years ago

Is there a chance to get this merged down, so I can eliminate this pull request from my GitHub overview :) ?

mathieujobin commented 4 years ago

@packrat386 lets merge ! (I don't have write access)

packrat386 commented 4 years ago

Hey y'all. As you can probably tell, this repo is in need of a bit of TLC and I haven't been able to spend all that much time on it. I'm planning to spend some time this weekend to go through outstanding pull requests that can be merged and then cutting a new version. I know it's been a while since there have been any updates, so thanks for your patience.

coding-red-panda commented 4 years ago

no worries :) I'm just happy I was able to contribute something! As we use this gem in our projects, I'm more than happy to make additional PR's if I discover more problems while upgrading Ruby and Rails versions

packrat386 commented 4 years ago

These changes have been pulled into 3.1.0