search

connorshea / vscode-ruby-test-adapter

A Ruby test adapter extension for the VS Code Test Explorer
https://marketplace.visualstudio.com/items?itemName=connorshea.vscode-ruby-test-adapter
MIT License
85 stars 52 forks source link

Convert to native test API #90

Open Tabby opened 2 years ago

Tabby commented 2 years ago

Rewrite the extension to use the new native test API in Visual Studio Code, instead of the Test Explorer Extension API

Why?

Mostly the first one. I think this will allow some massive time savings in only re-checking test files that have changed and updating a subset of the list of tests when a file save event is received, instead of reloading the entire list

Changes

TODO

connorshea commented 2 years ago

Awesome! Do you want a review now (well, now-ish, I definitely won't get to it tonight) or should I wait until it's out of the draft state?

Tabby commented 2 years ago

Wow that's fast! 😁

You're welcome to take a look if you'd like, but I still need to go through the framework specific parts of the test runner and update the tests

I also probably need to put the sorting code back in, during test loading but it's commented for now to keep things simpler until I get it running

I hope that it's ok that I'm rearranging things a fair bit. I'll update the PR description with an in-depth description of the changes once it's closer to being ready as well

The short version is:

I think that's most of it :)

But yeah, as implied by what description is already there I got a bit fed up with it having to reload all the tests every time I changed one and I wanted to fix that. Long story short, the fact that the native API makes it much easier to manipulate the list of tests means this seemed like it was worth doing first

Tabby commented 2 years ago

I guess most of that didn't really answer the question...

No rush at all! I like to make draft PRs early so I can check the diffs, and in case anyone wants to see the in progress work, but it's draft because it's still a fair way off being something I'd put up for review

Tabby commented 2 years ago

I haven't abandoned this. I broke a rib and haven't been able to code much outside of work since, and work itself has been pretty busy. I intend to finish it :)

connorshea commented 2 years ago

I haven't abandoned this. I broke a rib and haven't been able to code much outside of work since, and work itself has been pretty busy. I intend to finish it :)

😱 I hope you're recovering well! No rush, thank you for the update!

atipugin commented 2 years ago

This is what i was looking for :) Get well soon!

Tabby commented 1 year ago

I think it's about time for an update on this :)

I've been working on it a lot lately and it's very nearly at the point where I think it'll be usable, at which point I'll be installing it on my work computer and trying to use it on large repositories for a while. That should turn up most bugs/edge cases I might have missed and then I'll put it up for review.

In terms of remaining work, I need to finish adding a queue for resolving tests from FileWatcher events (because currently it spins up an instance per file changed, which grinds pretty much any computer to a halt on any reasonably large repository when doing things like changing branches or pulling), and I want to do some refactoring so that the classes and object lifetimes and structure are more aligned with the Visual Studio concepts because I think that'll make it easier to understand and reason about, and potentially allow for more concurrency. I also need to document how it all works once that's done.

I expect all this to take a few more weeks at most, assuming life doesn't get in the way

Thanks for your patience all! I hope you'll think it's worth it ^_^

connorshea commented 1 year ago

Awesome, thank you for all the work on it! ❤️

Tabby commented 1 year ago

I've started manually testing this on my work laptop and it seems to be working great so far! The lazy loading alone is a massive improvement, and I've yet to see it become unresponsive 😁

There are still 3 issues I'd like to fix before a release is made with this change in it (in order of priority, as I see it):

Given that this PR is already huge, I'm planning on fixing these in follow up PRs, especially as the last two are non-trivial, but I can fix them in this PR if you want

Assuming they should be done later, barring any major issues that come up in testing, I just need to write docs and see if anything can be cleaned up and then this will be ready

I will try and ensure that all commits from this point do not break anything or cause tests to fail so that people can safely check it out and try it

Tabby commented 1 year ago

Docs added. Manual testing is all that's left. As mentioned, from what I've seen so far, the basics all seem to be working nicely. I need to check that I haven't broken debugging, and that it works on Windows (i.e. that path handling is correct). In the meantime, I think this is close enough that I can mark it as ready for review :D

I also had a look through the open issues to see if any look like they might have been fixed by this. These are the ones I found that I'm pretty sure I've addressed, though not sure if I should update the changelog to say so:

atipugin commented 1 year ago

Can I somehow help with testing? I've installed extension manually from your branch (using "Install from location"), enabled testExplorer.useNativeTesting, but nothing happens :( Am I missing something?

Tabby commented 1 year ago

