Adapt vscode-r-test-adapter #1763

jennybc · 2023-11-02T05:18:08Z

Addresses #1365

jennybc · 2023-11-02T05:19:58Z

extensions/positron-r/package.json

@@ -448,7 +449,9 @@
  },
  "dependencies": {
    "p-queue": "^6.6.2",
-    "vscode-languageclient": "^7.0.0"
+    "split2": "^4.2.0",


The split2 module is used to stream testthat results back into the testing machinery in the R extension. I think it is probably a new dependency worth taking.

jennybc · 2023-11-02T05:20:38Z

extensions/positron-r/package.json

-    "vscode-languageclient": "^7.0.0"
+    "split2": "^4.2.0",
+    "vscode-languageclient": "^7.0.0",
+    "web-tree-sitter": "^0.20.8"


We need web-tree-sitter in order to use tree-sitter-r.wasm, which is used to parse test files.

extensions/positron-r/src/extension.ts

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

jennybc · 2023-11-02T05:51:00Z

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

+						testStartPosition: toVSCodePosition(match.captures[0].node.startPosition),
+						testEndPosition: toVSCodePosition(match.captures[0].node.endPosition),
+					});
+				} else {


This branch is for tests that use describe() + it() (as opposed to test_that()). This is dramatically less common than test_that() and I don't think we should wait for full support for describe() + it() to merge this.

I have discovered that the current query cannot handle

an it() statement without a code body

nested describe()

These are fairly edge case-y, within the relatively small usage of the describe() + it() style. But I will record this in some todo list for this testing feature. Why did I even try them? They both appear in the examples for describe().

As it stands testthat can't run individual describe() tests anyway. That feature was only introduced for test_that() tests. I will also open an issue about that in testthat.

jennybc · 2023-11-02T06:03:52Z

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

+	return testFound;
+}
+
+async function getRBinPath(testingTools: TestingTools) {


This should probably become a utility function higher in the extension, since we're doing something similar here and, e.g., in some of the other R package tasks.

jennybc · 2023-11-02T06:04:50Z

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

+		`devtools::load_all('${testReporterPath}');` +
+		`devtools::${devtoolsMethod}('${cleanFilePath}',` +
+		`${descInsert}reporter = VSCodeReporter)`;
+	const command = `${rBinPath} --no-echo -e "${devtoolsCall}"`;


The --no-echo part is extremely important (ask me how I know 😬 ), if you want to easily ingest output from calling R like this. I think everywhere we do this we should probably use --no-echo or, at least, --no-save (which --no-echo implies).

jennybc · 2023-11-02T06:09:06Z

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

+	// TODO: check workspace folder(s) for package-hood
+	// if no package, return
+	// if exactly one package among the workspace folders, proceed
+	// consider adding package metadata to testingTools (eg name and filepath)
+	// if >1 package, consult our non-existent policy re: multi-root, multi-package workspace


In due course, this is the right place to use some notion of "what kind of workspace is this?", which doesn't really exist yet.

jennybc · 2023-11-02T06:09:59Z

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

+
+export enum ItemType {
+	File = 'file',
+	TestCase = 'test',


I suspect this should differentiate between a test_that() test case and a describe().

jennybc · 2023-11-02T06:13:34Z

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

+	const watcher = vscode.workspace.createFileSystemWatcher(pattern);
+
+	// Check that tests are not from RCMD and are not temp files
+	const RCMDpattern = '**/check/*.Rcheck/**';


This strikes me as a weird thing to be worrying about and, unless others disagree, I think I'll remove it. In my development life, it seem very weird for such a check folder to end up inside the source of an R package. But maybe VS Code-y workflows encourage different ways of working 🤔

jennybc · 2023-11-10T00:06:55Z

extensions/positron-r/src/testing/LICENSE

+
+MIT License
+
+Copyright (c) [2023] [author]


What should [author] be here?

DavisVaughan

I think in general this feels like a really great first step!

Lots of comments, nothing that should block this MVP from being merged. The most important one right now is that it is currently dramatically slower to run a full test suite this way.

Otherwise the comments are mostly about workflow. We should turn important comments / discussions in this thread into issues so we can track them

extensions/positron-r/package.json

DavisVaughan · 2023-11-10T15:39:37Z

extensions/positron-r/package.json

@@ -435,6 +435,7 @@
    "@types/glob": "^7.2.0",
    "@types/mocha": "^9.1.0",


Nicely updating as I write new tests and delete old ones 👍

Screen.Recording.2023-11-10.at.10.38.33.AM.mov

DavisVaughan · 2023-11-10T15:43:08Z

extensions/positron-r/package.json

@@ -435,6 +435,7 @@
    "@types/glob": "^7.2.0",
    "@types/mocha": "^9.1.0",
    "@types/node": "14.x",


I was 100% expecting that clicking on the name of the test file would open that test file.

I realize we may not be able to control this as easily without making internal tweaks to the test infra.

I think it would be nice if clicking on the test file name both opened the test file in the editor and expanded the list of tests in that file. It may also be ok if the user has to manually hit the > to expand the list, not sure.

Screen.Recording.2023-11-10.at.10.40.37.AM.mov

Similarly I expected clicking on the name of a test would also jump to it in the editor rather than finding it in the file explorer

(i have realized that for an individual test you can double click on the name to open it in an editor, that's nice)

Screen.Recording.2023-11-10.at.10.44.35.AM.mov

DavisVaughan · 2023-11-10T15:50:15Z

extensions/positron-r/package.json

@@ -435,6 +435,7 @@
    "@types/glob": "^7.2.0",
    "@types/mocha": "^9.1.0",
    "@types/node": "14.x",
+    "@types/split2": "^4.2.2",
    "@types/which": "^3.0.0",


It would be cool to have cli coloring in here

DavisVaughan · 2023-11-10T15:51:55Z

extensions/positron-r/package.json

@@ -435,6 +435,7 @@
    "@types/glob": "^7.2.0",
    "@types/mocha": "^9.1.0",
    "@types/node": "14.x",
+    "@types/split2": "^4.2.2",
    "@types/which": "^3.0.0",
    "@typescript-eslint/eslint-plugin": "^5.12.1",


I have a feeling that 99% of the time for our tests, this red text off to the RHS is not actually going to be very useful. Our error messages are often structured over multiple lines and have enough preamble that is "extra info" that it means the actual meat of the issue gets cut off

I wonder if there is a way to turn that off?

DavisVaughan · 2023-11-10T16:09:52Z

extensions/positron-r/package.json

@@ -448,7 +449,9 @@
  },


I don't particularly love the Peek View. I turned Automatically Open Peek View to never so it doesn't automatically open on the first failure, but it still opens the Peek View if I click the test name in the explorer pane on the LHS (if i double click it, the peek view goes away).

I think I don't like the peek view because you always end up hitting the little x to get rid of it so that you can actually type in the file to fix the test. I think anything that gets in the way of you editing the source file itself is kind of unfortunate. My preferred workflow would be:

click on test name in explorer on the left

opens editor to test location (but no peek view!)

error is shown in Test Results down below

That allows you to immediately fix the test in the editor while seeing the failure down below. It also means the error message is a little more persistent (i.e. it doesn't just go away when you hit the x).

It would look like this, which I carefully crafted from some extra button clicks:

DavisVaughan · 2023-11-10T16:11:26Z

extensions/positron-r/package.json

@@ -448,7 +449,9 @@
  },
  "dependencies": {


I am noting that this Debug Tests button does ✨ nothing ✨

DavisVaughan · 2023-11-10T16:17:15Z

extensions/positron-r/package.json

@@ -448,7 +449,9 @@
  },
  "dependencies": {
    "p-queue": "^6.6.2",


It takes quite a long time to run dplyr tests. 139sec vs 35sec just using devtools::test(). That's quite unfortunate 😢 (I even reran the tests in Positron a second time and it was about the same. I wondered if the slowness had to do with dynamically finding the tests 1 time per file, but that doesn't seem to be the case)

It's also pretty "jumpy" over in the text explorer. I was expecting the tests to run in order. I was also expected a ✅ beside the test file as it passed (you get all the green checks at once at the very end, but im pretty sure the Positron tests show a green check mark immediately as the test passes?)

Screen.Recording.2023-11-10.at.11.15.29.AM.mov

When we use the test explorer to run all tests, currently that happens via a sequence of test_file() calls. I assume that's where the slowness is coming from (for example, we're launching a new R process for each file).

I will explore how to route this gesture through test() or similar. That might end up being promoted to a separate issue for me to tackle soon. This is related to my idea that we might need to introduce one more level into the test item hierarchy (mentioned in #1808, re: multi-root workspaces): package > file > test instead of just file > test.

Update: I have fixed this locally now. The slowness was due to spawning 1 R process per test! Now we detect when we're running the full test suite and spawn 1 R process, total.

DavisVaughan · 2023-11-10T16:21:05Z

extensions/positron-r/package.json

@@ -448,7 +449,9 @@
  },
  "dependencies": {
    "p-queue": "^6.6.2",
-    "vscode-languageclient": "^7.0.0"


For some reason, dplyr has a test like

test_that(paste0("group_by handles encodings for native strings (#1507)"), {

that has broken the test runner, showing me:

Test with id Users/davis/files/r/packages/dplyr/tests/testthat/test-group-by.R&group_by handles encodings for native strings (#1507) could not be found. Please report this.

It caused the test-group-by.R file to stop running the rest of its tests, but everything else ran fine still

Yeah, this is the sort of thing that executing match.call() on the test_that() call would clean up. I think the current stance is "don't do weird stuff like this".

DavisVaughan · 2023-11-10T16:23:48Z

extensions/positron-r/package.json

-    "vscode-languageclient": "^7.0.0"
+    "split2": "^4.2.0",
+    "vscode-languageclient": "^7.0.0",
+    "web-tree-sitter": "^0.20.8"
  },


An interesting workflow note. Some tests require running code in the Console to update them, like snapshot tests.

But because Test Results and the Console are in the same pane, it takes a click to get over to Console, run that code suggestion, and then come back.

It is also a clickable suggestion in RStudio, which is super nice

jennybc · 2023-11-11T22:48:01Z

extensions/positron-r/package.json

@@ -145,10 +145,10 @@
          "default": "off",
          "description": "%r.configuration.tracing.description%"
        },
-        "positron.r.testing.experimental": {


This doesn't feel much more experimental to me now than the rest of the R extension, so I renamed this setting.

jennybc · 2023-11-11T22:48:21Z

extensions/positron-r/package.json

          "type": "boolean",
-          "default": false,
-          "description": "%r.configuration.experimentalPackageTesting.description%"
+          "default": true,


Defaulting to true now that this is wired up.

jennybc · 2023-11-11T22:50:04Z

extensions/positron-r/package.json

+        "view": "testing",
+        "contents": "R tests appear here when (1) current workspace includes an R package that uses testthat and (2) the setting Positron > R: Testing is set to 'true'.\n[Adjust Settings](command:workbench.action.openSettings?\"positron.r.testing\")"
+      }
+    ],


Contributes the "R part" of this:

Possibly suppressing the "Python part" in a pure R project is tracked in #1808 (comment).

I like the button that takes the user to the specific setting they need to toggle, if it's disabled (we default to true now, though):

extensions/positron-r/package.json

Adapt https://github.com/meakbiyik/vscode-r-test-adapter for use in the positron R extension

split2 is new, child_process is not

It seems weird to have "special case worries" about files matching this pattern. Until I see the need with my own eyes, let's live without this.

jennybc · 2023-11-14T22:43:34Z

extensions/positron-r/package.json

@@ -403,6 +413,28 @@
        "path": "./snippets/r.code-snippets"
      }
    ],
+    "viewsWelcome": [


This controls the R extension's contribution to the test explorer view when it's not (yet) populated with test files.

jennybc force-pushed the feature/test-explorer-for-r-testthat branch from 5f71d3f to ac59e4a Compare November 2, 2023 05:37

jennybc commented Nov 2, 2023

View reviewed changes

jennybc force-pushed the feature/test-explorer-for-r-testthat branch from 7ac534d to 7741380 Compare November 9, 2023 18:01

This was referenced Nov 9, 2023

R: Test explorer improvements #1808

Closed

Create a central place to disclose use and comply with the MIT license's requirement for attribution #1809

Closed

jennybc commented Nov 10, 2023

View reviewed changes

extensions/positron-r/src/testing/LICENSE Outdated

MIT License

Copyright (c) [2023] [author]

Copy link

Member Author

jennybc Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should [author] be here?

DavisVaughan approved these changes Nov 10, 2023

View reviewed changes

jennybc commented Nov 13, 2023

View reviewed changes

jennybc added 7 commits November 13, 2023 09:55

Adapt vscode-r-test-adapter

4681048

Adapt https://github.com/meakbiyik/vscode-r-test-adapter for use in the positron R extension

Correct this comment

3a72be0

split2 is new, child_process is not

Add license info about vscode-r-test-adapter

d0005f1

Get rid of one layer of helper function

03eed1c

Turn on testing support by default

47ebba8

Support on-the-fly configuration for R test explorer

59c1221

Simplify the file watchers

b8de55b

It seems weird to have "special case worries" about files matching this pattern. Until I see the need with my own eyes, let's live without this.

jennybc force-pushed the feature/test-explorer-for-r-testthat branch from 65f6271 to b8de55b Compare November 13, 2023 17:55

Be more responsive to current testthat state

d9ec374

jennybc commented Nov 14, 2023

View reviewed changes

Fill in authors for the MIT license

cb4e11d

jennybc marked this pull request as ready for review November 14, 2023 23:27

jennybc merged commit 1c59a02 into main Nov 14, 2023
1 check passed

jennybc deleted the feature/test-explorer-for-r-testthat branch November 14, 2023 23:33

This was referenced Nov 16, 2023

Level up the file system hygiene for test explorer resources #1843

Merged

Provide a way for language packs to evaluate code or call functions in the interpreter #1860

Closed

jennybc mentioned this pull request Nov 27, 2023

Second wave of R test explorer improvement (eg better "run all tests") #1894

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt vscode-r-test-adapter #1763

Adapt vscode-r-test-adapter #1763

jennybc commented Nov 2, 2023

jennybc Nov 2, 2023

jennybc Nov 2, 2023 •

edited

Loading

jennybc Nov 2, 2023

jennybc Nov 2, 2023 •

edited

Loading

jennybc Nov 2, 2023

jennybc Nov 2, 2023

jennybc Nov 2, 2023

jennybc Nov 2, 2023

jmcphers Nov 2, 2023

jennybc Nov 10, 2023

DavisVaughan left a comment

DavisVaughan Nov 10, 2023

DavisVaughan Nov 10, 2023

DavisVaughan Nov 10, 2023

DavisVaughan Nov 10, 2023

DavisVaughan Nov 10, 2023

DavisVaughan Nov 10, 2023

DavisVaughan Nov 10, 2023

DavisVaughan Nov 10, 2023

jennybc Nov 10, 2023 •

edited

Loading

DavisVaughan Nov 10, 2023

jennybc Nov 14, 2023

DavisVaughan Nov 10, 2023

jennybc Nov 11, 2023 •

edited

Loading

jennybc Nov 11, 2023

jennybc Nov 11, 2023 •

edited

Loading

jennybc Nov 14, 2023

		@@ -435,6 +435,7 @@
		"@types/glob": "^7.2.0",
		"@types/mocha": "^9.1.0",

Adapt vscode-r-test-adapter #1763

Adapt vscode-r-test-adapter #1763

Conversation

jennybc commented Nov 2, 2023

Choose a reason for hiding this comment

jennybc Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavisVaughan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc Nov 2, 2023 •

edited

Loading

jennybc Nov 2, 2023 •

edited

Loading

jennybc Nov 10, 2023 •

edited

Loading

jennybc Nov 11, 2023 •

edited

Loading

jennybc Nov 11, 2023 •

edited

Loading