From dddb98b7b3c452f53d8f91110663414aae00ca0a Mon Sep 17 00:00:00 2001 From: Tobias Pfeiffer Date: Fri, 2 Nov 2018 16:37:56 +0100 Subject: [PATCH] Fix issue of passing nil to formatters (from CSV PR) 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 :tada: --- lib/benchee/configuration.ex | 16 ++++++++++++---- test/benchee/configuration_test.exs | 6 ++++++ test/benchee/formatter_test.exs | 12 ++++++------ test/benchee_test.exs | 4 ++-- test/support/fake_formatter.ex | 8 ++++---- 5 files changed, 30 insertions(+), 16 deletions(-) diff --git a/lib/benchee/configuration.ex b/lib/benchee/configuration.ex index f7806059..c3b312a6 100644 --- a/lib/benchee/configuration.ex +++ b/lib/benchee/configuration.ex @@ -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 diff --git a/test/benchee/configuration_test.exs b/test/benchee/configuration_test.exs index e18e04fd..339a38ef 100644 --- a/test/benchee/configuration_test.exs +++ b/test/benchee/configuration_test.exs @@ -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 diff --git a/test/benchee/formatter_test.exs b/test/benchee/formatter_test.exs index 40c0b424..00d0d4cb 100644 --- a/test/benchee/formatter_test.exs +++ b/test/benchee/formatter_test.exs @@ -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 @@ -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 diff --git a/test/benchee_test.exs b/test/benchee_test.exs index 51f9fe85..6fe693df 100644 --- a/test/benchee_test.exs +++ b/test/benchee_test.exs @@ -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 diff --git a/test/support/fake_formatter.ex b/test/support/fake_formatter.ex index 18928bd5..fabeba85 100644 --- a/test/support/fake_formatter.ex +++ b/test/support/fake_formatter.ex @@ -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