Proposal: Provide way in umbrella apps to run tests for all sub-apps together

[Myron Marston]

Currently, when you run mix test in an umbrella app, it recursively runs mix test in each child app, so that the tests for app b do not begin running until the tests for a have completed. For our project, we’re making extensive use of the umbrella feature and have a large number of child apps. We decided we’d like to be able to run all the tests for all child apps together. Running all tests together provides a few benefits:

1. It increases the benefit of async: true. A small app with only one or two test modules with async: true cannot fully utilize all cores when the tests are run for just that app, but when run together with all tests in the entire project it can.
2. It makes it harder to wrongly set async: true on a test module that can’t actually be run async. When we got all tests to run together using a new test_all task (see below), we immediately had to deal with a number of new failures due to tests defined in async: true modules that could not actually run async. We had to fix the tests or change async: true to async: false. The problem never occurred when running each apps test suite individually, because the tests that could not run at the same time were from different apps. It’s nice to learn about these issues ASAP (ideally, as soon as the problematic test is defined) instead of later, but it’s easy when each app’s test suite runs individually to not realize the problem — particularly if a sub-app has only one test module (and therefore wasn’t running concurrently with anything!).
3. It makes coverage reporting more accurate. We recently setup coveralls.io using excoveralls, but have noticed some inaccuracies in the reported numbers. For example, we have a test_util app in the umbrella that contains test utilities and helper functions used by many of the other apps. Many of these functions are not directly called in the test_util test suite and as a result they are not counted as covered when we use the excoveralls tasks (which run each child app’s suite independently, like mix test). However, they do get called from tests in some of the other child apps, and should be counted as covered. I believe they would be counted if we were able to run all tests together from the excoveralls tasks.

More generally, running all tests together seems to be in line with the recent changes in 1.2 that allow child apps in an umbrella to share the same config and build directories.

Anyhow, while elixir doesn’t currently provide a simple way to achieve this, I was able to figure a way out. I got it to work using a combination of test_paths and a new task that delegates to Mix.Tasks.Test:

defmodule MyProject.Mixfile do
  use Mix.Project

  def project do
    [
      # ...
      test_paths: test_paths,
      preferred_cli_env: %{ test_all: :test },
      # ...
    ]
  end

  defp test_paths do
    "apps/*/test" |> Path.wildcard |> Enum.sort
  end
end

# `mix test` cd's into each app directory in sequence and runs the tests.
# `mix test_all` runs tests for all apps at the same time.
defmodule Mix.Tasks.TestAll do
  use Mix.Task
  @shortdoc "Runs all delorean tests at once"
  defdelegate run(args), to: Mix.Tasks.Test
end

Note that setting test_paths did not affect mix test at all — since Mix.Tasks.Test is tagged as being recursive, it still runs individually against each child app. So I had to define a new task to make this work.

We’ve been using mix test_all for about a month and it’s working well for us. However, I worked on setting up coveralls.io yesterday and I couldn’t get it to integrate with excoveralls.  Our coveralls tasks are running each child app’s suite independently and reporting less accurate coverage results. Excoveralls allows you to configure a test_task and I tried doing that (via test_coverage: [test_task: "test_all"]) but it led to an error from mix that I'm not sure how to solve:

** (RuntimeError) Trying to access Mix.Project.app_path for an umbrella project but umbrellas have no app
    (mix) lib/mix/project.ex:316: Mix.Project.app_path/1
    (mix) lib/mix/project.ex:337: Mix.Project.compile_path/1
    (mix) lib/mix/tasks/test.ex:163: Mix.Tasks.Test.run/1
    lib/mix/tasks.ex:43: Mix.Tasks.Coveralls.do_run/2
    (mix) lib/mix/cli.ex:58: Mix.CLI.run_task/2

