Test parsing and running upgrades #2796

jennybc · 2024-04-18T05:29:12Z

Intent

Addresses More intelligent handling of quotes inside test desc in R test explorer #2639
Addresses Provide more support for describe() tests in the test explorer #2634
Improves output for skipped tests (addresses R: Test explorer improvements #1808 (comment))
(No issue for this but actually the most important bit ...) externalizes the tree-sitter queries used to identify test items and load up the test controller.
- In combination with https://github.com/DavisVaughan/r-tree-sitter, this paves the way for bringing these queries under test. I.e. I hope to test these against various test files in the R package.
- Also lays the foundation for possible extensibility, i.e. being able to work with test_that()-like functions exported by packages other than testthat or internal helpers for a specific package.

QA Notes

In a workspace that is an R package, click on the flask in the activity bar to activate the test explorer. If the package has testthat tests, the relevant test files will appear in a tree view, for navigation, execution, and display of test results.

I have put up a toy R package that can be used to exercise the bug fixes and features in this PR: https://github.com/jennybc/testfun. I would install this with pak::pak("jennybc/testfun"). Another good option is remotes::install_github("jennybc/testfun").

More intelligent handling of quotes inside test desc in R test explorer #2639

Run the entire test suite or just the file test-banana.R or just the individual test 'subtraction `still` "works"'. You can use the ▶️ button in the test explorer tree view or the ▶️ button in the gutter of test file to run a test item (file or test) or the double ▶️ button at the top of the explorer to run all tests.

The test 'subtraction `still` "works"' (and any other tests that have vexing quotes in the description) should run and pass.

Provide more support for describe() tests in the test explorer #2634

Use the test explorer to navigate to test-mathstuff.R and use the expando to reveal all the tests. Clear all test results from the menu accessed via the ... in the upper right corner of the TEST tree view. Here is the expected behaviour:

Running the single describe("matrix()", { ... }) should work: it should run, there should be green checks for both of its it()s and for the describe() itself. No other test items in this file should appear as being run; they should still have the grey circle.
The single describe("addition()", { ... }) should work if you run it from the gutter. A describe() with exactly 1 it() cannot be run from the explorer's treeview (VS Code tries to run just the child it()) and I'm still analyzing whether this is our problem or a bug in VS Code. This is a known issue; see Account for nested describe() in R test explorer #2805.
The nested describe()s and the enclosing describe("top-level describe with describe inside", { ... }) cannot be run individually and this is a known issue. For now, such a structure can only be run when the entire file is run. See Account for nested describe() in R test explorer #2805.

Improved output for skipped tests #1808 (comment)

Run the single test file test-apple-wtf.R. The messages appearing in the TEST OUTPUT tab of the lower panel should appear one to a line and include the test file location where the skip originated.

Good:

Reason: I am a skip message: test-apple-wtf.R:2:3
Reason: I am also a skip message: test-apple-wtf.R:9:3

Bad:

Reason: I am a skip messageReason: I am also a skip message

jennybc · 2024-04-18T05:31:03Z

build/filters.js

@@ -152,6 +152,7 @@ module.exports.indentationFilter = [
 	// --- Start Positron ---
 	'!**/amalthea/**/*',
 	'!extensions/positron-r/resources/scripts/*.R',
+	'!extensions/positron-r/resources/testing/**',


The tree-sitter queries are in .scm files and some of the precommit hygiene checks are not appropriate.

That also applies to the embedded R package for test reporting, which also lives here.

Same for the diff just below.

jennybc · 2024-04-18T05:32:17Z

extensions/positron-r/resources/testing/describe.scm

+        (namespace_operator
+			lhs: (identifier)
+            rhs: (identifier) @parent_function
+        )


In https://github.com/posit-dev/positron/pull/2726/files#r1559866539 I promised to bring back the ability to parse testthat::test_that() and here it is.

jennybc · 2024-04-18T05:37:32Z

extensions/positron-r/resources/testing/describe.scm

@@ -0,0 +1,32 @@
+(call


Moving these queries into their own files has various advantages:

If we want to add queries, we can drop the .scm files here and the test parser will pick them up automatically now. These queries are extremely un-fun to write and are similar to regular expressions in terms of how hard they are to understand later. I suspect it will be easier to handle more styles of usage by adding queries than by making these queries increasingly complicated.

Easier to test the queries in https://github.com/DavisVaughan/r-tree-sitter.

Easier to maintain the queries and look at diffs, now that they have some reasonable syntax highlighting. Apparently the .scm extension is conventional because it's almost like Scheme.

jennybc · 2024-04-18T05:41:32Z

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

+	 * Is this a top-level call in the testthat file? Only top-level tests can be run individually.
+	 * This is really about making sure we can distinguish a top-level `describe()` (individually
+	 * runnable) from a `describe()` that's nested inside another `describe()` call (only runnable
+	 * as part of its enclosing `describe()` or test file).


There is work yet to be done re: nested describe()s. But this is one part of what's needed to determine whether a describe() can be run individually. Not used yet, but will be in future. See #2805.

jennybc · 2024-04-18T05:42:51Z

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

+	const escapedLabel = test?.label.replace(/(['"`])/g, '\\$1');
+	const descInsert = isSingleTest ? ` desc = '${escapedLabel || '<all tests>'}', ` : '';


This is what fixes things for tests where the description contains quotes or backticks.

Before, here's what happened when trying to run a test item with ', ", or backtick in its description:

Screen.Recording.2024-04-18.at.8.18.52.AM.mov

After, it works:

Screen.Recording.2024-04-18.at.8.14.20.AM.mov

jennybc · 2024-04-18T05:46:08Z

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

@@ -158,7 +165,7 @@ export async function runThatTest(
 						break;
 					case 'add_result':
 						if (data.result !== undefined && data.test !== undefined) {
-							const testItem = isSingleTest
+							const testItem = testType === ItemType.TestThat


You can only assume a test result is for the test item that triggered this test run if you're running a single test_that(). In describe(it())-land, you get results for the individual it() calls, which are not the test item for the test run (although they are children of it).

The recording in https://github.com/posit-dev/positron/pull/2796/files#r1570975865 also covers this, i.e. it's why the it() calls are correctly receiving green checks.

jennybc · 2024-04-18T15:28:12Z

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

+		// TODO: Should really also require that this is a top-level describe().
+		case ItemType.Describe: {
+			const testthatInstalled = await checkInstalled('testthat', '3.2.1');
+			if (!testthatInstalled) {
+				return Promise.resolve('testthat >= 3.2.1 is needed to run R a single describe() test.');
+			}
+			LOGGER.info('Single describe() test');
+			break;
+		}


Running an individual top-level describe() now works, for most usage. Known issues recorded in #2805: nested describe() is not fully accounted for; different behaviour when clicking in test explorer tree view vs. in gutter of test file when the describe() has exactly 1 it().

This recording shows running a single describe(). Note that its it() children correctly receive their result (success) and the other describe()s in the file remain un-executed.

Screen.Recording.2024-04-18.at.8.23.41.AM.mov

jennybc · 2024-04-18T15:33:00Z

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

+										// Currently skip messages leak out to the TEST RESULTS
+										// area, which is presumably not the long-term plan for
+										// how we want to use that space.
+										// But in the meantime, let's at least break lines.
+										// appendOutput method is documented to need CRLF not LF.
+										run.appendOutput(
+											data.message + ': ' + data.location + '\r\n',
+											undefined,
+											testItem
+										);


There is clearly much room for improvement re: how we use the TEST RESULTS tab of the panel, but skip message are at least readable/usable now.

Before, where skip messages are smushed together and convey no information about where they originated:

After, where we have one entry per skip and identify the origniating test file location:

extensions/positron-r/resources/testing/describe.scm

extensions/positron-r/resources/testing/test_that.scm

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

DavisVaughan · 2024-04-19T14:51:43Z

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

+		// we start at 1 and end at (length - 1) because we don't want the surrounding quotes
+		desc: captureDesc.node.text.substring(1, captureDesc.node.text.length - 1),


There has been a request to expose a node for string_contents which is string without the " or ' quotes in the simple case, and for raw strings it would be without the raw string prefix/suffix, which is otherwise hard to account for.

What you have here won't work for raw strings, but i would not worry about that

Thanks for that heads up. Yeah, I would definitely be a customer for string_contents if it gets added to the grammar. Agree that raw strings aren't super relevant here.

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

Co-authored-by: Davis Vaughan <davis@rstudio.com> Signed-off-by: Jennifer (Jenny) Bryan <jenny.f.bryan@gmail.com>

jennybc · 2024-04-19T16:10:26Z

I'm now waiting for https://github.com/posit-dev/positron/actions/runs/8756403214 because I want to make sure the .scm queries make it into a release build before I merge instead of my tradition, which is to learn about this after I merge 😬 .

jennybc commented Apr 18, 2024

View reviewed changes

jennybc added 13 commits April 18, 2024 07:45

Make a start

acd8599

Index into the match by name not number

9f4bddf

Store function name

65d25f3

Catch up for describe(it())

249bfed

More refactoring

b05aa81

Improve names and docs

3436beb

Retain if test call is top-level for eventual use in the run handler

336a2c6

Read the query strings lazily and log that

b899c4f

Combine two helpers; manage line length

e89b3c6

Bring back the ability to cope with namespace-qualified calls

af0c3c0

Do something less awful with skip messages

4dc984b

Escape embedded single or double quotes or backticks in test description

6dfe055

Support running a single describe()

2e63fac

jennybc force-pushed the test-parsing-upgrade branch from edfb344 to 2e63fac Compare April 18, 2024 14:46

Whitespace

35eafb4

jennybc commented Apr 18, 2024

View reviewed changes

jennybc mentioned this pull request Apr 19, 2024

R: Test explorer improvements #1808

Closed

12 tasks

jennybc marked this pull request as ready for review April 19, 2024 04:57

jennybc requested review from juliasilge and DavisVaughan April 19, 2024 04:57

DavisVaughan approved these changes Apr 19, 2024

View reviewed changes

jennybc and others added 4 commits April 19, 2024 08:42

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

23160c0

Co-authored-by: Davis Vaughan <davis@rstudio.com> Signed-off-by: Jennifer (Jenny) Bryan <jenny.f.bryan@gmail.com>

Specify this type

1f78def

Get rid of some stealthy spaces instead of tabs

bf9ca2e

Make sure the .scm queries make it into the binary

b460b93

jennybc merged commit ffd2b5d into main Apr 19, 2024
15 checks passed

jennybc deleted the test-parsing-upgrade branch April 19, 2024 18:21

juliasilge mentioned this pull request Apr 23, 2024

Provide more support for describe() tests in the test explorer #2634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test parsing and running upgrades #2796

Test parsing and running upgrades #2796

jennybc commented Apr 18, 2024 •

edited

Loading

jennybc Apr 18, 2024

jennybc Apr 18, 2024

jennybc Apr 18, 2024

jennybc Apr 18, 2024 •

edited

Loading

jennybc Apr 18, 2024

jennybc Apr 18, 2024

jennybc Apr 18, 2024

jennybc Apr 18, 2024 •

edited

Loading

jennybc Apr 18, 2024 •

edited

Loading

jennybc Apr 18, 2024

DavisVaughan Apr 19, 2024

jennybc Apr 19, 2024

jennybc commented Apr 19, 2024

		const escapedLabel = test?.label.replace(/(['"`])/g, '\\$1');
		const descInsert = isSingleTest ? ` desc = '${escapedLabel \|\| '<all tests>'}', ` : '';

		// we start at 1 and end at (length - 1) because we don't want the surrounding quotes
		desc: captureDesc.node.text.substring(1, captureDesc.node.text.length - 1),

Test parsing and running upgrades #2796

Test parsing and running upgrades #2796

Conversation

jennybc commented Apr 18, 2024 • edited Loading

Intent

QA Notes

More intelligent handling of quotes inside test desc in R test explorer #2639

Provide more support for describe() tests in the test explorer #2634

Improved output for skipped tests #1808 (comment)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybc Apr 18, 2024 • 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 Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

jennybc Apr 18, 2024 • 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 commented Apr 19, 2024

jennybc commented Apr 18, 2024 •

edited

Loading

jennybc Apr 18, 2024 •

edited

Loading

jennybc Apr 18, 2024 •

edited

Loading

jennybc Apr 18, 2024 •

edited

Loading