Skip to content

Conversation

Gigitsu
Copy link
Contributor

@Gigitsu Gigitsu commented Aug 22, 2025

As discussed in this Elixir Forum post, I encountered issues when testing with a dynamic repo.

This PR allows explicitly passing a dynamic repo to both Ecto.Adapters.SQL.Sandbox.checkout/1 and Ecto.Adapters.SQL.Sandbox.start_owner!/1.

With this change, I can change my DataCase setup as follows:

def setup_sandbox(tags) do
  pid = SQL.Sandbox.start_owner!(Hello.Repo.get_dynamic_repo(), shared: not tags[:async])
  on_exit(fn -> SQL.Sandbox.stop_owner(pid) end)
end

This enables proper testing of dynamic repos without manual connection management.

@Gigitsu
Copy link
Contributor Author

Gigitsu commented Aug 22, 2025

Integration tests are failing because a test expects an UndefinedFunctionError with the message ~r"function UnknownRepo.get_dynamic_repo/0 is undefined".

Could there be a better way to check if an atom represents a module?

I could try using Kernel.function_exported?/3 but this won't fix the failing test.

@josevalim
Copy link
Member

I assume your code is a bit more complex than this, right?

def setup_sandbox(tags) do
  pid = SQL.Sandbox.start_owner!(Hello.Repo.get_dynamic_repo(), shared: not tags[:async])
  on_exit(fn -> SQL.Sandbox.stop_owner(pid) end)
end

How does your code look like?

@Gigitsu
Copy link
Contributor Author

Gigitsu commented Aug 22, 2025

I assume your code is a bit more complex than this, right?

Yes, it's not much more complex, but it involves some code that might be internal to Ecto.

How does your code look like?

I ended up with this minimal setup:

  def setup_sandbox(tags) do
    pid = SQL.Sandbox.start_owner!(repo_pid(), shared: not tags[:async])
    on_exit(fn -> SQL.Sandbox.stop_owner(pid) end)
  end

  defp repo_pid do
    GenServer.whereis(Hello.Repo.get_dynamic_repo())
  end

We could also introduce a new get_dynamic_repo_pid() to Ecto.Repo instead of this, but I don’t really like the idea of introducing a function solely for this case.

@josevalim
Copy link
Member

The code that you pasted above is completely fine to me and only rely on public APIs. That’s definitely what I would recommend in this case.

@Gigitsu
Copy link
Contributor Author

Gigitsu commented Aug 23, 2025

Ok, that seems fair to me.
Honestly, I thought this would be an easier PR... I didn’t expect it to break some tests.
Sorry about that, and thank you!

@Gigitsu Gigitsu closed this Aug 23, 2025
@josevalim
Copy link
Member

No need to be sorry, thank you for giving it a shot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants