-
Notifications
You must be signed in to change notification settings - Fork 92
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
Changes from 8 commits
4681048
3a72be0
d0005f1
03eed1c
47ebba8
59c1221
b8de55b
d9ec374
cb4e11d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
|
@@ -145,10 +155,10 @@ | |
"default": "off", | ||
"description": "%r.configuration.tracing.description%" | ||
}, | ||
"positron.r.testing.experimental": { | ||
"positron.r.testing": { | ||
"type": "boolean", | ||
"default": false, | ||
"description": "%r.configuration.experimentalPackageTesting.description%" | ||
"default": true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defaulting to |
||
"description": "%r.configuration.packageTesting.description%" | ||
}, | ||
"positron.r.restoreWorkspace": { | ||
"scope": "window", | ||
|
@@ -403,6 +413,28 @@ | |
"path": "./snippets/r.code-snippets" | ||
} | ||
], | ||
"viewsWelcome": [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
} | ||
], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
"taskDefinitions": [ | ||
{ | ||
"type": "rPackageTask", | ||
|
@@ -435,6 +467,7 @@ | |
"@types/glob": "^7.2.0", | ||
jennybc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"@types/mocha": "^9.1.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Screen.Recording.2023-11-10.at.10.40.37.AM.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
"@typescript-eslint/eslint-plugin": "^5.12.1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
"@typescript-eslint/parser": "^5.12.1", | ||
|
@@ -448,7 +481,9 @@ | |
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I think I don't like the peek view because you always end up hitting the little
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 It would look like this, which I carefully crafted from some extra button clicks: |
||
"dependencies": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
"p-queue": "^6.6.2", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I will explore how to route this gesture through 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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason, dplyr has a test like
that has broken the test runner, showing me:
It caused the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is the sort of thing that executing |
||
"split2": "^4.2.0", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need web-tree-sitter in order to use |
||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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 |
||
"repository": { | ||
"type": "git", | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
|
||
MIT License | ||
|
||
Copyright (c) [2023] [author] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What should |
||
|
||
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. |
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. |
There was a problem hiding this comment.
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.