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
  • Update and change dependencies
  • Change implementation to use new API
    • Rspec
    • Minitest
  • Add useful test stuff like stubs, loggers & mocks, and a unit test suite
  • Convert Rspec test suite to new API
    • Loading tests
    • success
    • fail
    • error
    • skip
  • Convert Minitest test suite to new API
    • Loading tests
    • success
    • fail
    • error
    • skip
  • 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)
  • Add unitTests to CI workflow
  • Write documentation
  • Clean up
  • Manual tests
• Fix CI test
• 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
• 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
• 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!

[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.

[Tabby]

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]

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

[Tabby]

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]

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]

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]

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]

I'm fine either way 👍

[Tabby]

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 >_>