(For more info an the excoveralls accuracy reporting issue and discussion of this error, see parroty/excoveralls#23).

So, my proposal is for elixir to provide a simpler way for umbrella projects to run all child app test suites together — ideally in a way that would “just work” with excoveralls and similar coverage tools.

Thoughts? I’m willing to work up a PR to work on this.

Thanks,
Myron

​

[José Valim]

I think this is a neat idea but I don't think it is a reasonable default for Elixir.

The goal of umbrella apps is to allow applications to be developed side-by-side but still be isolated. The changes for Elixir v1.2 broke part of this when we started sharing configuration but the huge benefits in compilation times were justified.

However, for tests, I am not convinced the added coupling is worth it. For example, take your bullet point 2. It may not be that async was wrongly set, it may simply mean that async tests when running A and B in isolation does not mean they can still be async when ran together. For example, if you have a single test case in A that uses named process "C.Foo" from application C and a single test case in B that uses named process "C.Foo" from application C, you will run into issues, and those will be harder to find because they are across applications boundaries.

Also, for bullet point 1, if your test suite is small, it isn't taking large amounts of time anyway, so making it 4x or 8x times faster may not be noticeable.

I would agree bullet point 3 can be a pain point but it can be solved at the coverage tool too. Maybe we can merge the files after they are run?

Again, I think this is a nice solution for the problems you are currently having and if it works for your team, then go ahead. But for a general solution in Elixir, I don't think the benefits outweigh what it breaks in terms of isolation.

Now to the implementation details.

The reason test_paths does not work is because the test task never runs in the context of the umbrella application as it is marked as @recursive true. Your extra task neatly works around it.

Mix.Project.app_path is not available for umbrellas because umbrellas do not have an app path in _build. So we raise to make it clear. This error is coming from here:

https://github.com/elixir-lang/elixir/blob/master/lib/mix/lib/mix/tasks/test.ex#L163

I think one way to solve this is to:

1. Instead of passing a single directory, we could pass multiple directories to the cover tool. It is a backwards incompatible change but a minor one which can be easily addressed by calling List.wrap on the coverage tool side

2. Once 1 is done, we could now pass a list with the path of the current application (same as today) or a list of all ebin paths for all children applications in the umbrella case. This would effectively solve your problem as you would be able to measure the coverage for all children apps at the same time.

Another option is to deprecate calling start/2 on the coverage tool and simply call start/1. After all, the current project information can be easily retrieved on the coverage side. This would allow us to be backwards compatible by checking if start/1 exists before falling back to start/2. Then the coverage tools could implement the step 2 above instead of relying on Mix to feed them the information.

I think any of those coverage specific improvements would be a nice addition.

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CADUxQmu2DVjB%2BOdhFF7GYYrFc3U7DMXx0JXKdpk6EFixwzcvAA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Myron Marston]

Thanks for the lengthy response, José.

I agree that making this the default doesn't make sense for elixir.  There are different use cases for umbrella apps and running tests all together is a bit less flexible.  I didn't mean to ask for a change to the defaults; I was intending to ask for a simpler way (e.g. an opt-in flag or option) to achieve this behavior.  It took me a bunch of time to figure out the solution I posted above and having an easier, documented solution would be nice.

As for the coverage issue: I haven't dug into how elixir coverage tools work so I can't really comment on whether your suggestions sound good or not.  I'll ask @parroty (author of Excoveralls) to come comment on this thread.

Thanks,

Myron

[parroty]

Hi,

> I would agree bullet point 3 can be a pain point but it can be solved at the coverage tool too. Maybe we can merge the files after they are run? 

I once tried simple merging of coverage reports from individual sub-app as in the following repository, 

  https://github.com/parroty/excoveralls_umbrella

but the merging would become a little tricky when tests in each sub-app are cross-referencing each other & also tests are called separately. It was the original report of the following issue (and I haven't been able to pursue further).

  https://github.com/parroty/excoveralls/issues/23

  https://github.com/ashneyderman/excoverall_issue23

> 1. Instead of passing a single directory, we could pass multiple directories to the cover tool. It is a backwards incompatible change but a minor one which can be easily addressed by calling List.wrap on the coverage tool side

> 

> 2. Once 1 is done, we could now pass a list with the path of the current application (same as today) or a list of all ebin paths for all children applications in the umbrella case. This would effectively solve your problem as you would be able to measure the coverage for all children apps at the same time.

> Another option is to deprecate calling start/2 on the coverage tool and simply call start/1. After all, the current project information can be easily retrieved on the coverage side. This would allow us to be backwards compatible by checking if start/1 exists before falling back to start/2. Then the coverage tools could implement the step 2 above instead of relying on Mix to feed them the information.

I think either option makes it easier to gather the entire sub-apps information and pass them to :cover tool through the unified call from the mix task.

Thanks,

2016年1月7日木曜日 8時38分29秒 UTC+9 Myron Marston: