Add testing for R packages #915

juliasilge · 2023-07-24T21:43:34Z

A draft for now!

juliasilge · 2023-07-24T21:46:59Z

extensions/positron-r/src/testing.ts

+	const fileContents = vscode.workspace.openTextDocument(uri);
+	const matches = [];
+
+	// TODO: get the tests out of the file contents


Major TODO right here: how to get the tests out of the file? As currently implemented, we need:

a label for the test (like from the first argument to testthat::test_that() plus an identifier for which test it is in that block)

a vscode.Position for the start

a vscode.Position for the end

EDITED TO SAY: I think this is probably a bad idea, upon further exploration:

If instead we made matches an array of strings (code for each test), we could use executeCode() to run them. However, that runs code in the console. That might be right for an interactive develop/test type workflow.

We should just make matches an array of label + start position + end position.

This is definitely out of scope for this PR because it involves some fairly extensive new work; let's merge with dummy data about where tests are.

I will hopefully make some progress on delivering the coordinates needed to address individual tests in the next week 🤞

extensions/positron-r/src/testing.ts

juliasilge · 2023-07-24T21:51:57Z

extensions/positron-r/src/testing.ts

+
+	return Promise.all(
+		vscode.workspace.workspaceFolders.map(async workspaceFolder => {
+			const pattern = new vscode.RelativePattern(workspaceFolder, 'tests/testthat/*.R');


This is testthat specific, which I think is fine for alpha. In the longer term, we'll want to support more than just testthat.

Finding the files for Runit or tinytest (or no testing framework?) will not be that hard, but the parsing will be fairly different.

juliasilge · 2023-07-24T23:27:28Z

As of right now, this correctly identifies testthat test files in a real R package and they populate in the testing UI (with the fake matches found via findTests() for now).

juliasilge · 2023-07-26T21:07:43Z

This now will actually run tests! 🎉

(I am still manually doing library(testthat) and devtools::load_all() before running a test though.)

juliasilge · 2023-07-27T20:16:49Z

This is now behind a feature flag:

juliasilge · 2023-07-27T20:20:36Z

extensions/positron-r/src/testing.ts

+	const matches = [];
+
+	// TODO: get the tests out of the file contents
+	// This is just dummy example data from my cereal package:


For fellow developers on this, here is where you put in some dummy matches for where tests are.

juliasilge · 2023-07-27T20:21:53Z

extensions/positron-r/src/testing.ts

+	const startIndex = document.offsetAt(range.start);
+	const endIndex = document.offsetAt(range.end);
+	const testSource = source.slice(startIndex, endIndex);
+	positron.runtime.executeCode('r', testSource, true);


As of right now, this always succeeds (i.e. tests can never fail) because we do not yet return anything from executeCode other than a logical about whether the code was sent off. This also is out of scope for this PR and we will come back to it.

…n files

juliasilge · 2023-07-27T21:45:40Z

Let me outline some of what still needs to be done to make this usable:

Discover tests in test files via a parser
Examine how we're going to run the tests (probably update executeCode to return something needed here like errors, probably add ability to run test in a "clean room" kind of fresh process)

Longer term, we'll want to add coverage runs. Even longer term we'll want to support tests other than testthat.

juliasilge · 2023-07-27T21:47:45Z

extensions/positron-r/src/extension.ts

@@ -59,5 +60,8 @@ export function activate(context: vscode.ExtensionContext) {
 	// Register commands.
 	registerCommands(context);

+	// Discover R package tests.
+	discoverTests(context);


I like how I gave this function a name that says it does something it literally does not do at all right now. 😆 For the long term (i.e. after we do parsing) is this what we want to name this function? Other ideas?

juliasilge · 2023-07-27T21:48:35Z

extensions/positron-r/src/testing.ts

+			}
+		};
+
+		controller.createRunProfile(


In the longer term, we will have multiple run profiles, like for "run in current process", "run in fresh process", and "coverage" at least.

jennybc

I can only review this from a somewhat weird perspective, but I went ahead and took one pass, in the name of getting this excellent start merged in before @juliasilge is OOO.

extensions/positron-r/src/testing.ts

jennybc · 2023-07-27T22:51:04Z

extensions/positron-r/src/testing.ts

+		);
+		context.subscriptions.push(controller);
+
+		controller.resolveHandler = async (test) => {


Presumably this will become clear as I engage with this further, but what is test? E.g. is it a file(path)?

It's a testItem, which lives in like a little tree structure and can be the whole test suite, some subset of the tests that belong together, or a single test. As I've implemented this here, I believe we have whole test suite, a file of tests, or an individual test. There's a bit of info here:
https://code.visualstudio.com/api/references/vscode-api#TestItem

extensions/positron-r/src/testing.ts

jennybc · 2023-07-27T23:04:57Z

extensions/positron-r/src/testing.ts

+
+async function runTest(run: vscode.TestRun, test: vscode.TestItem): Promise<vscode.TestRun> {
+	if (test.children.size > 0) {
+		test.children.forEach(childTest => runTest(run, childTest));


This feels like we're delegating operating on the test hierarchy to the VS Code infrastructure. I.e. as if running a test file = running each test in the file, and running all test files = running the tests in each file. Which seems totally logical except ...

We would normally call testthat::test_file() to run one file's-worth of tests and devtools::test() run one one package's-worth of tests. Those approaches come with goodies like doing appropriate setup/teardown (like load_all(), sourcing test helpers), reporting, etc.

Are we going to have think about how to align these conventional approaches with what this testing harness does?

I think "yes" is the answer here. Right now, with just sending everything to the console for a very interactive workflow, the user would be required to manage this like if they were using cmd + enter. When we add in other run types, we'll need to take on these requirements.

Co-authored-by: Jennifer (Jenny) Bryan <jenny.f.bryan@gmail.com>

jmcphers · 2023-10-03T23:50:36Z

@juliasilge What's the status of this PR? Happy to review if you still want to merge it. By my reading it's largely disengaged unless enabled so I am not concerned about stability implications.

juliasilge · 2023-10-04T16:29:56Z

@jmcphers yes, this is the scaffolding we need so I am in favor of merging it. It sounds like there are some questions about whether we should parse the tests using tree sitter in a middleware layer or the lower level Rust, but either way, IMO it's better to have this merged in so folks can concretely see what needs to be generated.

jmcphers · 2023-10-04T17:03:35Z

Ok, let's get it in!

juliasilge added 2 commits July 24, 2023 10:13

New discoverTests() function

1e009dd

Scaffold out how to discover tests

996ff65

juliasilge commented Jul 24, 2023

View reviewed changes

extensions/positron-r/src/testing.ts Outdated Show resolved Hide resolved

juliasilge commented Jul 24, 2023

View reviewed changes

Update regex, add items to test controller

373b4ea

Run a single test

cb018af

juliasilge added 2 commits July 27, 2023 14:06

Add new configuration for experimental testing UI

97baf73

Put testing support behind a feature flag

1eb2220

juliasilge commented Jul 27, 2023

View reviewed changes

Make runTest() recursive, because testItems can be files or tests i…

a842dae

…n files

juliasilge marked this pull request as ready for review July 27, 2023 21:43

juliasilge commented Jul 27, 2023

View reviewed changes

juliasilge requested review from jmcphers and jennybc July 27, 2023 22:00

jennybc approved these changes Jul 27, 2023

View reviewed changes

juliasilge and others added 4 commits July 27, 2023 18:09

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

668a07c

Co-authored-by: Jennifer (Jenny) Bryan <jenny.f.bryan@gmail.com>

Identify both test*.R and test*.r

0012b87

Early return

84bced4

Merge branch 'main' into add-testing-support

1ba540d

jennybc mentioned this pull request Sep 22, 2023

Integrate 'testthat' into the Tests feature for R packages #1365

Closed

Merge branch 'main' into add-testing-support

583eb6d

jmcphers merged commit 1bce527 into main Oct 4, 2023
1 check passed

juliasilge deleted the add-testing-support branch October 4, 2023 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add testing for R packages #915

Add testing for R packages #915

juliasilge commented Jul 24, 2023

juliasilge Jul 24, 2023

juliasilge Jul 25, 2023 •

edited

Loading

juliasilge Jul 27, 2023

jennybc Jul 27, 2023

juliasilge Jul 24, 2023

juliasilge Jul 27, 2023

juliasilge commented Jul 24, 2023

juliasilge commented Jul 26, 2023

juliasilge commented Jul 27, 2023

juliasilge Jul 27, 2023

juliasilge Jul 27, 2023 •

edited

Loading

juliasilge commented Jul 27, 2023

juliasilge Jul 27, 2023

juliasilge Jul 27, 2023

jennybc left a comment

jennybc Jul 27, 2023

juliasilge Jul 28, 2023

jennybc Jul 27, 2023 •

edited

Loading

juliasilge Jul 28, 2023

jmcphers commented Oct 3, 2023

juliasilge commented Oct 4, 2023

jmcphers commented Oct 4, 2023

Add testing for R packages #915

Add testing for R packages #915

Conversation

juliasilge commented Jul 24, 2023

Choose a reason for hiding this comment

juliasilge Jul 25, 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

juliasilge commented Jul 24, 2023

juliasilge commented Jul 26, 2023

juliasilge commented Jul 27, 2023

Choose a reason for hiding this comment

juliasilge Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

juliasilge commented Jul 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcphers commented Oct 3, 2023

juliasilge commented Oct 4, 2023

jmcphers commented Oct 4, 2023

juliasilge Jul 25, 2023 •

edited

Loading

juliasilge Jul 27, 2023 •

edited

Loading

jennybc Jul 27, 2023 •

edited

Loading