Provide test running code lens

[Blond11516]

Provide code lenses that allow to automatically run tests.

Adds a function to the CodeLens provider which provides the following lenses:

• One lens to run tests for the whole file
• One lens to run each describe block
• One lens to run each test block

Lenses are only provided for files which import the ExUnit.Case module.

VSCode PR: elixir-lsp/vscode-elixir-ls#155

Fixes #386

[Blond11516]

@lukaszsamson I've pushed a new version that fixes most of the issues you brought up in your review. This new version more accurately identifies tests by making sure that the calls are within a module which imports ExUnit.Case and correctly handles files with multiple modules.

I've found one small issue, where if a file contains multiple test modules, when running a test with the mix test file_path:test_line command, it will always run the corresponding test as well as the last test of all previous modules. This appears to be an issue with mix/ExUnit itself, but we might be able to work around it by providing more contextual data to the code lens (ie the module name, the test name, etc.). I've played around with this but couldn't find find a combination of mix test args that fixed this without creating some other problem.

Finally, I want to apologize for the long delay. I've had a lot on my plate lately but I should be able to work on this in a more timely fashion going forward.

[lukaszsamson]

I like the direction it's going. Some small fixes and we can merge it.

I've found one small issue, where if a file contains multiple test modules, when running a test with the mix test file_path:test_line command, it will always run the corresponding test as well as the last test of all previous modules. This appears to be an issue with mix/ExUnit itself, but we might be able to work around it by providing more contextual data to the code lens (ie the module name, the test name, etc.). I've played around with this but couldn't find find a combination of mix test args that fixed this without creating some other problem.

I don't think it's worth it to workaround mix bugs in such cornercases. Better report it upstream.

Finally, I want to apologize for the long delay. I've had a lot on my plate lately but I should be able to work on this in a more timely fashion going forward.

No need to apologize

[axelson]

First off thanks for tackling this ❤️! I haven't done a comprehensive review, but I think this is on a good path and I just have a few initial nitpicky things that I've left comments for.

I've found one small issue, where if a file contains multiple test modules, when running a test with the mix test file_path:test_line command, it will always run the corresponding test as well as the last test of all previous modules. This appears to be an issue with mix/ExUnit itself, but we might be able to work around it by providing more contextual data to the code lens (ie the module name, the test name, etc.). I've played around with this but couldn't find find a combination of mix test args that fixed this without creating some other problem.

Here's an invocation that we can use:

mix test --exclude test --include 'test:test recompiles files recompiles a file'  test/cortex/reloader_test.exs

I think this would be better than relying on the line numbers because it will facilitate re-running the previous test (since the line number may have changed since the test was initially ran).

[axelson]

This probably works well, although an alternative would be to use the same configuration that ex_unit itself is using:

:test_pattern - a pattern to load test files. Defaults to *_test.exs

To be clear I don't think we should change it at this time (unless the current code results in very poor performance or some similar concern).

[Blond11516]

Here's another update! I've had to change the way execution targets are identified to support the new test command syntax that @axelson suggested. I took a bit of work, especially finding a way to find which describe block (in any) a test belongs to, but I think it was worth it because it does indeed make for a much more comprehensive and repeatable test command. As an added bonus, using test names does not appear to suffer from the same inclusion bugs I was talking about earlier.

I've also updated the docs, but I wasn't quite sure what kind of information we wanted to include in there. Suggestions for improvements are very welcome on that front!

I was also wondering what the policy is on tests? From looking at the file structure, I've noticed that only a small part of the project appears to be tested. I'm gonna work on some tests for this anyway, but I still wanted to ask about it.

Finally, I'll open a PR on vscode-elixir-ls to make use of this soon, I just got to review my code to make sure it's clean first.

P.S. Should I resolve the opened conversations on the PR as I address them or do you guys want to take care of that once you've reviewed?

[Blond11516]

Here's the PR for vscode-elixir-ls: elixir-lsp/vscode-elixir-ls#155

[Blond11516]

Also, I was working on unit tests that I'd like to push before this gets merged! I expect to get them done by tomorrow (as well as making sure the pipeline passes), but I seem to be getting some inconsistent errors that I might need help with. I'll keep you posted on that.

[lukaszsamson]

@Blond11516 You can get test_paths and test_patterns like mix does https://github.com/elixir-lang/elixir/blob/281d35e52062c06efcae10b05b3f0c905155e5b3/lib/mix/lib/mix/tasks/test.ex#L400 (probably you will need to iterate over apps in case of umbrella)

[Blond11516]

