Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes tests not compiling after first run #463

Merged
merged 4 commits into from
Feb 8, 2021

Conversation

Blond11516
Copy link
Contributor

Fixes the issue with tests failing to compile after the first run, as described in #438. This was caused by the temporary files created by the fixture mechanism defining the module TestCodeLensTest multiple times in .exs files.

Another easy fix for this would be to delete the "tmp" folder that holds the fixtures on test start. A simple File.rm_rf!("test/tmp") in elixir_ls_utils/test/test_helper.exs does the trick. Not sure which solution is best. (The current solution results in warnings when running the tests because fixture_test.ex does not match the test pattern for elixir_ls_utils. Not a huge deal, but a little annoying)

Another solution would be to get ExUnit to ignore the "tmp" folder, but I couldn't find a way to configure it in this way. test_paths does not seem to handle negative globs.

This is only kind of related, but I was also wondering why it is that some fixtures, "references" for example, are used in some tests without leaving behind a temporary folder? Are the folders cleaned up manually? See here and here for examples of such tests. I am assuming that they are cleaned up automatically, but I couldn't figure out where that happens.

@@ -1136,7 +1136,7 @@ defmodule ElixirLS.LanguageServer.ServerTest do

test "returns code lenses for runnable tests", %{server: server} do
in_fixture(__DIR__, "test_code_lens", fn ->
file_path = "test/fixture_test.exs"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is a good approach. mix test skips .ex files by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it feels a little janky and I'm not sure if it's gonna hold when we add support for ExUnit's test_paths and test_pattern.

The best solution in my opinion would be to delete the elixir_ls_utils/test/tmp folder between test runs, but I don't know if this has any ramifications I don't know about.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far I have not seen any negative consequences of deleting elixir_ls_utils/test/tmp. The other solution I have in mind is moving the temp folder to e.g. elixir_ls_utils/.tmp

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't thought of moving the folder, but that's a pretty good idea! I can't think of any reason why it would cause problems and my limited testing (running mix test a bunch of times in all apps and umbrella root) it seems to work fine.

Copy link
Member

@axelson axelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this 👍 ❤️

@axelson axelson merged commit 69b2254 into elixir-lsp:master Feb 8, 2021
axelson added a commit to axelson/elixir-ls that referenced this pull request Feb 9, 2021
After elixir-lsp#463 was merged, there is nothing actually using the tmp directory
for tests. And any current contributors who pull down master will still
have the issue, so by removing `tmp` from .gitignore it is a signal to
contributors to clean up that directory (and then their tests will work
again).
axelson added a commit that referenced this pull request Feb 13, 2021
After #463 was merged, there is nothing actually using the tmp directory
for tests. And any current contributors who pull down master will still
have the issue, so by removing `tmp` from .gitignore it is a signal to
contributors to clean up that directory (and then their tests will work
again).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants