elixir-lang / elixir

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

No warn on assertions outside tests #7683

Closed ggcampinho closed 6 years ago

ggcampinho commented 6 years ago

Environment

Current behavior

I got a question from a co-worker why his tests were failing but the failures had no formatting (no color), and he had something similar to:

defmodule TestTest do
  use ExUnit.Case

  describe "greets the world" do
    assert Test.hello() != :world
  end
end

The output would be:

== Compilation error in file test/test_test.exs ==
** (ExUnit.AssertionError) 

Assertion with != failed, both sides are exactly equal
code: assert Test.hello() != :world
left: :world

    (ex_unit) lib/ex_unit/assertions.ex:330: ExUnit.Assertions.assert/2
    test/test_test.exs:5: (module)
    (stdlib) erl_eval.erl:670: :erl_eval.do_apply/6
    (elixir) lib/code.ex:678: Code.require_file/2
    (elixir) lib/kernel/parallel_compiler.ex:201: anonymous fn/4 in Kernel.ParallelCompiler.spawn_workers/6

Basically, he didn't notice that the assertion was outside of the test block.

Expected behavior

I think we could warn on assertions done outside a function because this is probably not the desired behavior, but I'm not sure if it's the best to do. If you agree, I'm happy to send a PR.

josevalim commented 6 years ago

Yeah, I understand the confusion but theoretically speaking there is nothing wrong with the code. :) --

José Valimwww.plataformatec.com.br http://www.plataformatec.com.br/Founder and Director of R&D

ggcampinho commented 6 years ago

There is nothing wrong but if you try to run and it's correct, you will get a green message with 0 successes. For me, the problem here is that it can be hard to see that you are actually placing your tests in the wrong place, especially if you used a describe, because everything seems to work but it's not the desired behavior.

josevalim commented 6 years ago

Sometimes I perform assertions inside a module inside a test:

test "something"
  defmodule Foo do
    use Something
    assert ...
  end
end

There is no way to restrict the usage above without also restricting valid use cases. The only other option I can think if is you have a describe without tests but I don't think that should be invalid either. So I am more inclined to declare this as "unresolved". :)

ggcampinho commented 6 years ago

I didn't consider this case, you're right :)

adkron commented 6 years ago

The issue that I have here is that after the Compilation Error the other tests stop running. If we get a compilation error and know that we might be in this situation couldn't we provide a better clue as to what the issue is so that people can fix it quickly?