[META] Issues with Run test Code Lense

[lukaszsamson]

I played with the new code lense and found a few issues that will need to be sorted out before we can enable it by default.

• server instance id needs to be checked, see Add unique server id #278
  Doesn't work properly in multi-root workspaces Run test command broken in multi-root workspaces vscode-elixir-ls#161
• often clicking run test on test inside describe runs mix test but the intended test is excluded
• in some cases tests defined with ExUnit.CaseTemplate are not found
  Initial investigation shows that elixir_sense often does not return any imports for such tests, e.g.

use MyApp.Web.ConnCase

Edit: this happens when elixir-ls MIX_ENV is not set to test - in that case /test/support is not compiled and elixir_sense is unable to expand the case template macro.
IMO we should drop the imports check as it's too ureliable

• module level run tests is often misplaced somewhere inside module code
• run tests on dirty file should trigger save (or warn if that's not feasible)
• umbrella behaviour is far from optimal - test is run for every app in umbrella
• tests fail to run with

mix test
==> elixir_ls_utils
Excluding tags: [pending: true]


== Compilation error in file test/tmp/ElixirLS.LanguageServer.ServerTest/test returns code lenses for runnable tests/test/fixture_test.exs ==
** (CompileError) test/tmp/ElixirLS.LanguageServer.ServerTest/test returns code lenses for runnable tests/test/fixture_test.exs:1: cannot define module TestCodeLensTest because it is currently being defined in test/tmp/ElixirLS.LanguageServer.ServerTest/test does not return code lenses for runnable tests when test lenses settings is not set/test/fixture_test.exs:1
    (stdlib 3.13.2) erl_eval.erl:680: :erl_eval.do_apply/6
** (exit) 1
    (mix 1.12.0-dev) lib/mix/tasks/cmd.ex:64: Mix.Tasks.Cmd.run/1
    (mix 1.12.0-dev) lib/mix/task.ex:394: Mix.Task.run_task/3
    (mix 1.12.0-dev) lib/mix/project.ex:352: Mix.Project.in_project/4
    (elixir 1.12.0-dev) lib/file.ex:1554: File.cd!/2
    (mix 1.12.0-dev) lib/mix/task.ex:511: anonymous fn/4 in Mix.Task.recur/1
    (elixir 1.12.0-dev) lib/enum.ex:2270: Enum."-reduce/3-lists^foldl/2-0-"/3
    (mix 1.12.0-dev) lib/mix/task.ex:510: Mix.Task.recur/1
    (mix 1.12.0-dev) lib/mix/project_stack.ex:181: Mix.ProjectStack.recur/1

• Crashes when test context spans more than 1 line

** (MatchError) no match of right hand side value: nil
    (language_server 0.6.2) lib/language_server/providers/code_lens/test.ex:93: anonymous fn/5 in ElixirLS.LanguageServer.Providers.CodeLens.Test.find_test_blocks/4
    (elixir 1.12.0-dev) lib/enum.ex:2270: Enum."-reduce/3-lists^foldl/2-0-"/3
    (language_server 0.6.2) lib/language_server/providers/code_lens/test.ex:81: ElixirLS.LanguageServer.Providers.CodeLens.Test.find_test_blocks/4
    (language_server 0.6.2) lib/language_server/providers/code_lens/test.ex:39: ElixirLS.LanguageServer.Providers.CodeLens.Test.code_lens/2
    (language_server 0.6.2) lib/language_server/server.ex:696: anonymous fn/3 in ElixirLS.LanguageServer.Server.handle_request/2
    (language_server 0.6.2) lib/language_server/server.ex:748: anonymous fn/3 in ElixirLS.LanguageServer.Server.handle_request_async/2

reproducible by a test with do on the next line like

test "some test", %{
} do
  end

• Test lens doesn't escape test name properly #485 test lens doesn't escape the test name properly
  I'll update this meta issue with some code that reproduces those issues.
  @Blond11516 feel free if you are interested in tackling any of those.

[wkirschbaum]

2. I found this once, but can't reproduce.
3. In emacs it prompts on dirty files, so maybe editor specific? How would elixir-ls dictate this?
4. I see that mix test runs all the umbrella app's test when filtering on a tests, so possibly a mix test change

[Blond11516]

@wkirschbaum

In emacs it prompts on dirty files, so maybe editor specific? How would elixir-ls dictate this?

elixir-ls simply responds to the LSP Code Lens Request. The exact events that trigger this request are indeed editor specific.

@lukaszsamson I'll try to fix some of these in the coming weeks, I've already got some ideas.

[Blond11516]

module level run tests is often misplaced somewhere inside module code

Should be fixed by #442.

[Blond11516]

2 and 4 were caused by the same thing, so I've closed #442 and opened #443, which fixes them both.

[Blond11516]

I've been looking into changing the method for detecting test files to use test_paths and test_patterns, like ExUnit does, as we discussed in the original PR. This would solve at least the issue with test cases not being identified when not importing ExUnit.Case directly, and might help with fixing the behaviour for umbrella apps (with the added bonus of probably making the implementation simpler).

Handling basic projects is fairly easy, but umbrellas are another story. I've thought of a few possible solutions but none of them feels completely satisfactory (in no particular order):

1. Using Mix.Project.in_project/4 to run Mix.Project.config/0 in each app. Would be the most simple and straight-forward, but sadly doesn't work for us (see here and the following comment by Jason to see why)
2. Manually evaluating the mix.exs file of each app (using Code.eval_file/2 or similar) to call their project/0 function. Probably not too hard, but involves executing client code on the server, which isn't ideal. (Also, I'm not sure how exactly to do this, so some tips would be appreciated if we do decide do this)
3. Manually parsing the mix.exs file of each app with a regex to extract test_paths and test_patterns.
4. Passing the values as a configuration for the extension. Not the most user friendly, but if we make the configuration optional and use the same default as ExUnit, I think this would be transparent for the vast majority of projects.
5. Always assume default values. Even less user friendly, but even more simple. Probably not worth the loss in flexibility compared to 4 imo.
6. Quoting the mix.exs file with Code.string_to_quoted/1 or similar and walking the AST down to the definition of project/0. Feels very overkill and tedious to me, but I have no experience walking ASTs, so this might not be as hard as I imagine.

I've looked around ElixirLS and ElixirSense a bit to see if there would be some existing functionality to help with this, but didn't find anything. I would obviously love if there was indeed something we could use!

Lastly, I think it's important to keep in mind that no solution will perfectly shield us from the most funky project configurations, where test_paths and test_patterns are, for example, sourced from a remote location at startup (far fetched, I know, but never say never right?). Whatever solution we decide to go with, I think it's going to be important that the corresponding limitations are documented.

PS: Sorry for the wall of text 😅

[garthk]

Should be fixed before on-by-default? #485 test lens doesn't escape the test name properly

[ZeLarpMaster]

I have this issue where I'm using VSCode on Windows, but my terminal is using WSL. This means ElixirLS thinks the paths are Windows ones but they're invalid once they reach WSL which makes the generated mix test command crash. I'm not sure what could realistically be done here other than a config to let us force a filepath type, but just wanted to raise this issue :)