@axelson I've just tested the lenses with https://github.com/elixir-lsp/example_phx and everything seems to work fine for me, both in test/example_phx_web/controllers/page_controller_test.exs and test/basic_test.exs.

[Blond11516]

I've added some unit tests for this. The tests for the CodeLens.Test provider work fine, but the test I've added for the server seems to cause some errors to pop up inconsistently during test execution. No tests fail, but I get some errors logged to the console.

I've narrowed the error down to the assert_receive call here: https://github.com/Blond11516/elixir-ls/blob/185a0d98c360949789857d3a9960eea1ddff4077/apps/language_server/test/server_test.exs#L641. I have no idea why, but replacing this with a :timer.sleep followed by assert_received appears to get rid of the error. Again, the error is inconsistent, so I can't be 100% certain that this fix works.

Would anyone have any idea what might be going on with this?

Here are the different errors I've seen for reference:

Cannot build without an application name...

22:32:26.108 [error] Process #PID<0.763.0> raised an exception
** (Mix.Error) Cannot access build without an application name, please ensure you are in a directory with a mix.exs file and it defines an :app name under the project configuration
    (mix 1.10.4) lib/mix.ex:392: Mix.raise/1
    (mix 1.10.4) lib/mix/project.ex:693: Mix.Project.ensure_structure/2
    (mix 1.10.4) lib/mix/compilers/elixir.ex:173: Mix.Compilers.Elixir.compile_manifest/9
    (mix 1.10.4) lib/mix/task.ex:330: Mix.Task.run_task/3
    (mix 1.10.4) lib/mix/tasks/compile.all.ex:76: Mix.Tasks.Compile.All.run_compiler/2
    (mix 1.10.4) lib/mix/tasks/compile.all.ex:56: Mix.Tasks.Compile.All.do_compile/4
    (mix 1.10.4) lib/mix/tasks/compile.all.ex:27: anonymous fn/2 in Mix.Tasks.Compile.All.run/1
    (mix 1.10.4) lib/mix/tasks/compile.all.ex:43: Mix.Tasks.Compile.All.with_logger_app/2
    (mix 1.10.4) lib/mix/task.ex:330: Mix.Task.run_task/3
    (mix 1.10.4) lib/mix/tasks/compile.ex:96: Mix.Tasks.Compile.run/1
    (mix 1.10.4) lib/mix/task.ex:330: Mix.Task.run_task/3
    lib/language_server/build.ex:198: ElixirLS.LanguageServer.Build.compile/0
    lib/language_server/build.ex:27: anonymous fn/3 in ElixirLS.LanguageServer.Build.build/3
    (stdlib 3.13) timer.erl:166: :timer.tc/1
    lib/language_server/build.ex:12: anonymous fn/3 in ElixirLS.LanguageServer.Build.build/3
    (kernel 7.0) global.erl:425: :global.trans/4

Compilation error in seemingly random file

22:39:22.043 [error] Process #PID<0.695.0> raised an exception
** (ErlangError) Erlang error: :terminated
    (stdlib 3.13) :io.put_chars(:standard_io, :unicode, ["\n== Compilation error in file lib/language_server/providers/completion.ex ==\n", ["** (MatchError) no match of right hand side value: {:error, :enoent}", 10, "    (elixir 1.10.4) lib/kernel/parallel_compiler.ex:304: anonymous fn/4 in Kernel.ParallelCompiler.spawn_workers/7\n"]])
    (elixir 1.10.4) lib/kernel/parallel_compiler.ex:531: Kernel.ParallelCompiler.wait_for_messages/7
    (elixir 1.10.4) lib/kernel/parallel_compiler.ex:145: Kernel.ParallelCompiler.spawn_workers/3
    (mix 1.10.4) lib/mix/compilers/elixir.ex:195: Mix.Compilers.Elixir.compile_manifest/9
    (mix 1.10.4) lib/mix/task.ex:330: Mix.Task.run_task/3
    (mix 1.10.4) lib/mix/tasks/compile.all.ex:76: Mix.Tasks.Compile.All.run_compiler/2
    (mix 1.10.4) lib/mix/tasks/compile.all.ex:56: Mix.Tasks.Compile.All.do_compile/4
    (mix 1.10.4) lib/mix/tasks/compile.all.ex:27: anonymous fn/2 in Mix.Tasks.Compile.All.run/1
    (mix 1.10.4) lib/mix/tasks/compile.all.ex:43: Mix.Tasks.Compile.All.with_logger_app/2
    (mix 1.10.4) lib/mix/task.ex:330: Mix.Task.run_task/3
    (mix 1.10.4) lib/mix/tasks/compile.ex:96: Mix.Tasks.Compile.run/1
    (mix 1.10.4) lib/mix/task.ex:330: Mix.Task.run_task/3
    lib/language_server/build.ex:198: ElixirLS.LanguageServer.Build.compile/0
    lib/language_server/build.ex:27: anonymous fn/3 in ElixirLS.LanguageServer.Build.build/3
    (stdlib 3.13) timer.erl:166: :timer.tc/1
    lib/language_server/build.ex:12: anonymous fn/3 in ElixirLS.LanguageServer.Build.build/3
    (kernel 7.0) global.erl:425: :global.trans/4

