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

fix(test runner): Use ExUnit testPaths and testPattern #500

Conversation

Blond11516
Copy link
Contributor

@Blond11516 Blond11516 commented Feb 28, 2021

First draft of a new way to identify through ExUnit's testPaths and testPattern parameters. For single app projects, we get the values directly from the project configuration.

For umbrella apps, I went with a configuration for now. Should we decide that we want this to be seamless, I thought we could evaluate each app's mix.exs file and execute the project/0 function, but that would imply running client code on the server.

As far as I know, these configurations are rarely changed, so while it is an inconvenience, I don't think it will affect too many people.

I'll start working on tests once I've had confirmation that we want to move forward with this.

Here's an example of what we expect these new configs to look like:

testPattern

{
  "app1": "*_custom_app1_pattern.exs",
  "app2": "*_custom_app2_pattern.exs"
}

testPaths

{
  "app1": ["my_app1_custom_path"],
  "app2": ["my_app2_custom_path"]
}

As of opening, this should fix the last of the issues described in #438.

end

defp get_test_code_lenses(state, uri, source_file, true = _enabled, false = _umbrella) do
file_path = SourceFile.path_from_uri(uri)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this call will fail if URI is not in file: schema (e.g. git:, untitled:). You need to rescue and return []

@lukaszsamson
Copy link
Collaborator

I think we could take a simpler way and only support the default test path and the default _test.exs pattern in umbrella apps.

@Blond11516
Copy link
Contributor Author

Hi @lukaszsamson!

I think we could take a simpler way and only support the default test path and the default _test.exs pattern in umbrella apps.

By "simpler way" I assume you mean the implementation would be simpler? While that is true, I don't feel like the current implementation is that complex (although maybe it could be extracted somewhere else to make it easier to find and work with) and I think it would be unfortunate to leave some (admittedly probably very few) users in the dust.

That said, you're the maintainer and more experienced with the project so I'll remove the functionality if you really believe it's not worth it.

@lukaszsamson
Copy link
Collaborator

That said, you're the maintainer and more experienced with the project so I'll remove the functionality if you really believe it's not worth it.

That was only a suggestion. If you find it's not too costly then feel free to do it :)

@Blond11516
Copy link
Contributor Author

That was only a suggestion. If you find it's not too costly then feel free to do it :)

Well it's already done and I really don't think it adds too much complexity :)

I'll fix your other comment and update the tests then!

@Blond11516 Blond11516 marked this pull request as ready for review April 5, 2021 00:59
@Blond11516
Copy link
Contributor Author

All right, tests are updated, along with a couple bug fixes! @lukaszsamson I think this should fix the last of the issues in #438 on the elixir-ls side.

@lukaszsamson lukaszsamson merged commit de4e4eb into elixir-lsp:master May 22, 2021
vanjabucic pushed a commit to vanjabucic/elixir-ls that referenced this pull request Jul 5, 2021
* fix(test runner): Use ExUnit testPaths and testPattern

* Handle bad URI format

* Add tests for test_paths and test_pattern

* Make all fixture apps unique

* Rename umbrella app to match filesystem

* wait for compilation after tests

* wait for compilation before code lens request
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.

2 participants