Some good changes in Job module #10

tallysmartins · 2018-07-05T02:14:22Z

Support to wait-for, added to command of the docker-compose.yml
Remove run_times from measurements results
Refactor Job module and add some tests

- Job.start_job/2 now receive a function to execute jobs, so we can write tests easily without make the full processing of a job - Turning private methods into public so we can test the complex logic of processing a job to ensure everything is working Signed-off-by: Tallys Martins <tallysmartins@gmail.com>

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

- Now, when a docker dependencie is specified, it can define a *wait-for* attribute in the config.yml file. This attribute will be used to listen the service port for TCP conections, assuming that its ready if it responds in a given timeout. There is a script wait-for.sh in the runner-container to make this check. For example, if a dependencie is given with wait-for: 5432, the command for the runner service in the docker-compose is overriden with 'wait-for localhost:5432 -- mix run bench/bench_helper.exs`. Otherwise, if no dependence is given or the param wait-for is not given, the default command 'mix run bench/bench_helper' is returned. Signed-off-by: Tallys Martins <tallysmartins@gmail.com>

PragTob · 2018-07-18T19:03:29Z

Sorry I seem to have missed this one not getting a notification for it :'(

PragTob

Sorry I somehow missed this - must have clicked away the notification or something 😊 . Comments inline

PragTob · 2018-07-18T19:04:29Z

lib/runner.ex


-    config = Config.from_string_map(config)
-    job = Job.start_job(id, repo_slug, branch, commit, config)
+    job = Job.start_job(job, opts)


seems like we're missing opportunity to use the great pipe here :) |>

revision |> fix() ✔️

PragTob · 2018-07-18T19:08:57Z

lib/runner.ex

+      branch: branch,
+      commit: commit,
+      config: config
+    }


it's not immedeatly obvious what the benefit of building the job struct here is. Not sure it should be or how to make it better. Just an observation. I think we do it so that we have a job struct to work with rather than the data map. Then it's also weird though why some of the fields are called slightly different - we're in control of all those field names right? Or am I missing something? Then we should probably call them all the same, then creating a job struct from it is also much easier.

PragTob · 2018-07-18T19:09:44Z

lib/runner/job.ex

-      branch: branch,
-      commit: commit,
-      config: config
-    }


I see you moved the responsibility. I think that's a good thing, that the job module just deals with job structs here so I like it 👍

Yeah.. regarding the field names, I created an issue #11 to make them compatible later

PragTob · 2018-07-18T19:12:51Z

lib/runner/job.ex

    }
  end

+  defp build_runner_command_from_deps(deps) do
+    command =
+      Enum.reduce(deps, "", fn {_dep_name, dep_params}, acc ->


I think renaming acc to something like command here helps the readability as it makes it more obvious the accumulator is hopening the command or maybe the wait_command something like that

fixed! ✔️

PragTob · 2018-07-18T19:17:31Z

lib/runner/job.ex

-      config: config
-    }
+    timeout = Keyword.get(opts, :timeout, Confex.fetch_env!(:runner, :job_timeout))
+    task_fun = Keyword.get(opts, :task_fun, &run_job/1)


not sure if having the task_function be part of the options is the best solution here. Having it be part of the options sort of suggests to me that it's viable to pass something different in there. In reality we just want to have it for testing purposes.

I think using a default argument like runner_function \\ &fun works in our favor here

Changed! ✔️

PragTob · 2018-07-18T19:25:53Z

test/runner/job_test.exs

@@ -45,4 +19,253 @@ defmodule ElixirBench.Runner.JobTest do
    assert job.context.worker_num_cores in 1..64
    assert job.context.worker_os in [:Linux, :Windows, :macOS]
  end
+
+  describe "start_job/6" do


This isn't start_job/6 anymore (parameter number has changed)

PragTob · 2018-07-18T19:27:44Z

test/runner/job_test.exs

+    end
+  end
+
+  describe "get_benchmars_output_path/1" do


this should also probably be benchmarks :)

fixed! ✔️

PragTob · 2018-07-18T19:28:43Z

test/runner/job_test.exs

+
+      compose_config =
+        Job.get_compose_config(job)
+        |> Antidote.decode!()


usually one starts the pipe with a simple value (job in this case)

PragTob · 2018-07-18T19:29:28Z

test/runner/job_test.exs

+
+      assert %{"runner" => %{"environment" => %{"MYSQL_URL" => "root@localhost"}}} = services
+    end
+  end


I was unsure if your changes in build_services were ok in retaining behaviour, but these tests help my confidence - thank you 👍

I can make it more explicit and put the entire config within a pattern match like I did in test "return docker-compose setup for simple job" do, wdyt?

PragTob · 2018-07-18T19:30:46Z

test/runner/job_test.exs

+      assert %{} = Job.format_measurement(nil, "insert_mysql")
+      assert %{} = Job.format_measurement(%{"a" => "b"}, "insert_mysql")
+    end
+  end


where are we testing that we drop all that big measurement data? It's also not quite sure for me where that happens in the code as it's not that explicit, so I think we could improve the code and tests there :)

there is the format_measurements/2 I added a comment on it to explicit say that we are interested in just the statistics data

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

PragTob

Looking good, just a couple things 👏

PragTob · 2018-07-21T12:18:11Z

lib/runner/job.ex

@@ -63,12 +55,11 @@ defmodule ElixirBench.Runner.Job do
    end
  end

-  defp ensure_no_other_jobs! do
+  def ensure_no_other_jobs! do
    %{active: 0} = Supervisor.count_children(ElixirBench.Runner.JobsSupervisor)


I still think we can easily keep this private and test as part of the start_job logic as it raises in the error case so no other logic is actually triggered.

PragTob · 2018-07-21T12:18:44Z

lib/runner/job.ex

@@ -93,11 +84,12 @@ defmodule ElixirBench.Runner.Job do
    end
  end

-  defp get_benchmars_output_path(%Job{id: id}) do
+  def get_benchmars_output_path(%Job{id: id}) do


should be called brnchmarks still with the k :)

✔️ changed

PragTob · 2018-07-21T12:19:57Z

lib/runner/job.ex

+  # Delete key that is not allowed in docker-compose otherwise it will break.
+  # This function iterates recursively over the services map and nested maps to delete the
+  # key for all services.
+  defp delete_non_docker_tag(services, tag) when is_map(services) do


Good name change 👍

PragTob · 2018-07-21T12:20:46Z

test/runner/job_test.exs

+      assert %{log: "success", status: 0} = job
+    end
+
+    test "not start job if there is other job running" do


ah I see you already added the test here you just need to make the function private again I think

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

PragTob

tallysmartins changed the title ~~Test suite~~ Test suite and some refactoring Jul 5, 2018

tallysmartins changed the title ~~Test suite and some refactoring~~ WIP: Test suite and some refactoring Jul 6, 2018

tallysmartins force-pushed the test_suite branch from 60a3824 to 775951b Compare July 10, 2018 20:39

tallysmartins added 2 commits July 15, 2018 22:07

Remove run_times from measurementd results

9200bbc

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

tallysmartins force-pushed the test_suite branch from 775951b to a4f2821 Compare July 16, 2018 01:40

tallysmartins changed the title ~~WIP: Test suite and some refactoring~~ Test suite and some refactoring Jul 16, 2018

tallysmartins changed the title ~~Test suite and some refactoring~~ Some good changes in Job module Jul 16, 2018

tallysmartins changed the title ~~Some good changes in Job module~~ WIP: Some good changes in Job module Jul 16, 2018

tallysmartins force-pushed the test_suite branch from a4f2821 to 5641d8d Compare July 17, 2018 20:13

tallysmartins changed the title ~~WIP: Some good changes in Job module~~ Some good changes in Job module Jul 17, 2018

PragTob reviewed Jul 18, 2018

View reviewed changes

tallysmartins added 2 commits July 20, 2018 01:30

Apply review changes

9b7384e

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

Apply review changes

8587186

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

PragTob reviewed Jul 21, 2018

View reviewed changes

Apply review changes

52fd0f6

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

PragTob approved these changes Jul 25, 2018

View reviewed changes

PragTob merged commit 3b78b27 into elixir-bench:master Jul 25, 2018

tallysmartins mentioned this pull request Aug 4, 2018

[DISCUSSION] Problems when runninig benchmarks with dependencies #5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some good changes in Job module #10

Some good changes in Job module #10

tallysmartins commented Jul 5, 2018 •

edited

Loading

PragTob commented Jul 18, 2018

PragTob left a comment

PragTob Jul 18, 2018

tallysmartins Jul 19, 2018

PragTob Jul 18, 2018

PragTob Jul 18, 2018

tallysmartins Jul 19, 2018 •

edited

Loading

PragTob Jul 18, 2018

tallysmartins Jul 19, 2018

PragTob Jul 18, 2018

tallysmartins Jul 19, 2018

PragTob Jul 18, 2018

tallysmartins Jul 20, 2018

PragTob Jul 18, 2018

tallysmartins Jul 20, 2018

PragTob Jul 18, 2018

tallysmartins Jul 20, 2018

PragTob Jul 18, 2018

tallysmartins Jul 20, 2018 •

edited

Loading

PragTob Jul 18, 2018

tallysmartins Jul 20, 2018

PragTob left a comment

PragTob Jul 21, 2018

PragTob Jul 21, 2018

tallysmartins Jul 22, 2018

PragTob Jul 21, 2018

PragTob Jul 21, 2018

tallysmartins Jul 22, 2018

PragTob left a comment

Some good changes in Job module #10

Some good changes in Job module #10

Conversation

tallysmartins commented Jul 5, 2018 • edited Loading

PragTob commented Jul 18, 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

tallysmartins Jul 19, 2018 • edited Loading

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

Choose a reason for hiding this comment

tallysmartins Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 left a comment

Choose a reason for hiding this comment

tallysmartins commented Jul 5, 2018 •

edited

Loading

tallysmartins Jul 19, 2018 •

edited

Loading

tallysmartins Jul 20, 2018 •

edited

Loading