[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 :)

[Blond11516]

@ZeLarpMaster This issue has to do with the VS Code extensions itself more so than elixir-ls, but I couldn't find a way to handle this automatically (doesn't look like vscode's API exposes which shell is running inside the terminal). I guess a config is the best we could do.

[tomekowal]

I see

tests fail to run with

is already marked as done but I've run into the issue today when getting the newest version.
Removing tmp directories resolved the issue.
From what I understand, the in_fixture macro https://github.com/elixir-lsp/elixir-ls/blob/master/apps/elixir_ls_utils/test/support/mix_test.case.ex#L64 deletes the previous directory and create a new one. But if the directory changed, the old one won't be cleared and that causes the error.
Is deleting the entire tmp directory when starting the tests an option here?

[Blond11516]

@tomekowal

I've run into the issue today when getting the newest version

Just to be sure, by "getting the newest version" you mean that you had cloned the repo and the past and recently tried to pull the latest version? If so, this is expected behaviour.

Is deleting the entire tmp directory when starting the tests an option here?

We did consider this when implementing the fix, but decided that simply moving the tmp folder somewhere else was simpler and cleaner. Jason did however remove the old tmp folder from the gitignore as a signal that it should now be deleted. Maybe it isn't clear enough?

You can read all the discussion regarding this in the relevant PRs: #463 and #482.

[tomekowal]

@Blond11516 Yes, I cloned the repo in the past and upgraded recently. Thank you for pointing out the discussions. I missed the fact that tmp is not in .gitignore.

[Blond11516]

I was playing around with this before writing the documentation and I found a new issue (only remotely relate do the test lenses). It seems that running a specific file from an umbrella's root causes weird dependency resolution failures.

A good example of this is elixir-ls itself. From the repo root, running the command mix test --exclude test --include 'test:test returns all module code lenses' /home/blond/Documents/vscode-elixir-ls/elixir-ls/apps/language_server/test/providers/code_lens/test_test.exs yields this result:

==> elixir_ls_utils
Excluding tags: [:test, {:pending, true}]

== Compilation error in file /home/blond/Documents/vscode-elixir-ls/elixir-ls/apps/language_server/test/providers/code_lens/test_test.exs ==
** (CompileError) /home/blond/Documents/vscode-elixir-ls/elixir-ls/apps/language_server/test/providers/code_lens/test_test.exs:4: module ElixirLS.LanguageServer.Test.PlatformTestHelpers is not loaded and could not be found

** (exit) 1
    (mix 1.12.1) lib/mix/tasks/cmd.ex:64: Mix.Tasks.Cmd.run/1
    (mix 1.12.1) lib/mix/task.ex:394: anonymous fn/3 in Mix.Task.run_task/3
    (mix 1.12.1) lib/mix/project.ex:353: Mix.Project.in_project/4
    (elixir 1.12.1) lib/file.ex:1560: File.cd!/2
    (mix 1.12.1) lib/mix/task.ex:520: anonymous fn/4 in Mix.Task.recur/1
    (elixir 1.12.1) lib/enum.ex:2356: Enum."-reduce/3-lists^foldl/2-0-"/3
    (mix 1.12.1) lib/mix/task.ex:519: Mix.Task.recur/1
    (mix 1.12.1) lib/mix/project_stack.ex:181: Mix.ProjectStack.recur/1

Changing the import of PlatformTestHelpers to an alias (and fixing the related function calls) yields this (partial) result instead:

==> elixir_ls_utils
Excluding tags: [:test, {:pending, true}]
Including tags: [test: "test"]

warning: ElixirLS.LanguageServer.Build.load_all_modules/0 is undefined (module ElixirLS.LanguageServer.Build is not available or is yet to be defined)
  /home/blond/Documents/vscode-elixir-ls/elixir-ls/apps/language_server/test/providers/code_lens/test_test.exs:10: ElixirLS.LanguageServer.Providers.CodeLens.TestTest.__ex_unit_setup_0/1

warning: ElixirLS.LanguageServer.Providers.CodeLens.Test.code_lens/3 is undefined (module ElixirLS.LanguageServer.Providers.CodeLens.Test is not available or is yet to be defined)
Found at 9 locations:
(rest of output omitted)

Everything seems to be fine when running the same command from apps/language_server.

This isn't an issue with the test lenses themselves, but I still find it relevant since it could be fixed by navigating to the proper app folder before attempting to run the test.

Is this a known issue of either the elixir-ls project or umbrella apps in general? I have little experience with umbrella apps but couldn't reproduce this issue with a simple test app.

[lukaszsamson]

@Blond11516 note that in this project there is an alias on mix test. When I comment it out the result is different

  1) test returns all module code lenses (ElixirLS.LanguageServer.Providers.CodeLens.TestTest)
     apps/elixir_ls_utils/Users/lukaszsamson/vscode-elixir-ls/elixir-ls/apps/language_server/test/providers/code_lens/test_test.exs:21
     ** (RuntimeError) failed to start child with the spec {ElixirLS.LanguageServer.Server, nil}.
     Reason: already started: #PID<0.804.0>
     stacktrace:
       (ex_unit 1.12.1) lib/ex_unit/callbacks.ex:433: ExUnit.Callbacks.start_supervised!/2
       (language_server 0.7.0) test/support/server_test_helpers.ex:12: ElixirLS.LanguageServer.Test.ServerTestHelpers.start_server/0
       /Users/lukaszsamson/vscode-elixir-ls/elixir-ls/apps/language_server/test/providers/code_lens/test_test.exs:13: ElixirLS.LanguageServer.Providers.CodeLens.TestTest.__ex_unit_setup_0/1
       /Users/lukaszsamson/vscode-elixir-ls/elixir-ls/apps/language_server/test/providers/code_lens/test_test.exs:1: ElixirLS.LanguageServer.Providers.CodeLens.TestTest.__ex_unit__/2



Finished in 0.1 seconds (0.00s async, 0.1s sync)
8 tests, 1 failure, 7 excluded

Randomized with seed 950028
==> elixir_ls_debugger
Excluding tags: [:test, {:pending, true}]
Including tags: [test: "test returns all module code lenses"]



Finished in 0.00 seconds (0.00s async, 0.00s sync)
0 failures

Randomized with seed 950028
==> language_server
Excluding tags: [:test, {:pending, true}]
Including tags: [test: "test returns all module code lenses"]



Finished in 0.00 seconds (0.00s async, 0.00s sync)
0 failures

@axelson do we really need that alias?

[axelson]

@axelson do we really need that alias?

If we can get rid of the alias, that would be great.

[Blond11516]

If we can get rid of the alias, that would be great.

Well, the result is different, but the tests still don't work, so while it would indeed be great I guess it's gonna have to be a whole different discussion.

[lukaszsamson]

All the issues are addresses