Pass repo_options to repo.insert! to support TenantFactories #377
Conversation
joeppeeters
force-pushed
the
pass_repo_options
branch
from
eec825f to
a1e2418
Compare
| end | ||
|
|
||
| test "it passes repo_options to Repo.insert!" do | ||
| assert [prefix: "test_tenant"] == MockedTenantFactory.insert(:user) |
There was a problem hiding this comment.
Thanks for adding a test with this 🎉
I see that we're using the MockRepo defined above which only returns the opts back (which makes for really easy testing). But I'm wondering, how do you feel about adding a test that exercises the full insert into a separate tenant? Since it's a new feature, it would be nice to have more certainty that everything works as expected. What do you think?
There was a problem hiding this comment.
Mocking the repo was done in an attempt to prevent that, but I agree that changing the return spec of insert/1 makes it a bit weird.
I'm fine with creating an integration test to the database. Any advise on how to set that up? I'll try to look into it later this week. (Sorry, I'm don't know when I get to this. Someone should feel free to add the missing test.)
There was a problem hiding this comment.
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 test/support/test_repo.ex and config/test.exs.
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.
@germsvel So to merge this PR is all about to add a sample test with multi tenancy? The sample User.ex schema would be enough?
There was a problem hiding this comment.
So to merge this PR is all about to add a sample test with multi tenancy?
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.
The sample User.ex schema would be enough?
I don't know if you mean the User schema that coming from the factory defined above this test or something like https://github.com/thoughtbot/ex_machina/blob/master/test/support/models/user.ex. I think ideally we have a new schema for the multi-tenancy?
There was a problem hiding this comment.
@germsvel Any thoughts on @mojidabckuu's last comment? We too are in need of this feature! 🙂
There was a problem hiding this comment.
Sorry, I missed that. Yes, that sounds like a good test to me!
There was a problem hiding this comment.
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.
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.
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 Repo.query("CREATE SCHEMA test_tenant"). I saw Ecto.Migrator.run(Repo, :up, options) and Ecto.Adapters.SQL.Sandbox.mode(Repo, :auto) which are related but I keep running into pid ownership issues no matter what I try.
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 ex_machina currently or wants to support MySQL.
With what I know now, on an individual project, it wouldn't be too hard to code a project's mix test to do a prefix migration with mix ecto.migrate --prefix "prefix_1" ahead of running tests and making some assumptions about a tenant name. Sadly, dynamic tenants seem to be much harder for me to get working, with or without this library. :(
This thread may also help people, but I am suspect that this works for async tests.
|
|
||
| ... | ||
| end | ||
| ``` |
|
I just opened #411, which might help with the multi-tenancy issue at the point of calling |
|
This pull request has been automatically marked as "stale:discard". If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated!. |
|
Closing this pull request after a prolonged period of inactivity. If this issue is still relevant, please ask for this pull request to be reopened. Thank you! |
Closing #280