-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Upgrade test suite to use Rails 5 #981
Conversation
`@secord_is_batch_request` -> `@second_is_batch_request`
users with different providers should be able to use the same email address. here is the line that we're overriding in devise: https://github.com/plataformatec/devise/blob/83213569dd77551bb5cac493e2767457970905bc/test/rails_app/lib/shared_admin.rb#L12
notable changes: - added rails version to migrations - updated test requests to new format - updated several test runner dependencies - various rubocop linting fixes within test suite
Cool, looks like #863 fixes the password reset issue. @zachfeldman @MaicolBen please take a look when you get a chance. this PR resolves a bunch of issues, but I'm gonna need another day or two to test with clients before cutting a release (#972). |
This is a lot! I scanned through and didn't see anything too crazy. Way to update past hash rocket hashes :). Considering this PR is from the author of the gem, and tests are passing, I'm inclined to merge it. Regressions will be evident on the next release and we're paying close attention to the issue tracker if people have problems. I'm going to approve this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!! You can merge it
I apologize for this massive PR. Rails 5 changed the test request format and it resulted in a ton of changes to the test suite.
Notable changes:
A few bugs were revealed (and fixed) as a result of this update:
render: :nothing
, which is no longer supported in Rails 5:confirmable