Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt vscode-r-test-adapter #1763

Merged
merged 9 commits into from
Nov 14, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions extensions/positron-r/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@
"title": "%r.command.packageTest.title%",
"shortTitle": "%r.menu.packageTest.title%"
},
{
"command": "r.useTestthat",
"category": "R",
"title": "%r.command.useTestthat.title%"
},
{
"command": "r.useTest",
"category": "R",
"title": "%r.command.useTest.title%"
},
{
"command": "r.packageCheck",
"category": "R",
Expand Down Expand Up @@ -145,10 +155,10 @@
"default": "off",
"description": "%r.configuration.tracing.description%"
},
"positron.r.testing.experimental": {
Copy link
Member Author

@jennybc jennybc Nov 11, 2023

Choose a reason for hiding this comment

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

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

"positron.r.testing": {
"type": "boolean",
"default": false,
"description": "%r.configuration.experimentalPackageTesting.description%"
"default": true,
Copy link
Member Author

Choose a reason for hiding this comment

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

Defaulting to true now that this is wired up.

"description": "%r.configuration.packageTesting.description%"
},
"positron.r.restoreWorkspace": {
"scope": "window",
Expand Down Expand Up @@ -403,6 +413,28 @@
"path": "./snippets/r.code-snippets"
}
],
"viewsWelcome": [
Copy link
Member Author

Choose a reason for hiding this comment

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

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

{
"view": "testing",
"contents": "Workspace does not appear to be an R package.",
"when": "!isRPackage"
},
{
"view": "testing",
"contents": "R tests can appear here if the package uses testthat and the setting Positron > R: Testing is set to 'true'.\n[Adjust this setting](command:workbench.action.openSettings?\"positron.r.testing\")",
"when": "isRPackage && !config.positron.r.testing"
},
{
"view": "testing",
"contents": "Package does not appear to use testthat.\n[Configure testhat for R](command:r.useTestthat)",
"when": "isRPackage && config.positron.r.testing && !testthatIsConfigured"
},
{
"view": "testing",
"contents": "No testthat tests found.\n[Add a test](command:r.useTest)",
"when": "isRPackage && config.positron.r.testing && testthatIsConfigured && !testthatHasTests"
}
],
Copy link
Member Author

@jennybc jennybc Nov 11, 2023

Choose a reason for hiding this comment

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

Contributes the "R part" of this:

Screenshot 2023-11-14 at 2 43 57 PM

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):

Screenshot 2023-11-15 at 11 41 23 AM

"taskDefinitions": [
{
"type": "rPackageTask",
Expand Down Expand Up @@ -435,6 +467,7 @@
"@types/glob": "^7.2.0",
jennybc marked this conversation as resolved.
Show resolved Hide resolved
"@types/mocha": "^9.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

"@types/node": "14.x",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

"@types/split2": "^4.2.2",
"@types/which": "^3.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cool to have cli coloring in here

Screenshot 2023-11-10 at 10 49 53 AM

"@typescript-eslint/eslint-plugin": "^5.12.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Screenshot 2023-11-10 at 10 50 42 AM

"@typescript-eslint/parser": "^5.12.1",
Expand All @@ -448,7 +481,9 @@
},
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Screenshot 2023-11-10 at 11 01 03 AM

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:

Screenshot 2023-11-10 at 11 04 05 AM

"dependencies": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Screenshot 2023-11-10 at 11 10 58 AM

"p-queue": "^6.6.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@jennybc jennybc Nov 10, 2023

Choose a reason for hiding this comment

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

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.

"vscode-languageclient": "^7.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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".

"split2": "^4.2.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

"vscode-languageclient": "^7.0.0",
"web-tree-sitter": "^0.20.8"
Copy link
Member Author

@jennybc jennybc Nov 2, 2023

Choose a reason for hiding this comment

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

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

},
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Screenshot 2023-11-10 at 11 22 16 AM

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

Screenshot 2023-11-10 at 11 23 27 AM

