R: Test explorer improvements

[jennybc]

In anticipation of merging #1763 in the near-ish future, I'm opening this issue to track TODOs that are already on my mind, but that won't be part of #1763. It's also a fine place to put feedback as people use the testing feature.

• When our posture towards multi-root workspaces matures, revisit handling of that in the test explorer. The current code does (sort of) work with a multi-root, multi-package workspace, but I've made no special effort to, e.g., clearly delineate the tests from the different packages. If we decide to support this officially, I suspect we should introduce one more level in the test hierarchy: instead of test file > test case, do package > test file > test case.

  This additional level of hierarchy may also be important for rationalizing the way we run the entire test suite even when working with a workspace that is a single package (see Adapt vscode-r-test-adapter #1763 (comment)).

  OTOH I suspect, even if we support multi-root workspaces, we will decide that only 1 package can be active as a target for package-development tasks. Update: I have definitely decided that the test explorer should be single-package only for now and probably forever. I have also concluded it is untenable to introduce a whole package as a concrete test item.

• When we develop an official notion of project type, this code should check for a package and early exit if not a package. If the project is a package, I would expect to receive metadata, such as the package name and its top-level root directory (which is often the workspace folder, but not always). This could then be incorporated in TestingTools, making that info available to lower-level functions in the testing facility, and removing the need for some janky filepath operations. Now done in Second wave of R test explorer improvement (eg better "run all tests") #1894, at least at a "first pass" level.

• Revisit the construction of the ID for a testItem that is a file. Feels like it should share a prefix/stem with the ID for a testItem that is a test case within the file. Done in Second wave of R test explorer improvement (eg better "run all tests") #1894.

• Rebuild tree-sitter-r.wasm from the same grammar used in ARK. Adapt parsing code accordingly. This is now Update tree-sitter-r.wasm in positron-r #2632.

• Call match.call() on test_that() calls to truly resolve desc vs code? This is now Use match.call() when parsing test files and finding the test identifier (desc) #2633.

• Level up the describe() treatment once testthat supports running a single describe(). Update: happened in Support running a single describe() test r-lib/testthat#1903. This is now Provide more support for describe() tests in the test explorer #2634.

• Switch to more general machinery for checking the presence and version of necessary R packages, once that exists.

• See if we can make single clicks on a test file or a test case do something more immediate. More details in https://github.com/posit-dev/positron/pull/1763/files#r1389563485. Preliminary research suggests the testing API does not support this, but co-pilot was able to suggest a workaround: set a flag in createRunProfile() that helps the resolveHandler() distinguish whether it was called due to a test run request or for some other reason (presumably mouse click?). Haven't tried that yet. Part of UI improvements for R test explorer #2636 now.

• Improve display of test failures. See https://github.com/posit-dev/positron/pull/1763/files#r1389571071 and https://github.com/posit-dev/positron/pull/1763/files#r1389572783 and https://github.com/posit-dev/positron/pull/1763/files#r1389604417. Part of UI improvements for R test explorer #2636 now.

• The Debug Tests button isn't terribly useful in R, where we can walk through our tests easily in the REPL. Can I remove the button or otherwise mitigate its presence? See https://github.com/posit-dev/positron/pull/1763/files#r1389605966. Part of UI improvements for R test explorer #2636 now.

• It would be nice to be able to execute clickable suggestions, e.g. testthat::snapshot_accept(). See https://github.com/posit-dev/positron/pull/1763/files#r1389618442. Part of UI improvements for R test explorer #2636 now.

• There should be no need to explicitly run devtools::load_all(). I think every test-running function we call handle this internally. Check this and, if true, edit the command run in the child process. Not even sure what I meant by this any more. We do call load_all() on the embedded test reporter package, but that is also necessary.

[jennybc]

When there is no current testing view, we currently see this:

If the workspace is an R package, I think we'd like to not see the contribution from the positron-python package here:

https://github.com/posit-dev/positron-python/blob/98da4f51910f36c1d898a96461acdf19b662d19c/package.json#L2028-L2032

but I think that would require some work/coordination in positron that provides the positron R and Python extensions some notion of "this is an R project" vs. "this is a Python" project, for use in the when clause.

This is now #2637.

[jennybc]

• Ask the running R runtime about its locale and then match that in the child process used to run tests

Update: done in #2488

[jennybc]

readr uses testthat but does not have a tests/testthat.R file. Instead it has tests/first_edition.R and tests/second_edition.R. This violates the current test for whether a package uses testthat:

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

Lines 82 to 87 in 9b349bd

	const dotRPattern = new vscode.RelativePattern(packageRoot, testthatDotRPattern);
	const testthatDotR = await vscode.workspace.findFiles(dotRPattern, null, 1);
	if (testthatDotR.length === 0) {
	Logger.info('tests/testthat.R not found');
	return;
	}

• Revisit refreshTestthatStatus() and the way we set the testthatIsConfigured context key. I suppose the check should align with what I do in usethis:

  uses_testthat <- function() {
    paths <- proj_path(c(path("inst", "tests"), path("tests", "testthat")))
    any(dir_exists(paths))
  }
  

Despite this fact, the test explorer still works in readr. This is nice, I guess, but is also somewhat surprising. Rationalize that when I come back to do 👆

Now part of #2638.

[jennybc]

• Figure out how to initialize the parser just once.

The intent already is to initialize the tree-sitter-r parser exactly once:

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

Lines 84 to 86 in 9b349bd

	if (parser === undefined) {
	parser = await initializeParser();
	}

But if the first use of the parser coincides with a full test run, and all the test files are getting parsed for the first time, it appears to effectively result in one initialization per test file:

2023-12-04 09:55:07.706 [info] Test run started
2023-12-04 09:55:07.706 [info] Running all tests
2023-12-04 09:55:07.706 [info] Test type is directory
2023-12-04 09:55:07.706 [info] Parsing test file file:///Users/jenny/rrr/readr/tests/testthat/test-parsing-character.R
2023-12-04 09:55:07.706 [info] Initializing parser
2023-12-04 09:55:07.707 [info] Parsing test file file:///Users/jenny/rrr/readr/tests/testthat/test-utils.R
2023-12-04 09:55:07.707 [info] Initializing parser
2023-12-04 09:55:07.707 [info] Parsing test file file:///Users/jenny/rrr/readr/tests/testthat/test-collectors.R
2023-12-04 09:55:07.707 [info] Initializing parser
2023-12-04 09:55:07.707 [info] Parsing test file file:///Users/jenny/rrr/readr/tests/testthat/test-read-lines.R
2023-12-04 09:55:07.707 [info] Initializing parser
...

The parsing is either happening literally in parallel or in such close succession that it is effectively in parallel. This doesn't seem to cause a user-perceivable problem, but it still seems worth correcting.

Now part of #2638.

[jennybc]

Slack feedback from @aronatkins:

I was just experimenting with running package tests in Positron and one of the tests encountered an error. When I used devtools::check(remote = TRUE, manual = TRUE) in the console or when run in RStudio, I got an error and its traceback. Is there a way to see the full traceback in the Positron test view? (for the curious: rsconnect bioc local test failures again)

If you’re ever looking for a package that just doesn’t work at all when its tests are run in Positron, look to packrat. Oy. There’s probably all sorts of local filesystem shenanigans that require very specific test ordering…

[jennybc]

Something I've noticed and was also raised by @hfrick in Slack:

• Do something (more) sensible with the message associated with a skipped test.

I skip some tests and noticed that the reasons for the skip (= the text inside of skip(), e.g. skip("because I haven't implemented this yet")) are smashed together without a space or line break. Can I wish for a line break?

Update: usethis is a good example for seeing this behaviour:

Reason: Not on GitHub Actions, Travis, or AppveyorReason: Not on GitHub Actions, Travis, or AppveyorReason: Not on GitHub Actions, Travis, or Appveyor

Fixed in #2796

[jennybc]

Another observation from @hfrick:

Positron seems to struggle with single quotes in test description.

test_that("this test without single quotes will pass", {
  expect_identical(TRUE, TRUE)
})

test_that("this test with 'single quotes' will error in Positron", {
  expect_identical(TRUE, TRUE)
})

Later addition: @hadley is also seeing something odd with test that use backticks in their description.

This is now #2639.

[jennybc]

Raised by @hadley in slack.

• Determine the connection between "Test: Run Tests in Current File" and the testing API and our test explorer.

Update: "Test: Run Tests in Current File" does already work if and only if the test files have been loaded into the controller. So, if you've already interacted with the test explorer (the beaker), all is well.

Takeaway: maybe my listener for test file creation and modification should expand to include opening a test file in an editor.

Now part of #2638.

[jennybc]

Reported by @andrie in slack:

Testing:
Testing using the VS-code testing pane on Windows still throw errors, based on incorrect handling of Windows file paths, e.g:

Error: ℹ Loading vscodereporter
Error: '\U' used without hex digits in character string (<input>:1:32)
Execution halted

Update: this was fixed in #2194

[jennybc]

This issue was, unofficially, a huge epic. Closing now that I've opened the remaining items as individual issues.