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

Smoother main and test discovery #2532

Merged
merged 6 commits into from
Mar 8, 2021
Merged

Smoother main and test discovery #2532

merged 6 commits into from
Mar 8, 2021

Conversation

ckipp01
Copy link
Member

@ckipp01 ckipp01 commented Feb 20, 2021

What this is

This pr introduces some new functionality around run/test/debug. My main goal with this is to enable clients to quickly be able to run or test based off of where they are, and that's it. This is especially useful for users without code lenses or useful in the case where a user doesn't want to go back to the file with the main in to hit run, or scroll all the way back up to the top of a file to hit test, or even if they want an easy way to test an entire build target.

How does a client use this

This is accomplished by introducing a new params object for the debug-adapter-start where all a client has to send in is the current file it's in, and the runType which can be run, testFile, or testTarget. Env, args, and options are all still possible, but not necessary. Ideally a client will have these three different run types mapped to a command. You can see a very rough example of this in nvim-metals since that's how I was testing this.

Some examples

In the case where there is just a single main method (in the build target the file belongs to), it will just be ran. You don't even have to be in that file, just the build target. If there are no mains in your build target, you'll be warned.

2021-02-20 12 52 36

In the case of multiple mains in a single build target, you'll be prompted to choose which you'd like to run.

2021-02-20 13 01 19

The testFile option will check to see if you're currently in a test suite. If so, it runs it, if not you will get a warning about no tests being found in your file. If you use the testTarget it will run all the suites in that target. If there are no tests in that target, you'll get a warning.

2021-02-20 13 08 49

What's left

In this pr I'd still like to get a couple things done, but before I do I want to make sure you're on board with this direction. After that I need to make sure

  • Debug docs are updated

Closes #2491

@ckipp01 ckipp01 force-pushed the dap branch 14 times, most recently from 073bb5e to 20267c0 Compare March 2, 2021 18:38
Now it's possible to just give a `runType` and a `path` argument when
calling `debug-adapter-start`. The possible `runType`s are `run`,
`testFile` and `testTarget`.
@ckipp01 ckipp01 force-pushed the dap branch 2 times, most recently from b6332fd to 042b6b8 Compare March 3, 2021 19:32
@ckipp01 ckipp01 marked this pull request as ready for review March 3, 2021 19:33
@ckipp01
Copy link
Member Author

ckipp01 commented Mar 3, 2021

Alright, I think this should now be ready to review. I ended up keeping the separate params mainly because I think it is way cleaner than accounting for everything being nullable. I tried that and it got messy pretty quickly. Plus, this allows for clients to start to use this without worrying at all about any existing implementation or things breaking as the others work fully the same.

I figured out the tests, and it was just a timing thing, and I need to wait a bit to ensure that the classes are cached and then the tests work. However, for windows I had major issues getting the URI right. Finding a clean way to always send in the correct URI proved to be way harder than anticipated and I kept getting a mixture of file:///D:\blah\blah/blah/blah.scala with mixed /. I couldn't figure out a clean way to do that, so if I am missing an obvious way, please let me know. For now I just marked them as .flakyWindows. Not ideal, but I was sick of wrestling with it.

I think this is a good foundation to start actually using the discovery and then fine tuning. Let me know what you think.

@ckipp01 ckipp01 requested a review from tgodzik March 3, 2021 19:37
Copy link
Contributor

@tgodzik tgodzik 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 digging into the failures! That was a lot of debugging from what I've seen in the emails 😨

docs/integrations/debug-adapter-protocol.md Outdated Show resolved Hide resolved
docs/integrations/debug-adapter-protocol.md Outdated Show resolved Hide resolved
docs/integrations/debug-adapter-protocol.md Outdated Show resolved Hide resolved
docs/integrations/debug-adapter-protocol.md Outdated Show resolved Hide resolved
tests/unit/src/test/scala/tests/DebugDiscoverySuite.scala Outdated Show resolved Hide resolved
@ckipp01
Copy link
Member Author

ckipp01 commented Mar 4, 2021

Woo, all ✅ on the window test!

@ckipp01 ckipp01 requested a review from tgodzik March 5, 2021 16:30
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@ckipp01 ckipp01 merged commit 3ada84f into scalameta:main Mar 8, 2021
@ckipp01 ckipp01 deleted the dap branch March 8, 2021 14:51
tgodzik added a commit to tgodzik/metals-vscode that referenced this pull request Mar 12, 2021
Thanks to @ckipp01 it's now possible to run test or main classes inside the current file.

The Metals PR that added this functionality is scalameta/metals#2532
tgodzik added a commit to tgodzik/metals-vscode that referenced this pull request Mar 12, 2021
Thanks to @ckipp01 it's now possible to run test or main classes inside the current file.

The Metals PR that added this functionality is scalameta/metals#2532
tgodzik added a commit to tgodzik/metals-vscode that referenced this pull request Apr 6, 2021
Thanks to @ckipp01 it's now possible to run test or main classes inside the current file.

The Metals PR that added this functionality is scalameta/metals#2532
tgodzik added a commit to tgodzik/metals-vscode that referenced this pull request Apr 6, 2021
Thanks to @ckipp01 it's now possible to run test or main classes inside the current file.

The Metals PR that added this functionality is scalameta/metals#2532
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.

Smoother main discovery when no mainClass is given.
2 participants