[axelson]

@axelson I've just tested the lenses with https://github.com/elixir-lsp/example_phx and everything seems to work fine for me, both in test/example_phx_web/controllers/page_controller_test.exs and test/basic_test.exs.

Okay, I see what the problem is. I'm running into #193 since I built my version of ElixirLS with 1.8.2 and OTP 21.3. Which means that there isn't anything to fix here.

[Blond11516]

I've been working on checking whether files should be considered based on test_patterns and test_paths. The regular project implementation is simple, but supporting umbrellas is proving to be more complicated.

The solution relies on Mix.Project.in_project, but because we receive multiple code lens requests in rapid succession this causes file system navigation and project loading errors. I seem to understand that the spec code lens generation suffers from a similar problem and has a working solution which could probably be reused.

That said, I think the current PR is big enough, so I suggest tackling this later on. We do currently run the risk of providing code lenses for files that mix won't run, but that would require importing ExUnit.Case and making calls to test/2, test/3 or describe/2 in a non-test file, which feels somewhat unlikely to me.

We could also make this feature opt-in until this is fixed. A setting to allow disabling the feature wouldn't hurt anyway.

If that's alright with you I'll add the new setting and this should be ready to be merged, apart from the test errors I mentioned earlier.

[axelson]

In general I think we should strive to keep the LanguageServer as current working directory agnostic as possible, and instead rely on the project directory setting/state which shouldn't change out from under us.

I like the approach of adding it as an experimental/beta feature and requiring users to specifically enable it. Also, I am definitely agreed about keeping the scope for this PR manageable.

To add the new setting, the setting itself can be added in this PR and elixir-lsp/vscode-elixir-ls#155 should be updated to the document the setting (once I finish #338 this process will be a little cleaner and more structured).

We do currently run the risk of providing code lenses for files that mix won't run, but that would require importing ExUnit.Case and making calls to test/2, test/3 or describe/2 in a non-test file, which feels somewhat unlikely to me.

Agreed

[axelson]

Some minor naming changes. After these changes and the introduction of the new setting (with a default of off) I think this is ready to merge 🎊

[Blond11516]

the setting itself can be added in this PR

Not quite sure what exactly you mean by that. I've added a check to skip the provider unless the new setting is true and added the new setting to elixir-lsp/vscode-elixir-ls#155 package.json. Let me know if there's anything else to do.

[Blond11516]

I just updated the branch with master and that raised a new issue. We now return an error when suggestSpecs is disabled instead of just return an empty list. I've mimicked this behaviour for the test lenses, but the problem is that now as soon as one type of code lens is disabled, both will be because the task will return an error.

Is there a way to return "2 results" from a task? If not, I'm not sure how to make those two processes independent.

[lukaszsamson]

I just updated the branch with master and that raised a new issue. We now return an error when suggestSpecs is disabled instead of just return an empty list. I've mimicked this behaviour for the test lenses, but the problem is that now as soon as one type of code lens is disabled, both will be because the task will return an error.

OK, let's bring back the old behaviour and return {:ok, []} when disabled

[Blond11516]

I've restored the old behaviour. Out of curiosity, what was the rationale behind returning an error when code lenses are disabled? Are we losing much by reverting this?

[lukaszsamson]

I've restored the old behaviour. Out of curiosity, what was the rationale behind returning an error when code lenses are disabled? Are we losing much by reverting this?

My rationale to return error was that if the setting is disabled the server shouldn't get this request.

[Blond11516]

My rationale to return error was that if the setting is disabled the server shouldn't get this request.

I see. Thanks for the info!

In other news, I think I've addressed every comment and request for change! I'll leave it up to you to do a final review and merge. In the meantime I'll have another look over elixir-lsp/vscode-elixir-ls#155 to make sure everything is tidy on that side.

[axelson]

Okay, this is looking good! ❤️
@lukaszsamson want to give a final review as well?

[lukaszsamson]

Looks fine, let's merge it