Can I somehow help with testing? I've installed extension manually from your branch (using "Install from location"), enabled testExplorer.useNativeTesting, but nothing happens :( Am I missing something?

The testExplorer.useNativeTesting setting only affects the Test Explorer UI extension, which is what provided the API that the regular version of this extension uses. This PR is a rewrite of... pretty much everything to integrate with VSCode's native test API directly, which means that it no longer uses or needs the Test Explorer UI extension at all. I've actually uninstalled it in most of my VSCode installs now. So that setting should, at best, have no effect but at worst, might cause issues. I don't think it's likely to be an issue, though I will do some testing to try and check.

That said, as this a pretty big rewrite, I would not be at all surprised if I've messed something up for people with different setups than I have, and while I'd be happy to help, I could really use some more information about how you're using it, and what is happening if that's possible:

atipugin commented 1 year ago

I've built .vsix version and installed it instead. Works like a charm, will give it a shot in my daily work and see if any bugs occur. Thanks for you work!

Tabby commented 1 year ago

I'm very glad to hear it! :D

Thanks for the update, and please do let me know if you run into any issues. I'll be glad to get them fixed

I have already noticed that there seems to be no visual feedback of any kind (no notifications, or anything in the test view) if the extension fails to find any tests and I'm looking into that because that's pretty important

Tabby commented 1 year ago

Seems it would be helpful to add some code to look for Gemfiles in the workspace and set the child process cwd to the location of the shallowest one found that's in the same tree as the configured test folder. It currently doesn't handle, for example, being able to open the workspace for this extension and run the tests in the ruby folder because there's no Gemfile in the workspace root

So something like:

connorshea commented 1 year ago

@Tabby rare edge case, but you can also call it gems.rb instead of Gemfile and that'll work. You should check for that as well.

Tabby commented 1 year ago

@Tabby rare edge case, but you can also call it gems.rb instead of Gemfile and that'll work. You should check for that as well.

Nice, thanks for the tip!

I've added items to the todo list in the description for things I've found that probably need fixing before this is merged.

It also occurs to me that the test runs should probably use the queue as well as the test loading.

~I don't know if it's worth doing that in this PR or doing it as a follow-up. What do you think @connorshea? Currently it just returns if a test run is in progress and another test run is requested~

Actually I was looking into the cancelling of a running test load when needed and I realised that the cancellation handling is a bit of a mess and would probably actually be simpler if I use the queue for test runs too as everything could be handled the same way so I guess I've a bit more refactoring to do 😅

naveg commented 1 year ago

Following up on the performance issue, I notice the problem when I open a spec file but before I run any tests. It seems to run a couple operations for every single test case in the file individually, and some of our spec files have hundreds of test cases. Perhaps there is a way to batch these. Otherwise I worry that the extension will become unusable for very large projects.

I see log entries like for every single test case in the file:

{
  "label": "RubyTestExplorer.TestLoader.loadTests",
  "level": "info",
  "message": "Loading tests...",
  "testIds": [
    "path/to/a/file/foo_spec.rb[1:18:1:2:1:1:4:3]"
  ],
  "time": "2023-01-17T02:40:20.885Z"
}
{
  "label": "RubyTestExplorer.TestRunner.runHandler",
  "level": "info",
  "message": "Creating test run",
  "request": {
    "include": [

... snip ...
Tabby commented 1 year ago

Following up on the performance issue, I notice the problem when I open a spec file but before I run any tests. It seems to run a couple operations for every single test case in the file individually, and some of our spec files have hundreds of test cases. Perhaps there is a way to batch these. Otherwise I worry that the extension will become unusable for very large projects.

Yes, I have also come across this, and have an item in my TODO tracker for it. The issue is the lazy test loading. Every time you expand an item in the tree in the UI, VSC sends a request to the registered resolveHandler to load it, so it does it again when you expand a child that's just been loaded.

I was hoping it wouldn't be a huge problem and I could do so in a follow up PR as I've not yet come across any instances of it being too annoying. I've been testing this on the big monolith repo we have at my work, but while I have plenty of complaints about our codebase, there are not many spec files with that many tests in, and I can absolutely see how this could make it unusable

I can fix this pretty easily I think. I just need to ignore anything that isn't a file in the resolveHandler. That should at least stop it thrashing the CPU. I think that, combined with the file modification watcher which should reload a file on save should actually be ok at ensuring the tree stays up to date

So actually your raising this was super helpful because in explaining the issue I've rubber-ducked myself into realising I was over-complicating the required solution in my head, thinking about file modification time and git commit caches or something 😅

I've pushed a fix. Can you please see if this works better for you @naveg?

atipugin commented 1 year ago

I've noticed that extension doesn't provide any feedback in case if test command fails for some reason. For example, sometimes Gemfile is changed after git pull and running bundle exec rspec fails because you have to call bundle install before.

Go extension simply writes everything to test output and it's pretty obvious what's going on:

image

@Tabby is it possible to do something similar?

Tabby commented 1 year ago

@atipugin thanks for raising that. I had noticed the same and I added an item to the todo list in the PR description to address it. I feel like I mentioned this in a comment somewhere too but I can't find it

The error output is actually now logged at info level after a recent change I made to allow stdout messages from tests to be visible (e.g. puts lines that people sometimes use to help debug simple issues) but it uses a separate log message per line so it's not exactly readable:

{

  "label": "RubyTestExplorer.TestRunner.FrameworkProcess.onDataReceived",

  "level": "info",

  "message": "stdout: LoadError:",

  "time": "2023-01-16T12:07:56.877Z"

}

{

  "label": "RubyTestExplorer.TestRunner.FrameworkProcess.onDataReceived",

  "level": "info",

  "message": "stdout:   dlopen(/home/tabby/git/some-project/.some-project-gemset/gems/cool_library-0.2.3/lib/cool_library/cool_library.bundle, 0x0009): Library not loaded: /usr/local/opt/icu4c/lib/libicudata.71.so",

  "time": "2023-01-16T12:07:56.877Z"

}

{

  "label": "RubyTestExplorer.TestRunner.FrameworkProcess.onDataReceived",

  "level": "info",

  "message": "stdout:     Referenced from: <CD325034-AD67-36FE-BE9A-53A03B16B364> /home/tabby/git/some-project/.some-project-gemset/gems/cool_library-0.2.3/lib/cool_library/cool_library.bundle",

  "time": "2023-01-16T12:07:56.877Z"

}
{
  ...

I plan to try and detect when errors like this happen (using the exit code of the process - in this case it exited with status code 10) to do exactly what you're asking - log it in a single block that's easy to read.

I'd also like to have a toast notification or something to say "Error running test command - please see output" or something so that it's very obvious that it didn't work

naveg commented 1 year ago

@Tabby the latest branch seems much faster! Thanks for fixing that up

Tabby commented 1 year ago

Glad to hear it's an improvement! ^_^

It's still bothering me that it won't run a test while a test load is in progress but I'll fix that when I do the refactoring I want to do soon.

For now, I'd say just be aware that you can cancel the in-progress test load with the little button with a square stop symbol on it at the top of the test pane, in case you need to

I also want to add some caching of which files have been loaded, so they don't get reloaded unless they change, but that's less urgent

connorshea commented 1 year ago

Some small merge conflicts caused by #115, FYI

Tabby commented 1 year ago

Still need to resolve the conflicts but I've applied the same fix so that the behaviour works in both branches at least :) Thanks for the heads up

matteeyah commented 1 year ago

@Tabby Thank you for taking a stab at this. This is amazing!

navels commented 1 year ago

Thanks for all this work! Need more beta testers? Do I check out the PR branch and build the extension and then install it?

Tabby commented 1 year ago

@navels Beta testers would be great, though there's a few known issues to be aware of that I've not had time to fix yet. I think they're all listed in this conversation somewhere, and I've added them to the TODO checklist in the description. I need to move them somewhere more easily readable, but I'd rather just fix them. I'm hoping to soon get some time to make some good progress.

Off the top of my head, major known issues are (in rough order of my prioritisation for fixing them):

Pretty sure those last two are also issues in the main branch, but I want to get them sorted

And yes, to try it just check out this branch and run the following commands from the repository folder to build:

bin/setup
npm run build package

That should give you a .vsix file you can install from VSCode (use the 3 dots menu at the top of the extension sidebar) image

navels commented 1 year ago

Great, I'll give it a go. None of those issues are a blocker for me.

navels commented 1 year ago

Running tests works, but first try to debug a single test seems hung with this in the log:

{
  "label": "RubyTestExplorer.TestRunner.startDebugSession",
  "level": "error",
  "message": "Cannot debug without a folder opened",
  "time": "2023-03-29T17:40:05.608Z"
}
{
  "label": "RubyTestExplorer.TestRunner",
  "level": "info",
  "message": "Running command: {\n  command: 'bundle exec rdebug-ide --host 127.0.0.1 --port 1240 -- $EXT_DIR/debug_rspec.rb --require /home/lee.nave/.vscode-server/extensions/connorshea.vscode-ruby-test-adapter-0.10.0/ruby/custom_formatter.rb --format CustomFormatter',\n  args: [Array]\n}",
  "time": "2023-03-29T17:40:05.609Z"
}

FYI I am using the extension in a docker container through the Dev Containers extension.

Stop button no workie, extension claims to be running tests, had to reload vscode and open a shell in the container and kill the still-running rdebug-ide process.

Tried again, same result.

Tabby commented 1 year ago

Thanks for the report! I've still yet to check debugging tests, or running in a Dev container at all actually (same goes for any other kind of remote workspace) so either of those might just be broken at the moment, but I'll make a note to look into it as they're all things that will need to work :)

navels commented 1 year ago

Is it straightforward to run/debug the extension itself from vscode? I'd be happy to help.

Tabby commented 1 year ago

Is it straightforward to run/debug the extension itself from vscode? I'd be happy to help.

Good question >_>

I'm not actually sure. I generally try and write unit/integration tests for things that aren't working, get them passing and then look at logs while trying to use the extension normally to figure out if it's doing the right thing. I've not actually tried running/debugging it from the project, largely because then I can test while doing my day job.

I can have a look later and try it, and write up some instructions if that's helpful? And if you want to and can help then please do :D. Though the next change I need to make will be a potentially quite big refactor, so it might be best to wait until that's in, and I will try and get that done this week

navels commented 1 year ago

Gotcha, gotcha . . . I will try to get up to speed on either approach but won't try to do much with it given a refactor is coming. And I also have a day job that is keeping me particularly busy atm so take your time responding to my questions, I am not spinning my wheels waiting to work on this :-)

Tabby commented 1 year ago

I can make a slack/discord for coordinating on this (unless @connorshea would rather do so) as that might be easier than trying to communicate in this thread

What do y'all think?

navels commented 1 year ago

I was thinking the same. Prefer slack over discord but either works, just that I can go for days without remembering to open Discord :-)

connorshea commented 1 year ago

I'm fine either way 👍

Tabby commented 1 year ago

I've made a Slack for this, invite link is https://join.slack.com/t/slack-ozc7095/shared_invite/zt-1to8gtadd-n~RYAg6XZmp4gfczRFdryw

I'm also going to create Issues (in my fork) for the various major changes that need doing on this PR so separate out discussions a bit more and make it easier to find relevant context. I'll make sure to detail what my thoughts are on how I was planning on approaching each one, but if others have ideas on how to do things better then we can totally discuss them. I'm not arrogant enough to think my plans are going to be perfect :P

And lastly, I'm hoping to get at least the worst parts of the refactoring I want to do done today so that it's in a better place for having more collaborators which it needs because I'm kinda burning out a little on this but I really want to see it through >_>

Splines commented 2 weeks ago

Hey, that's amazing, would love to see this shipped such that the native Test API is used! Are there any updates? At least, we now have the native Test UI since vscode-text-explorer uses the native API as of version 2.22.0 which was released 6 hours ago.

I've still yet to check debugging tests, or running in a Dev container at all actually (same goes for any other kind of remote workspace) so either of those might just be broken at the moment, but I'll make a note to look into it as they're all things that will need to work :)

At MaMpf, I've written this Python file. We use the following setting to point the extension to this file and execute it for the tests:

"rubyTestExplorer.rspecCommand": "python3 ./spec/rspec_inside_docker.py"

It works flawlessly for us with proper error reporting if tests fail.

Tabby commented 1 week ago

Hey, that's amazing, would love to see this shipped such that the native Test API is used! Are there any updates? At least, we now have the native Test UI since vscode-text-explorer uses the native API as of version 2.22.0 which was released 6 hours ago.

I've still yet to check debugging tests, or running in a Dev container at all actually (same goes for any other kind of remote workspace) so either of those might just be broken at the moment, but I'll make a note to look into it as they're all things that will need to work :)

At MaMpf, I've written this Python file. We use the following setting to point the extension to this file and execute it for the tests:

"rubyTestExplorer.rspecCommand": "python3 ./spec/rspec_inside_docker.py"

It works flawlessly for us with proper error reporting if tests fail.

I've not had a chance to work on this for a while as I got kinda burned out and couldn't get any clear feedback on what needed to be done for it to be considered mergeable, but I agree that it would be great to get it finished off. I'll try and carve out some time to make some more progress on it soon :)

Splines commented 1 week ago

No worries, your health is for sure top priority. Take your time.