elixir-lang / elixir

Elixir is a dynamic, functional language for building scalable and maintainable applications
https://elixir-lang.org/
Apache License 2.0
24.29k stars 3.35k forks source link

ExUnit should run tests within the scope when called with file:line #13726

Closed azizk closed 2 months ago

azizk commented 2 months ago

Elixir and Erlang/OTP versions

Erlang/OTP 27 [erts-15.0] [source] [64-bit] [smp:16:16] [ds:16:16:10] [async-threads:1] [jit:ns]

Elixir 1.17.2 (compiled with Erlang/OTP 27)

Operating system

NixOS

Current behavior

When running tests with ExUnit and specifying a file:line combination, ExUnit currently runs the test that is closest to the line number by searching upwards. For example, given the following code:

defmodule Test do
  describe "one" do
    test "x" do
    end

    test "y" do
    end
  end
end

If the line number is between the tests x and y, ExUnit runs the test x. Similarly, if the line number is at the end of the defmodule block, ExUnit runs the test y.

If the line number is on defmodule it will exclude all tests.

Expected behavior

When specifying file:line, ExUnit should:

  1. Run the test function if the line number is within its scope (including its header or the end).
  2. Run all tests within the describe block if the line number is within its scope (or its header...), but outside any test blocks.
  3. Run all tests within the defmodule block if the line number is within its scope, but outside of any describe or test blocks.
Examples:
  1. If the line number is between the tests x and y in the example above, ExUnit should run the entire describe "one" block.
  2. If the line number is at the head, the end or a non-test-block, ExUnit should run all tests within the defmodule.
  3. If the line number is outside of the defmodule block, all tests should be excluded.
josevalim commented 2 months ago

We have no plans to change this at the movement. It would be surprising to run all tests in any situation and for the describe case we don’t have the relevant information in the AST to know exactly when it starts and ends. Thank you.

azizk commented 2 months ago

Ah, it's a pity we don't have that info for code blocks. In any case, I think the current behaviour is also surprising and not what I would expect.

josevalim commented 2 months ago

I agree it would be better to run the describe if inside a describe (but never run them all).

To expand a bit more on the problem, we could deal with cases like this:

describe ... do
  # cursor here
end

but the problem is that someone writes their own macro on top of describe, something like this:

defmacro my_describe(do: block)
  quote do
    describe do
      unquote(block)
    end
  end
end

The do-end blocks of the describe does not map to what the user wrote, unless we also start providing a mechanism for passing these forward, which I don't think it is worth it.

azizk commented 2 months ago

I agree it would be better to run the describe if inside a describe (but never run them all).

Yep. But what do you mean by "never run them all"? Sounds contradicting. If you run a describe block, you run all tests inside that block, no?

To expand a bit more on the problem, we could deal with cases like this:

Yes, I see, but maybe it's doable without passing extra information? How does it work when defining property tests? That's a macro wrapping a test case and if I remember right, Elixir knew to only execute the property test when the line number was inside. The macro could possibly inject code before the test block (probably doesn't). Why doesn't the issue exist there as well?

The do-end blocks of the describe does not map to what the user wrote, unless we also start providing a mechanism for passing these forward, which I don't think it is worth it.

Maybe it's wishful thinking, but shouldn't it be able to find the right block after AST expansion, because the line numbers in the AST are not changed? The whole do-end block is merely transplanted with line numbers intact, no?

Besides, I've seen dynamically generated tests like the following and ExUnit manages to figure out how to execute them:

for name <- ["a", "b"] do
  test name do
    assert unquote(name) in ["a", "b"]
  end
end

Currently ExUnit does something even when the tests and describes come from a macro. Maybe it's not terribly difficult to improve upon it and introduce a more logical behaviour?

I think this would be very helpful, especially for editors, otherwise scripts need to be written to figure this stuff on their own (which I did in the ElixirSyntax plugin, and it can never be perfect.)

josevalim commented 1 month ago

“Never run them all” I meant in the case of defmodule.

The reason it works for property test is because we only track the line the test is defined (which in case of macros is the place the macro is called). It is not possible to do that for a block of code.

azizk commented 1 month ago

Okay, I see. I haven't checked the code, but I guess the compiler basically has a flat list of line numbers mapped to describes and tests, e.g. [{2, %Describe{}}, {3, %Test{}}, {10, %Test{}}] and it picks the test or describe block that is closest to the given line number.

I was still hopeful and tried to do something like the following, but that doesn't work unfortunately:

defmacro describe(name, block) do
  quote do
    do_describe unquote(name) do
      unquote(block)
      # do something here
    end
  end
end
azizk commented 1 month ago

@josevalim If I may revive this with a question: why do you think it's bad to run all defmodule tests when the line number is before all test and describe blocks (but inside the defmodule)? Doesn't it make sense to run them all, especially when you have two or more modules in a single test file?