Benchmarks tests #18

tallysmartins · 2018-05-22T21:47:48Z

No description provided.

coveralls · 2018-05-22T21:50:17Z

Coverage increased (+18.3%) to 62.903% when pulling a39b9b1 on tallysmartins:benchmarks_tests into 91c2ae6 on elixir-bench:master.

PragTob

Looking good!

PragTob · 2018-05-23T18:47:06Z

test/elixir_bench/benchmarks/benchmarks_test.exs

@@ -2,17 +2,16 @@ defmodule ElixirBench.BenchmarksTest do
  use ElixirBench.DataCase
  alias ElixirBench.{Benchmarks, Benchmarks.Job}
  alias ElixirBench.Repos
+  import ElixirBenchWeb.TestHelpers


this uhhh... shouldn't work? Afaik/iirc (macros aren't my strong suite admittedly) - but iirc you'd need to reuquire a module to use its macros. I'm confused, CI passes 🤔

@michalmuskala ? :D

import implicitly does a require. I'm not a huge fan of this behaviour, but it's probably far too late to change it now.

😱 😱 😱 😢 😢 😢

Thanks Michał

PragTob · 2018-05-23T18:47:21Z

test/elixir_bench/benchmarks/benchmarks_test.exs

@@ -2,17 +2,16 @@ defmodule ElixirBench.BenchmarksTest do
  use ElixirBench.DataCase
  alias ElixirBench.{Benchmarks, Benchmarks.Job}
  alias ElixirBench.Repos


alias unused

PragTob · 2018-05-23T19:01:08Z

test/test_helper.exs

@@ -43,6 +43,22 @@ defmodule ElixirBenchWeb.TestHelpers do
    assert json_data == response
  end

+  defmacro assert_difference(queryable, value, do: expression) do


this is interesting! 👍 I think it's a cool macro - generally macros should be our last resort. Instead of macros we could have also used anonymous functions.

However, especially for test helpers macros can be super helpful as they have the benefit over "usual" functions that they don't show up in the stack trace.

so yeah, seems good - but careful with macros :)

Nice! 😄 I couldn't think about a way to not evaluate the function before I count, so I needed the macro.

PragTob · 2018-05-25T05:45:06Z

Tests are red because something you changed now seems to require erlang 20 🤔

PragTob

Looking good, couple of smaller suggestion and comments + those CI failures :)

PragTob · 2018-05-25T05:48:56Z

lib/elixir_bench/benchmarks/job.ex

@@ -46,6 +46,7 @@ defmodule ElixirBench.Benchmarks.Job do
  @create_fields [
    :branch_name,
    :commit_sha,
+    :repo_id,


it seems weird that we'd need to add this here the application was working before/is working - why would we need to add it here? 🤔

PragTob · 2018-05-25T05:52:19Z

test/elixir_bench/benchmarks/benchmarks_test.exs

+      {:ok, runner} = Benchmarks.create_runner(%{name: name, api_key: api_key})
+
+      assert runner.id
+      assert %Runner{name: ^name, api_key: ^api_key} = runner


I think this test might be enough for the runner tests - create_runner just creates a changeset and tries to insert it. All the ways in which that could or could not go wrong is probably better tested in the runner unit tests by just checking if the changeset is valid.

That tests the core component this relies on, is faster and more focussed :)

PragTob · 2018-05-25T05:54:46Z

test/elixir_bench/benchmarks/benchmarks_test.exs

+    test "return error if job not found" do
+      binary_uuid = "99999999-9999-9999-9999-999999999999"
+      assert {:error, :not_found} = Benchmarks.fetch_job_by_uuid(nil)
+      assert {:error, :not_found} = Benchmarks.fetch_job_by_uuid(binary_uuid)


these tests are ok but I think in the future we don't need to write as many - these are a very thin wrapper around ecto's Repo fetch so we can generally trust that it works and I don't think they provide too much value :)

PragTob · 2018-05-25T05:56:08Z

test/elixir_bench/benchmarks/benchmarks_test.exs

+      repo = insert(:repo)
+
+      {:ok, job} = Benchmarks.create_job(repo, %{branch_name: "mm/benche", commit_sha: "ABC123"})
+      assert %Job{} = job


I don't think we need both the it's inserted test and it returned it test - both can be rolled into one test imo. Here we could also assert some attributes :)

PragTob · 2018-05-25T05:58:00Z

test/elixir_bench/benchmarks/benchmarks_test.exs

+
+      # This does not work because in `measurement` the attributes job and benchmark
+      # are not loaded `#Ecto.Association.NotLoaded`
+      # assert_map_attr(my_measurement, measurement, [:id, :job, :benchmark])


the association is not loaded, which means you need to do some form of preloading

I refactored the tests, no need to preload anymore.

tallysmartins · 2018-05-25T22:23:23Z

test/elixir_bench/benchmarks/benchmarks_test.exs

-        Benchmarks.create_job(repo, %{branch_name: "mm/benche", commit_sha: "ABC123"})
-
-      refute changeset.valid?
+      assert_raise Postgrex.Error, fn ->


I didn't want this to raise an exception, thats why I added the verification for repo_id in the Job.changeset. I found that the association attributes are handled by database constraints and they should be created by ecto when you have a references(target_table) in the migration, so it returns {:error, reason} in case of violation. We have all this, but it didn't act as I expected.

But I think this is ok for now. I will just put a FIXME on it.

yeah we should fix it but probably in another ticket :)

Not a big fan of FIXME - rather create an issue for it

tallysmartins · 2018-05-25T22:27:54Z

The tests are failing maybe beacaus now the tests cover function calls for Bcrypt inside the ElixirBench.Benchmarks.Runner module, but idk why this didn't break before. 🤔

PragTob

Looking good 👍

PragTob · 2018-05-26T09:26:35Z

test/elixir_bench/benchmarks/benchmarks_test.exs

-        Benchmarks.create_job(repo, %{branch_name: "mm/benche", commit_sha: "ABC123"})
-
-      refute changeset.valid?
+      assert_raise Postgrex.Error, fn ->


yeah we should fix it but probably in another ticket :)

Not a big fan of FIXME - rather create an issue for it

PragTob · 2018-05-26T09:29:17Z

Yeah seems like it... maybe they didn't fail before because we didn't hit that path? Not sure... there were no dependency upgrades maybe just change the travis test matrix to only use erlang 20.3 or something :)

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>

PragTob

🎉

tallysmartins force-pushed the benchmarks_tests branch from 8fd5ee3 to fffb2c6 Compare May 22, 2018 21:48

PragTob reviewed May 23, 2018

View reviewed changes

tallysmartins force-pushed the benchmarks_tests branch 2 times, most recently from 9e79aa1 to ab58302 Compare May 24, 2018 22:07

PragTob reviewed May 25, 2018

View reviewed changes

tallysmartins commented May 25, 2018

View reviewed changes

PragTob reviewed May 26, 2018

View reviewed changes

tallysmartins force-pushed the benchmarks_tests branch from c82591a to ddb9ade Compare May 27, 2018 14:33

tallysmartins added 5 commits May 27, 2018 16:56

Adds assert_difference

e45c958

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>

Adds more tests

0334b0d

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>

More tests to Benchmarks

019bc40

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>

Refactoring tests

24cfc2f

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>

Adds more tests to Benchmarks module

a39b9b1

Signed-off-by: Tallys Martins <tallysmartins@gmail.com>

tallysmartins force-pushed the benchmarks_tests branch from ddb9ade to a39b9b1 Compare May 27, 2018 19:57

tallysmartins changed the title ~~WIP: Benchmarks tests~~ Benchmarks tests May 27, 2018

PragTob approved these changes May 28, 2018

View reviewed changes

PragTob merged commit ddb5107 into elixir-bench:master May 28, 2018

tallysmartins deleted the benchmarks_tests branch May 28, 2018 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Benchmarks tests #18

Benchmarks tests #18

tallysmartins commented May 22, 2018

coveralls commented May 22, 2018 •

edited

Loading

PragTob left a comment

PragTob May 23, 2018

michalmuskala May 23, 2018

PragTob May 23, 2018

PragTob May 23, 2018

PragTob May 23, 2018

tallysmartins May 25, 2018

PragTob commented May 25, 2018

PragTob left a comment

PragTob May 25, 2018

PragTob May 25, 2018

PragTob May 25, 2018

tallysmartins May 25, 2018

PragTob May 25, 2018

tallysmartins May 25, 2018

PragTob May 25, 2018

tallysmartins May 25, 2018

tallysmartins May 25, 2018

PragTob May 26, 2018

tallysmartins commented May 25, 2018 •

edited

Loading

PragTob left a comment

PragTob May 26, 2018

PragTob commented May 26, 2018

PragTob left a comment

Benchmarks tests #18

Benchmarks tests #18

Conversation

tallysmartins commented May 22, 2018

coveralls commented May 22, 2018 • edited Loading

PragTob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragTob commented May 25, 2018

PragTob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallysmartins commented May 25, 2018 • edited Loading

PragTob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragTob commented May 26, 2018

PragTob left a comment

Choose a reason for hiding this comment

coveralls commented May 22, 2018 •

edited

Loading

tallysmartins commented May 25, 2018 •

edited

Loading