PROPOSAL: Formalize ExUnit.Formatter as a behaviour

31 views
Skip to first unread message

Devon Estes

unread,
Dec 3, 2019, 5:16:47 AM12/3/19
to elixir-lang-core
I'm not sure if there is a reason for not having done this in the past but I couldn't find anything in the mailing list about it already.

There is essentially a behaviour that's defined  in the documentation for ExUnit.Formatter, but it's not actually enforced by defining a @behaviour for those formatters because it's just a duplicate of the GenServer behaviour. Because this behaviour is rather specific, I think it would be easier for users to implement their own formatters if there was a properly defined ExUnit.Formatter behaviour. If there was a way to `use ExUnit.Formatter` as a base implementation that would be even better, but I think a good first step is moving to a behaviour here.

My idea for this behaviour is as follows:

```
defmodule ExUnit.Formatter do
  @callback init(config :: list) ::
              {:ok, state}
              | {:ok, state, timeout | :hibernate | {:continue, term}}
              | :ignore
              | {:stop, reason :: any}
            when state: any

  @callback suite_started(opts :: list) :: :ok
  @callback suite_finished(run_us :: non_neg_integer, load_us :: non_neg_integer) :: :ok
  # similar callbacks for all the other events
end
```

By keeping `init/1` from the GenServer behaviour we make it so we can still easily start these formatters under the DynamicSupervisor in ExUnit.EventManager.add_handler/2, but by moving away from calling `GenServer.cast/2` directly and using the behaviour interface we can more clearly define that interface and also broaden the way people can implement formatters. For example, it's currently deprecated to use a gen_event handler, but if we move to this behaviour then users can use any abstraction they want behind that interface as long as the module has an `init` function so it can be started under a supervisor. They would most likely still be GenServers, but if folks wanted to do something else, they totally could. This would require a change to how formatters are stored in ExUnit.EventManager to keep a list of the formatter modules that we're calling our functions on instead of just relying on DynamicSupervisor.which_children/1 to get the PIDs.

I also thought that this behaviour might also work (and maybe be better) since it would more easily allow creating formatters that weren't started as globally named processes:

```
defmodule ExUnit.Formatter do
  @callback init(config :: list) ::
              {:ok, state}
              | {:ok, state, timeout | :hibernate | {:continue, term}}
              | :ignore
              | {:stop, reason :: any}
            when state: any

  @callback suite_started(formatter :: pid, opts :: list) :: :ok
  @callback suite_finished(formatter :: pid, run_us :: non_neg_integer, load_us :: non_neg_integer) :: :ok
  # similar callbacks for all the other events
end
```

This essentially keeps the same behaviour that we have now, but moves the implementation details of sending messages to the formatters into the formatters themselves as opposed to ExUnit.EventManager.notify/2 knowing about the internals of sending messages to the formatters as they do now (and has caused issues as evidenced by the gen_event deprecation). For this we'd want to change ExUnit.EventManager to keep a mapping of which pids belong to which module to make things easy when processing events.

The biggest downside of this that I can see is that it would be a bit of a hassle to keep this third way of handling formatters in addition to the existing two, but once Elixir 2.0 comes along and breaking changes are allowed to be made, we could go down to just this single way of handling things and it should be extensible in the future since it more cleanly decouples the developer's formatter implementation from ExUnit's implementation.

I'd be happy to send a PR on this if y'all think it might make it easier to discuss this with a bit of an implementation to look at as a reference.
Reply all
Reply to author
Forward
0 new messages