Convert to native test API

[Tabby]

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

Why?

• The new API gives a lot more flexibility; e.g. the list of tests can be built incrementally or edited in place
• Better cancellation using cancellation tokens
• No new features will be added to Test Explorer extension. Native API is more likely to be maintained long-term and potentially will get new features and performance benefits
• Removes the need for another extension

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

• adapter.ts is going. Instead you create and subscribe to a TestController and one or more (2 in this case, run and debug) run configurations. The run configurations each get passed a lambda on construction which is called when the user runs/debugs the tests
• controller has a field called items which replaces the root node and should be populated after subscription. controller.items is a TestItemCollection (essentially a TestItem[] but more limited, and presumably the methods available are thread safe)
• TestInfos have become TestItems, and so have TestInfoSuites as TestItem has a children field which is also a TestItemCollection so we no longer need to distinguish between tests and suites
• A factory is now used to recreate the test loader and runner on startup and if the framework in use changes
• More use of disposables, e.g. to ensure child processes get killed
• Tests are now called via a run handler that gets a request object with a list of tests to include and exclude (if include is empty, run all tests), a TestRun object for signalling test statuses instead of the events that were fired before, and a cancellation token which should make cancelling easier

TODO

• [ ] Convert extension to use native test API
  • [x] Update and change dependencies
  • [x] Change implementation to use new API
    • [x] Rspec
    • [x] Minitest
  • [x] Add useful test stuff like stubs, loggers & mocks, and a unit test suite
  • [x] Convert Rspec test suite to new API
    • [x] Loading tests
    • [x] success
    • [x] fail
    • [x] error
    • [x] skip
  • [x] Convert Minitest test suite to new API
    • [x] Loading tests
    • [x] success
    • [x] fail
    • [x] error
    • [x] skip
  • [x] Add queue to prevent spawning unlimited rspec/minitest instances due to lazy loading (ideally in a subsequent PR test files can be parsed with an AST parser to get test information rather than doing dry runs)
  • [x] Add unitTests to CI workflow
  • [x] Write documentation
  • [x] Clean up
  • [ ] Manual tests
• [x] Fix CI test
• [x] ~Refactor test runs to also use the queue and simplify cancellation handling~ Allow test loads to happen in parallel to test runs by not using a TestRunProfile for them
• [x] Improve logging of errors if test process fails to run any tests (e.g. dependencies not installed)
• [ ] Notification if no tests found
• [ ] Detect gemfile/gems.rb location when needed for setting cwd of test process
• [x] Update logger when configured log level is changed

[connorshea]

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]

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:

• adapter.ts is going. Instead you create and subscribe to a TestController and one or more (2 in this case, run and debug) run configurations. The run configurations each get passed a lambda on construction which is called when the user runs/debugs the tests
• controller has a field called items which replaces the root node and should be populated after subscription. controller.items is a TestItemCollection (essentially a TestItem[] but more limited, and presumably the methods available are thread safe)
• TestInfos have become TestItems, and so have TestInfoSuites as TestItem has a children field which is also a TestItemCollection so we no longer need to distinguish between tests and suites
• A factory is now used to recreate the test loader and runner on startup and if the framework in use changes
• More use of disposables, e.g. to ensure child processes get killed
• Tests are now called via a run handler that gets a request object with a list of tests to include and exclude (if include is empty, run all tests), a TestRun object for signalling test statuses instead of the events that were fired before, and a cancellation token which should make cancelling easier

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]

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]

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]

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]

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

[Tabby]

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]

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

[Tabby]

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):

• Can't configure multiple test folders
• RSpec contexts don't have proper descriptions, only a location in a file (e.g. foo_spec.rb[1:2:3])
  • Worth pointing out that they do now at least show in the list as containers with the specs/contexts they contain as children
• After loading tests from a file, when a context in that file is expanded in the list it gets loaded again

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]

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:

• Extension reloads all tests on startup and editing test files - Fixed by lazy test loading and load queue
  • #36
  • #52
  • #57 - maybe not fixed, not much info but seems likely to be about test loading
• Extension hangs after failure to load tests - Hang should be fixed by rewrites of the test loading code. Causes of the failures that are due to the extension itself probably aren't fixed though
  • #107
  • #106
  • #70
• #84 - should be fixed by native API integration, as the option is no longer needed

[atipugin]

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]

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:

• Is there any log output in the "Ruby Test Explorer log" channel? (If there are, but just not saying anything helpful, try changing the rubyTestExplorer.logLevel setting to "debug")
• Is there any relevant log output in the VSCode extension host log channel?
• Are you opening a workspace/folder, just some ruby files, something else?
• Are you using a dev container, remote workspace, VSCode server/hosted on the web or anything?
• What platform is it running on (Windows/Linux/Mac, amd64/arm64/armv7, etc), if you know?

[atipugin]

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]

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]

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:

• workspace root
  • someFolder
    • Gemfile <-- it should pick this one, if the test dir is someFolder/specs
    • specs
      • some_spec.rb
  • someOtherFolder
    • Gemfile
    • ...

[connorshea]

@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]

@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]

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]

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]

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:

@Tabby is it possible to do something similar?

[Tabby]

@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]

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

[Tabby]

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]

Some small merge conflicts caused by #115, FYI

[Tabby]

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]

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

[navels]

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]

@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):

• Can't run tests while it's loading tests (workaround: use the square "stop" button on the test sidebar to stop loading so you can run them)
• If a spec causes multiple errors/failures, the RSpec::Core::MultipleExceptionError wrapper is displayed in the UI, not any of the actual errors/failures
• Doesn't handle cases where Gemfile is not in project root
• Probably doesn't work on Windows without WSL (yet)
• Can't have multiple test folders configured (yet)

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)

[navels]

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

[navels]

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.