-
Notifications
You must be signed in to change notification settings - Fork 148
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
Pass repo_options to repo.insert! to support TenantFactories #377
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,22 @@ defmodule ExMachina.EctoTest do | |
end | ||
end | ||
|
||
defmodule MockRepo do | ||
def insert!(_model, opts), do: opts | ||
end | ||
|
||
defmodule MockedTenantFactory do | ||
use ExMachina.Ecto, repo: MockRepo, repo_options: [prefix: "test_tenant"] | ||
|
||
def user_factory do | ||
%ExMachina.User{} | ||
end | ||
end | ||
|
||
test "it passes repo_options to Repo.insert!" do | ||
assert [prefix: "test_tenant"] == MockedTenantFactory.insert(:user) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding a test with this 🎉 I see that we're using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mocking the repo was done in an attempt to prevent that, but I agree that changing the return spec of I'm fine with creating an integration test to the database. Any advise on how to set that up? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't looked deeply into how to set the multi-tenancy with Ecto either. I think all the setup for Ecto in ExMachina's tests comes from If I have time, I will also try to give this a shot, but I'm a bit swamped right now. Thank you for getting this PR this far 🎉. Let's see if someone else comes along and can finish it or if either of us frees up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @germsvel So to merge this PR is all about to add a sample test with multi tenancy? The sample There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, I think if we could get a full integration test with multi-tenancy we'd be good. I just want to make sure this works correctly and that we have it tested.
I don't know if you mean the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @germsvel Any thoughts on @mojidabckuu's last comment? We too are in need of this feature! 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I missed that. Yes, that sounds like a good test to me! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found this while looking for tenet-support fixtures for my own project. If this PR still needs more test, and the above described acceptance criteria are still valid, let me know and maybe I can take a shot. Love using this on my other projects. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for jumping in @zorn. Yeah, the above acceptance criteria is still what's needed. I just want to make sure we have a test testing multi-tenancy. I'd love to get this merged after that. Let me know if you need any help or if that's good enough to move forward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I worked on this today but ran into the same issues I ran into a year ago when trying to use the Ecto sandbox env and dynamic tenants. Specifically in a traditional test, the database is dropped/created/migrated before the tests run inside a db transaction (to allow for fast async tests), but when doing a dynamic tenant it's not obvious to me how to run prefixed migrations inside a the transaction-based checkouts after manually making the tenant with something like The other thing to keep in mind (for anyone who picks this up) is that Postgres and MySQL do this behavior differently. In one of my projects I've leaned on Triplex for this abstraction. Not sure if With what I know now, on an individual project, it wouldn't be too hard to code a project's This thread may also help people, but I am suspect that this works for async tests. |
||
end | ||
|
||
test "params_for/1 still works as expected" do | ||
user_params = NoRepoTestFactory.params_for(:user) | ||
|
||
|
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.
🎉