Eh, testing has more than one function. I use this strategy the most when I’m doing TDD (I’m not religious about it) and for example it appears when I’m testing intermediate compile-time artifacts with NimbleParsec. In this case, the primary function of the test is clarity during the code writing phase and assistance in reading and understanding the code later. I personally would really hate to test complex parsers strictly in situations that are “the code you see in prod”.
ityonemo
tcoopman
The boundary library might also help you to keep things public but still define who is allowed to use it and who isn’t
Personally I don’t think there’s anything wrong with testing private functions. So I wouldn’t worry too much about it if the library does what you need.
2
pragdave
Author of Programming Elixir
FWIW, my personal belief is that no one knows enough about software development to be able to make rules: it’s just too young a discipline.
But, I also believe that:
-
defining helper functions as private sets a demarcation layer in my code: if the code is to be used by a third party, then making a function private means that I’m free to change it without worrying about unintended consequences.
-
tests can be helpful for some functions. Which ones? The ones where there’s doubt; the ones which are pivotal to the rest of the code; the ones where the tests help you explore the problem.
These two things do not necessarily correlate. I often find myself writing internal (private) functions which need tests.
“But, Dave” say those in the never-test-a-private-function camp. “Why not just test them indirectly via the public functions that use them?”
Because, I say (outside of quotes), that is often a lot of work. The higher level the function, the more state it assumes, and the harder the test is to write. And, typically, when you invoke these higher level functions, you’re calling a number of lower-level ones. The test loses focus.
Let’s look at this a different way. Let’s think about the demarcation of code in GenServers.
When I write GenServers, I typically split them into two files. One file contains the server stuff: start_link, init, and all the handle_xxx calls, the other contains the implementation.
Here’s part of a typical server:
defmodule ExMdns.Server.ReceiveDNSPackets do
alias ExMdns.Impl.PubSub
alias ExMdns.Impl.ReceiveDNSPackets, as: Impl
use GenServer
require Logger
@me __MODULE__
def start_link(_) do
GenServer.start_link(__MODULE__, [], name: @me)
end
def init(_) do
PubSub.join()
{ :ok, Impl.initial_state }
end
def handle_info({ :pubsub, msg=%PubSub.PacketReceived{}}, state) do
state = Impl.handle_incoming_packet(state, msg.data)
{ :noreply, state }
end
def handle_info({ :pubsub, _ }, state), do: { :noreply, state }
end
None of these functions contain any logic (apart from pattern matching on their parameters. Instead, they delegate to a second module that contains the actual logic.
The functions in this second module contain the implementation of the logic behind the server. The server module calls an implementation function with the server state as a parameter, along with any additional parameters from the handle_xxx. The implementation function returns an updated state.
Here’s the implementation corresponding to that server:
defmodule ExMdns.Impl.ReceiveDNSPackets do
alias ExMdns.Impl.PubSub
require Logger
def initial_state() do
[]
end
def handle_incoming_packet(state, raw_packet) do
try do
DnsPackets.decode(raw_packet.data)
|> publish_replies()
rescue
e ->
Logger.error("Ignoring invalid DNS packet: #{inspect raw_packet}")
Logger.error(inspect e)
end
state
end
defp publish_replies(packet) when packet.header.qr == :reply do
for answer <- packet.answers do
PubSub.publish(%PubSub.AnswerReceived{ answer: answer })
end
end
defp publish_replies(packet) do
Logger.info("ignoring query\n#{inspect packet, pretty: true}")
end
end
Why is this relevant to testing? Because I can test the implementation functions standalone, without firing up a GenServer. And it’s easy to do, because my tests just create the state to pass in, and then verify the state that comes back. In fact, I rarely run any genservers in test: My application.ex looks like this:
def start(_type, _args) do
children = case Mix.env do
:test ->
[ ]
_ ->
[
Server.Cache,
Server.ReceiveDNSPackets,
Server.UdpInterface,
# Registration,
]
end
opts = [strategy: :one_for_one, name: ExMdns.Supervisor]
Supervisor.start_link(children, opts)
end
What has this to do with private functions?
It’s an illustration of how I believe that testing convenience trumps following rules.
Typically, Elixir developers would put the implementation functions in the same module as the server stuff. They’d then struggle with whether they should be private. Purity insists that they should: they are accessed via the GenServer.call interface, and so shouldn’t be exposed externally. Make 'em private, and then test via GenServer calls. Or, make 'em public, test them directly, and feel guilty.
Instead, this two-module approach sidesteps the dogma. In my apps, nothing in a subdirectory of lib/ should be accessed externally. Instead, I have a top-level api.ex that delegates each API function, calling .Server files if the implementation of the function uses a GenServer, and calling the implementation files directly for stateless functions.
It then becomes immediately obvious to any external user of the code what they’re allowed to use: anything with more than one module name in the path is internal to my application. It may be public, but that’s for my convenience and not for your use.
Do I do this consistently? No: to be honest I’m experimenting with each app I write. This is where I am now.
TLDR; forget what people tell you is right. Do what you feel makes your code easier to work with.
25
sezaru
I do the same thing for gen servers (I’m pretty sure I got the idea from some article from you, haha).
But I tend to be, let’s say, a little bit more hardcore with the separation, I usually separate it into 4 files:
- The server public API file:
defmodule MyGenServer do
@moduledoc false
alias MyGenServer.Server
@spec child_spec(keyword) :: Supervisor.child_spec()
defdelegate child_spec(opts), to: Server
@spec do_something(any) :: :ok
def do_something(some_data),
do: GenServer.call(Server, {:do_something, some_data})
...
end
- The server gen_server implementation file:
defmodule MyGenServer.Server do
@moduledoc false
alias MyGenServer.Impl
use GenServer
def start_link(args),
do: GenServer.start_link(__MODULE__, args, name: __MODULE__)
@impl GenServer
def init(some_args), do: {:ok, Impl.init(some_args)}
@impl GenServer
def handle_call({:do_something, some_data}, from, state) do
{reply, state} = Impl.do_something(some_data)
{:reply, reply, state}
end
end
- The server business logic file:
defmodule MyGenServer.Impl do
@moduledoc false
alias MyGenServer.State
@spec init(any) :: State.t()
def init(args), do: State.new(args)
@spec do_something(any, State.t()) :: State.t()
def do_something(some_data, state) do
...
{:ok, State.update_something(2)}
end
end
- And finally the state file:
defmodule MyGenServer.State do
@moduledoc false
@type t :: %__MODULE__{something: integer}
defstruct something: 0
@spec new(any) :: t
def new(args), do: struct!(__MODULE__, args)
@spec update_something(t, integer) :: t
def update_something(state, something),
do: %{state | something: something}
end
It is a little bit more work to separate the server into these files, but IMO it makes the whole thing way easier to read, very easy to test and gives a very clear API for others to use.
0
pragdave
Author of Programming Elixir
Yeah, I went through a phase of having the GenServer API file, but stopped doing it in favor of having a top-level file for the actual external API.
I also use a module if my state is more complex than a list, but I keep it as an internal defmodule State in the Impl file.
But those are just details: whatever works is good.
1
trisolaran
The need to test private functions is in my opinion and experience, a serious design smell.
Testing should be focused on the external behavior of the system, defined by its API. Why? Because this is the only thing that matters to anyone who will be using it. As long as the system behaves as expected, it doesn’t matter what its internal implementation is.
If you have a component and you realize that you can’t properly test it with its API, chances are the component is too complex and tries to do too many things. Try to break it down into smaller components, each one with its own API that you can test. Then add integration tests for the whole system.
Sure, it’s more work in the short term, but your future self and your team members who will be looking at your code later will be thankful.
The resulting system will be easier to understand, extend, and refactor. Less work in the long term.
TDD can help you guide your design towards components with clear, testable interfaces.
1
tcoopman
First you say focus on external behavior.
A bit later you say, break a component down into smaller components with its own API.
What do you mean with external behavior? In the case of a REST API, is the REST API the only external behavior? Or does every module that we implement have external behavior for some other module using it?
What’s the difference between moving a private function into a separate module and testing that vs keeping the private function in the module and test it like that?
To be clear, I don’t think there is anything wrong to move a complex private function into a separate module because that makes it easier to test, but I also don’t think it’s wrong to keep it private and test it in an other way.
1
dorgan
Also, code doesn’t need to be complex to be hard to test.
The behaviour of a genserver with no public api that all it does is periodically call itself to perform some side effect is built on entirely private behavior that should not be used by any other module. I’ve had to deal with a bunch of them and they are always nasty to test.
Whatever approach you take, you’re ripping the module open and testing its internals, not any “observed behavior through public api”. Theres no public api.
The process calling itself every X time is not something you can test via public api, so giving up on idealistic notions of how code should be tested in favor of pragmatism is best. This point is also raised in the Testing Elixir book by Andrea Leopardi, in the Testing OTP chapter.
You can decide to move some functions to a different module and test that because now it’s no longer “private” and you can test it “normally”, but imho that’s just lying to yourself and if theres no other reason for that module to exist(to split complex logic into smaller files, to name an example), it’s just an indirection that brings very little value.
At that point, just test the behaviour callbacks directly by providing some state. This may sound controversial, but if theres no client api then no solution will appeal to everyone’s taste. FWIW this would be just like testing a custom Ecto type by calling the callbacks directly instead of creating a schema and persisting it to indirectly test dump and cast.
1000 times this
6
trisolaran
The latter. That’s what I mean by external behavior. The interfaces between modules or services.
Breaking down the logic among separate services is likely to make your code easier to read, understand, and reuse.
Let’s say you have a REST controller that needs: to a) access the database to fetch some data b) process the data in some way and c) call some external API to publish the data somewhere else.
Option 1: You build everything in the REST controller, which will do a, b and c. The public API is the REST API, you call it in a test and check it. All the nitty gritty of the logic from a, b and c live in private functions, and you test them too as part of the test suite for the REST controller.
Option 2: You spread a, b and c across their own modules or services with clear interfaces. You test the REST API in the rest controller, and the nitty gritty details of all things that can go wrong in a, b and c in their own tests suites. Since you’re working at a lower level, you can have more low-level, focused and precise tests.
Both options work. Both could lead to a perfectly tested and functioning system. But which option do you think is more likely to lead to a codebase (and test suite) which is easier to understand, refactor, and with components that can be reused in other parts of the codebase if needed? For me, the answer is clear.
I also don’t think it’s “wrong”. I didn’t want to give the impression that these are absolute rules you have to follow. But I think you have generally accepted good design principles, even in software engineering. There are surely exceptions, but there are always exceptions. I don’t think I ever added a test for a private function or method in my career - everytime I was tempted to do it because it was “easy”, I took a step (sometimes 2) back and realized that I had uncovered something not very good about my design. That’s what I meant by design smell.
0
tcoopman
I definitely agree with splitting into modules in your example, but there are also examples where I wouldn’t do it.
I’m talking a situation where there is complex behavior + something that’s really tightly coupled to something else. I don’t have a good example right away because it’s not something that comes up that often.
But every now and then, I want to test some private functions. But because a lot of languages don’t really have decent support for that (some do), I mostly don’t bother and either make it public or extract it. Even though it’s an implementation detail - that I’d rather not expose to everyone.
I also think we have these kind of discussions because most languages only have private or public functions. That’s what I like about the idea of boundary. You don’t need to make things public for everyone, you can say who is allowed to call what. So, by making something public for testability reasons, you don’t need to expose it to the whole world.
1