"repository": {
"type": "git",
Expand Down
4 changes: 3 additions & 1 deletion extensions/positron-r/package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
"r.command.packageBuild.title": "Build R Package",
"r.command.packageInstall.title": "Install R Package and Restart R",
"r.command.packageTest.title": "Test R Package",
"r.command.useTestthat.title": "Configure testthat",
"r.command.useTest.title": "Add (or visit) a test file",
"r.command.packageCheck.title": "Check R Package",
"r.command.packageDocument.title": "Document R Package",
"r.menu.createNewFile.title": "R File",
Expand All @@ -33,7 +35,7 @@
"r.configuration.logLevel.debug.description": "Debug messages",
"r.configuration.logLevel.trace.description": "Verbose tracing messages",
"r.configuration.logLevel.description": "Log level for the R Kernel (requires restart to take effect)",
"r.configuration.experimentalPackageTesting.description": "Experimental UI support for testing R packages",
"r.configuration.packageTesting.description": "Explore and run testthat tests",
"r.configuration.restoreWorkspace.description": "Restore the workspace from .RData on startup (requires restart to take effect)",
"r.configuration.extraArguments.description": "Extra command-line arguments for R intialization",
"r.configuration.quietMode.description": "Run R in quiet mode, to suppress initial copyright and welcome messages (requires restart to take effect)",
Expand Down
9 changes: 9 additions & 0 deletions extensions/positron-r/src/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { getRunningRRuntime } from './provider';
import { getRPackageName } from './contexts';
import { getRPackageTasks } from './tasks';
import { randomUUID } from 'crypto';
import { refreshTestthatStatus } from './testing/watcher';

export async function registerCommands(context: vscode.ExtensionContext, runtimes: Map<string, RRuntime>) {

Expand Down Expand Up @@ -97,6 +98,14 @@ export async function registerCommands(context: vscode.ExtensionContext, runtime
positron.runtime.executeCode('r', 'devtools::test()', true);
}),

vscode.commands.registerCommand('r.useTestthat', () => {
positron.runtime.executeCode('r', 'usethis::use_testthat()', true);
}),

vscode.commands.registerCommand('r.useTest', () => {
positron.runtime.executeCode('r', 'usethis::use_test("rename-me")', true);
}),

vscode.commands.registerCommand('r.packageCheck', async () => {
const tasks = await getRPackageTasks();
const task = tasks.filter(task => task.definition.task === 'r.task.packageCheck')[0];
Expand Down
3 changes: 3 additions & 0 deletions extensions/positron-r/src/contexts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@
*--------------------------------------------------------------------------------------------*/

import * as vscode from 'vscode';
import { refreshTestthatStatus } from './testing/watcher';

export async function setContexts(_context: vscode.ExtensionContext): Promise<void> {
const isRPackage = await detectRPackage();
vscode.commands.executeCommand('setContext', 'isRPackage', isRPackage);

await refreshTestthatStatus();
}

export async function detectRPackage(): Promise<boolean> {
Expand Down
12 changes: 8 additions & 4 deletions extensions/positron-r/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { registerCommands } from './commands';
import { registerFormatter } from './formatting';
import { providePackageTasks } from './tasks';
import { setContexts } from './contexts';
import { discoverTests } from './testing';
import { setupTestExplorer, refreshTestExplorer } from './testing/testing';
import { rRuntimeProvider } from './provider';
import { RRuntime } from './runtime';

Expand Down Expand Up @@ -38,7 +38,11 @@ export function activate(context: vscode.ExtensionContext) {
// Provide tasks.
providePackageTasks(context);

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

// Setup testthat test explorer.
setupTestExplorer(context);
vscode.workspace.onDidChangeConfiguration(async event => {
if (event.affectsConfiguration('positron.r.testing')) {
refreshTestExplorer(context);
}
});
}
162 changes: 0 additions & 162 deletions extensions/positron-r/src/testing.ts

This file was deleted.

22 changes: 22 additions & 0 deletions extensions/positron-r/src/testing/LICENSE
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@

MIT License

Copyright (c) [2023] [author]
Copy link
Member Author

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?


Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in
all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.
1 change: 1 addition & 0 deletions extensions/positron-r/src/testing/README
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
The code in this folder (positron-r/src/testing/) has been adapted from https://github.com/meakbiyik/vscode-r-test-adapter, which is licensed under the MIT license.
Loading