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

connection to LSP with findTests request and various improvements #184

Merged
merged 26 commits into from
May 9, 2021

Conversation

frawa
Copy link
Contributor

@frawa frawa commented May 2, 2021

Use the new LS request to find tests at load time.
This PR explores the impact of using static analysis, via LS, on the integration with Test Explorer.

Goes with elm-tooling/elm-language-server#583

Open questions:

  • How to trigger the first load request when LS is ready to handle it?

Done

  • match dynamic tests, run results win over loaded tests
  • report errors from run
  • fix handling of multiple elm folders inside a workspace (the current approach does not fit TestAdapterRegistrar semantics)

Todo:

@razzeee
Copy link
Member

razzeee commented May 3, 2021

Tested a bit on elm-review, and it seems like the nesting is a bit buggy.

image

The top one should have been the parent to the one below (and maybe some more, didn't check for that)

@frawa
Copy link
Contributor Author

frawa commented May 4, 2021

Tested a bit on elm-review, and it seems like the nesting is a bit buggy.

image

The top one should have been the parent to the one below (and maybe some more, didn't check for that)

There is a trade-off between static detection of tests and how they are composed during actual execution.
Getting parenting right for cases like

all : Test
all =
    describe "NoListLiteralsConcat"
        [ usingPlusPlusTests
        ]

are somewhat on the edge.

For now, I have no good idea to go after such cases.
Do you?

@razzeee
Copy link
Member

razzeee commented May 4, 2021

I think it should be possible via checker.findDefinition / References.find, depending if your coming from the top or the bottom.

@frawa
Copy link
Contributor Author

frawa commented May 5, 2021

I think it should be possible via checker.findDefinition / References.find, depending if your coming from the top or the bottom.

That opens the path for a lot of cases, as soon as you start de-referencing you slowly get into actually executing the code.
Do you see what I mean?
I do not think that is reasonable.

Let me put more detail on the purpose of this static analysis approach.
The use case it to allow users navigate from the test explorer view (as close as possible) to related test code.

There are certain patterns to write tests that will bring you closer to the test code than others.
Users will decide how to handle the trade-offs.

In the absence of test location information reported from actual test execution, see rtfeldman/node-test-runner#368, using Elm-LS's static analysis feature serve that use cases better already.

The results are more accurate, and way quicker than the current grepping through all test source files.

@razzeee
Copy link
Member

razzeee commented May 5, 2021

I agree and I think you fixed the mayor annoyance for me already. They now group up much nicer and actually running the tests moves stuff around, which I had problems with before.

But there still seems to be weird behavior:

  1. Run all tests
  2. enable auto run
  3. change stuff in a test file
  4. Some grey nodes appear, that won't go away (at least not with the auto run)
    tested on elm review

@frawa frawa marked this pull request as ready for review May 7, 2021 21:28
@frawa frawa changed the title draft connection to LSP with findTests request connection to LSP with findTests request and various improvements May 7, 2021
@razzeee razzeee requested review from razzeee and jmbockhorst May 9, 2021 01:56
@razzeee
Copy link
Member

razzeee commented May 9, 2021

Code looks good to me, I would like to merge this and play with it some more in the coming days.

@razzeee razzeee merged commit e10619e into elm-tooling:main May 9, 2021
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