-
Notifications
You must be signed in to change notification settings - Fork 231
Stop mocking models in controller specs and switch to using ActiveRecord directly #390
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
Stop mocking models in controller specs and switch to using ActiveRecord directly #390
Conversation
…ord directly Sorcery used to support multiple ORMs, so controller specs were heavily mocked to stay ORM-agnostic. Reference: [ORM-wide controller specs #586 by kirs · Pull Request #591 · NoamB/sorcery](NoamB/sorcery#591 (comment)) However, since [v0.10.0](https://github.com/Sorcery/sorcery/blob/a5e2035d0a2c3cdabbeaf720949e9bf7d360986a/CHANGELOG.md#0100), non-ActiveRecord ORM adapters were moved out, and Sorcery now only supports ActiveRecord. In this situation, mocks hurt test readability and make maintenance harder, so I removed them and now use ActiveRecord directly in controller specs. Also removed redundant logic and unnecessary hacks.
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.
Pull Request Overview
This pull request removes ORM-agnostic mocking from controller specs and switches to using ActiveRecord directly. Since Sorcery v0.10.0 dropped support for non-ActiveRecord ORMs, the previous heavy mocking approach is no longer necessary and actually hurts test readability and maintainability.
- Removed
SORCERY_ORM
constant and related conditional logic - Replaced mock objects with real User instances created via ActiveRecord
- Enabled transactional fixtures for better test isolation
- Removed unnecessary database cleanup and Rails 4 compatibility hacks
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
spec/spec_helper.rb | Removed ORM constant, enabled transactional fixtures |
spec/shared_examples/user_shared_examples.rb | Removed ORM conditionals from migration setup |
spec/rails_app/config/application.rb | Simplified configuration to use ActiveRecord directly |
spec/controllers/*.rb | Replaced mock users with real User instances, removed mocking expectations |
lib/sorcery/test_helpers/internal/rails.rb | Cleaned up callback management and removed DataMapper references |
lib/sorcery/test_helpers/internal.rb | Removed ORM conditional logic |
lib/sorcery/controller/submodules/activity_logging.rb | Simplified attribute accessor definitions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Thanks @joshbuker, much appreciated. @willnet, I'm happy to take a look t this. I'll make a note to look through it tomorrow. We can also make a bit of a plan for what you want to get done going forward. |
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.
Looks pretty good. My only concern is that expects
adds actual expectations to tests. Removal of these expectation clauses means we're not actually checking if a method is or isn't called on an object and how many times etc...
I think going forward this would have been a good PR to be split into maybe 3 smaller ones to make it easier to quickly see what's being changed :)
Great work though! Looking forward to seeing future progress on this gem! :D
Feel free to merge this @willnet :) |
@brendon Thanks for the review. To make it easier to review, it would have been better to split this into smaller PRs. I’ll do that next time. |
No worries at all :) We should be able to get some momentum going now. One of my biggest wishes is making oauth optional so we don't get the gem included if we don't want it :) |
Thanks @joshbuker, that's interesting. How far did you get on that, and do you think it'd be easier to start again or has the main codebase not drifted too far yet? |
@brendon I got pretty far, but it's a complete rewrite from the ground up so drift is a non-issue. If you're wanting to improve Sorcery for long term use, I would recommend starting from it rather than starting over from scratch or trying to use the existing codebase. |
Fair enough :) Were there any major chances to the current |
Any commits after 2023 (so 3694994 and later) should be reviewed for relevant changes to functionality. Where I left off was getting the current test suite 1:1 in the rework repo to ensure it at least had parity with what existed. |
I think it’s more realistic to focus on improving this repository rather than trying to push forward with sorcery-rework. Getting sorcery-rework to the point of release would require a huge amount of energy and time, and currently, no one seems to have the capacity for that (though of course, if someone does step up, I’d be happy to help). Improving this repository is still a big challenge, but making incremental fixes to code that already runs feels like something I can manage. My main concern is making sure Sorcery works smoothly with the latest Ruby and Rails. In the near term, I’d like to tackle #373 and #351, and then move on to #312. I’d also like to separate out oauth eventually, but I think that comes after addressing the more immediate issues. If @brendon is planning to work on the oauth separation, I’d be glad to support that. |
Thanks @willnet, I like your plan and at some point if I have a chunk of time I'll look at removing oauth. My thoughts around that were that instead of having to seperate all of that functionality out into a seperate gem, we could do some kind of configuration around enabling oauth with the caveat that if it's activated, the user needs to load in the oauth gem themselves. |
Sorcery used to support multiple ORMs, so controller specs were heavily mocked to stay ORM-agnostic. Reference: ORM-wide controller specs #586 by kirs · Pull Request #591 · NoamB/sorcery
However, since v0.10.0, non-ActiveRecord ORM adapters were moved out, and Sorcery now only supports ActiveRecord. In this situation, mocks hurt test readability and make maintenance harder, so I removed them and now use ActiveRecord directly in controller specs.
Also removed redundant logic and unnecessary hacks.