Skip to content

Commit

Permalink
Fix issue of passing nil to formatters (from CSV PR)
Browse files Browse the repository at this point in the history
Interestingly the bug occurred thanks to our legacy format support.
Can't wait to get rid off it ;)

I changed the code for the formatter test to really make sure that
the same options passed to format/2 are also passed to write/2.
It didn't find the bug as the bug was in Configuration, but I
think it's still an improvement worth keeping.

Once this code is merged, we can delet the one unnecessary line
from the CSV formatter. Already tried it and it worked 🎉
  • Loading branch information
PragTob committed Nov 2, 2018
1 parent 01c767d commit dddb98b
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 16 deletions.
16 changes: 12 additions & 4 deletions lib/benchee/configuration.ex
Original file line number Diff line number Diff line change
Expand Up @@ -337,14 +337,22 @@ defmodule Benchee.Configuration do
# backwards compatible formatter definition without leaving the burden on every formatter
defp formatter_options_to_tuples(config) do
update_in(config, [Access.key(:formatters), Access.all()], fn
Console -> {Console, config.formatter_options[:console]}
CSV -> {CSV, config.formatter_options[:csv]}
JSON -> {JSON, config.formatter_options[:json]}
HTML -> {HTML, config.formatter_options[:html]}
Console -> formatter_configuration_from_options(config, Console, :console)
CSV -> formatter_configuration_from_options(config, CSV, :csv)
JSON -> formatter_configuration_from_options(config, JSON, :json)
HTML -> formatter_configuration_from_options(config, HTML, :html)
formatter -> formatter
end)
end

defp formatter_configuration_from_options(config, module, legacy_option_key) do
if Map.has_key?(config.formatter_options, legacy_option_key) do
{module, config.formatter_options[legacy_option_key]}
else
module
end
end

defp force_string_input_keys(config = %{inputs: inputs}) do
standardized_inputs =
for {name, value} <- inputs, into: %{} do
Expand Down
6 changes: 6 additions & 0 deletions test/benchee/configuration_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ defmodule Benchee.ConfigurationTest do

assert [{Benchee.Formatters.Console, %{a: :b}}] == suite.configuration.formatters
end

test "legacy formatter options default to just the module if no options are given" do
suite = init(formatters: [Benchee.Formatter.CSV])

assert [Benchee.Formatter.CSV] == suite.configuration.formatters
end
end

describe ".deep_merge behaviour" do
Expand Down
12 changes: 6 additions & 6 deletions test/benchee/formatter_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,25 @@ defmodule Benchee.FormatterTest do
test "calls `write/1` with the output of `format/1` on each module" do
Formatter.output(%Suite{configuration: %{formatters: [{FakeFormatter, %{}}]}})

assert_receive {:write, "output of `format/1` with %{}"}
assert_receive {:write, "output of `format/1` with %{}", %{}}
end

test "works with just modules without option tuple" do
test "works with just modules without option tuple, defaults to empty map" do
Formatter.output(%Suite{configuration: %{formatters: [FakeFormatter]}})

assert_receive {:write, "output of `format/1` with %{}"}
assert_receive {:write, "output of `format/1` with %{}", %{}}
end

test "options are passed on correctly" do
Formatter.output(%Suite{configuration: %{formatters: [{FakeFormatter, %{a: :b}}]}})

assert_receive {:write, "output of `format/1` with %{a: :b}"}
assert_receive {:write, "output of `format/1` with %{a: :b}", %{a: :b}}
end

test "keyword list options are deep converted to maps" do
Formatter.output(%Suite{configuration: %{formatters: [{FakeFormatter, [a: [b: :c]]}]}})

assert_receive {:write, "output of `format/1` with %{a: %{b: :c}}"}
assert_receive {:write, "output of `format/1` with %{a: %{b: :c}}", %{a: %{b: :c}}}
end

test "mixing functions and formatters works" do
Expand All @@ -39,7 +39,7 @@ defmodule Benchee.FormatterTest do

Formatter.output(suite)

assert_receive {:write, "output of `format/1` with %{}"}
assert_receive {:write, "output of `format/1` with %{}", %{}}
assert_receive {:fun, ^suite, "me"}
end

Expand Down
4 changes: 2 additions & 2 deletions test/benchee_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -315,8 +315,8 @@ defmodule BencheeTest do
end)

assert_received_exactly([
{:write, "output of `format/1` with %{}"},
{:write, "output of `format/1` with %{}"},
{:write, "output of `format/1` with %{}", %{}},
{:write, "output of `format/1` with %{}", %{}},
:other
])
end
Expand Down
8 changes: 4 additions & 4 deletions test/support/fake_formatter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ defmodule Benchee.Test.FakeFormatter do

@behaviour Benchee.Formatter

def format(_, opts) do
"output of `format/1` with #{inspect(opts)}"
def format(_, options) do
"output of `format/1` with #{inspect(options)}"
end

def write(output, _) do
send(self(), {:write, output})
def write(output, options) do
send(self(), {:write, output, options})
end
end

0 comments on commit dddb98b

Please sign in to comment.