Add new r.packageTestExplorer command

[juliasilge]

Addresses #6265

This PR adds a new, additional command for running R package tests in the Test Explorer UI, and maps the existing keybinding (from RStudio) to the new command:

@:r-pkg-development

Release Notes

New Features

• New command to run R package tests in the Test Explorer UI. The old command is still available and can be mapped to a preferred keybinding, if desired. (Switch R package testing shortcut to use Test Explorer UI #6265)

Bug Fixes

• N/A

QA Notes

• In an R package, the existing keybinding Cmd+Shift+T should now pop up the Test Explorer UI and run tests there.
• Both the old and new commands should be runnable from the command palette.
• Both the old and new commands should be runnable multiple times in a row.
• Let's also check the command's behavior is sensible if you open and close the workspace.

[github-actions]

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:r-pkg-development

^{readme  valid tags}

[juliasilge]

It takes a little while for the Test Explorer to set up; without any delay, the tests do not run. I did not see a way to check whether the Test Explorer is done setting up; see docs at https://code.visualstudio.com/docs/editor/testing and https://code.visualstudio.com/api/extension-guides/testing. I'm guessing a bit at the delay here but have tested with some real packages and this seems good for an initial go, to get this in front of people for real work.

[jennybc]

I commented the delay out, because I wanted to experience what you describe. When I do so, I see this:

It feels like it's attempting to run Python tests and then determines that no Python test framework is configured.

It's true that the delay results in better behaviour, but it feels like we're papering over something for which there might be a better solution.

I believe what's happening is that workbench.view.testing.focus is causing this first run of controller.resolveHandler:

positron/extensions/positron-r/src/testing/testing.ts

Lines 77 to 91 in bd2d2d3

	// The first time this is called, `test` is undefined, therefore we do full file discovery and
	// set up file watchers for the future.
	// In subsequent calls, `test` will refer to a test file.
	controller.resolveHandler = async (test) => {
	if (test) {
	await loadTestsFromFile(testingTools, test);
	} else {
	await discoverTestFiles(testingTools);
	const watchers = await createTestthatWatchers(testingTools);
	for (const watcher of watchers) {
	context.subscriptions.push(watcher);
	}
	LOGGER.info('Testthat file watchers are in place.');
	}
	};

Specifically, we enter the else branch in which test files get discovered. The time this takes will vary, so a fixed delay doesn't feel great. It feels like we should instrument that in some way that we could determine if it has been completed at least once. I think this is only an issue if the first "touch" of the test explorer comes via the command.

[jennybc]

Yeah I just did an experiment where (1) I've removed the delay and (2) I click on the test explorer beaker before I do my first run of Cmd + Shift + T (i.e. executing r.packageTestExplorer). When I do actually execute r.packageTestExplorer, it executes normally. Presumably because the initial run of controller.resolveHandler() has already completed.

[juliasilge]

Yep, that is exactly what we are dealing with and you are definitely right that the delay wasn't ideal. I did a little more thinking and added an explicit event to listen for, before kicking off the command to run tests.

[juliasilge]

• We don't need to check if R is up, devtools is installed, etc, because the R extension's Test Explorer support already does this.
• I did not map straight from the keybinding to 'testing.runAll' because it doesn't focus the Test Explorer and needs to have the Test Explorer already set up.
• This will not respect TESTTHAT_MAX_FAILS (i.e. related to testthat::set_max_fails() not working anymore #2723) because these tests are run in separate processes IIUC; it wouldn't do us any good to pass this env var through to the test runner. We would need to follow up with additional work to have this approach for running tests respect TESTTHAT_MAX_FAILS.

[jennybc]

It definitely works, but I've made a comment about that fixed delay. It feels like we could/should find a solution that's more correct by definition, i.e. knows we're potentially waiting for test file discovery.

[jennybc]

I commented the delay out, because I wanted to experience what you describe. When I do so, I see this:

It feels like it's attempting to run Python tests and then determines that no Python test framework is configured.

It's true that the delay results in better behaviour, but it feels like we're papering over something for which there might be a better solution.

I believe what's happening is that workbench.view.testing.focus is causing this first run of controller.resolveHandler:

positron/extensions/positron-r/src/testing/testing.ts

Lines 77 to 91 in bd2d2d3

	// The first time this is called, `test` is undefined, therefore we do full file discovery and
	// set up file watchers for the future.
	// In subsequent calls, `test` will refer to a test file.
	controller.resolveHandler = async (test) => {
	if (test) {
	await loadTestsFromFile(testingTools, test);
	} else {
	await discoverTestFiles(testingTools);
	const watchers = await createTestthatWatchers(testingTools);
	for (const watcher of watchers) {
	context.subscriptions.push(watcher);
	}
	LOGGER.info('Testthat file watchers are in place.');
	}
	};

Specifically, we enter the else branch in which test files get discovered. The time this takes will vary, so a fixed delay doesn't feel great. It feels like we should instrument that in some way that we could determine if it has been completed at least once. I think this is only an issue if the first "touch" of the test explorer comes via the command.

[jennybc]

This definitely feels like the right path. But tests no longer seem to run if the test explorer has already been stood up. Presumably because _onDidDiscoverTestFiles.fire() does not fire. We somehow need to rig this to "have we ever discovered test files" vs. "did we just finish discovering test files".

You can experience what I'm talking about in a few ways, but the easiest is probably just to try to use the command / keyboard shortcut a second time. You can make it really clear the tests are not re-running by changing/adding one or clearing all results (behind the ... in the test explorer view).

[juliasilge]

Oh gosh, thank you for noticing that! I was tricked last night by a quirk of one of the test packages I was looking at. I don't believe there is a way to both wait for discovery and check if discovery has happened in one step so in f6c0c1b I set this up so:

• a listener waits for the test discovery setup event to fire
• workspace state tracks whether we have done so already

I tested how this behaves when we close/reopen workspaces and it looks good to me.

[jennybc]

The current state is working as expected for me ✅ I made 2 last comments/suggestions.

[jennybc]

If this is a global workspace state, should we somehow namespace this to positron-r? In Positron Assistant, I see a key like this: 'positron-assistant.recentFiles'.