Open firelizzard18 opened 3 years ago
@hyangah @suzmue, is this something you support? I don't want to start work if this is not a direction you want to go.
Both @suzmue and I agree that is a good change. Making the behavior consistent and consolidating them is the direction to go. What properties in TestConfig would be left?
Thanks!
I think the biggest functional change would be output handling. Instead of sending the output to a OutputChannel, I would send it to the testing console and report any errors to the TestRun if a test fails. Of course, the question of 'what is an error message' is not straight forward for go test
.
I would add a request: vscode.TestRunRequest
property. I can remove functions
, since the run request includes a set of test items. I can remove isMod
- if a test item is in a module, the top-most ancestor must be a module test item. I might remove isBenchmark
, since that information is encoded into the test item. I think applyCodeCoverage
would be redundant, as the run request has a profile, and the profile kind is Run, Debug, or Coverage. includeSubDirectories
may be unnecessary - I'm guessing that's used to run all workspace tests, which can be done by requesting a run of the top-level test item(s) for the workspace.
I'm not sure about the rest. I'll have to look at the current code and figure out how and when they're used.
I think the first step will be creating a src/goTest
directory and splitting up src/goTestExplorer.ts
. 1000+ lines in one file adds unnecessary mental overhead to reasoning about the code.
Change https://golang.org/cl/343878 mentions this issue: src/goTest: prepare for refactoring TestConfig
@hyangah I've spent some time thinking about this.
The test explorer is the only method of running tests that potentially involves invoking go test
more than once. All other test support invokes go test
once, per invocation of command/lens/etc. Thus, the test explorer necessarily has additional logic that is not relevant to any other way of running tests. Thus, my plan:
NewTestConfig
as a subset of TestConfig
include
, exclude
, and profileKind
from TestRunRequest1dir
, functions
- determined from include/excludeapplyCodeCoverage
- determined from profileKindincludeSubDirectories
- including a module or workspace will run recursivelyisMod
- in most cases, the module will be loaded in the workspace, and the test resolver will already have identified it, so "is this test item within a module" can be answered by walking its ancestor chaincomputeTestConfig
that turns a single-invocation NewTestConfig
into a TestConfig
computeTestConfig
goTest
to use computeTestConfig
goTest
to use TestConfig
directly and incorporate the functionality of computeTestConfig
into it (or more likely, make it a helper function).TestConfig
/drop the 'New' from NewTestConfig
1 TestRunRequest has profile: TestRunProfile
, but we don't need anything from it other than the kind: TestRunProfileKind
, and only including the kind avoids the issue of retrieving/creating a run profile, which is supposed to be done with TestController.createRunProfile
.
CL 343878 has an initial implementation of computeTestConfig
Thanks for updating the issue with the details. Will take a look at the cl over the weekend. Do you know the current status of the new test apis in editors other than VS Code? (e.g Theia ID, etc...) Wonder how long we need to handle the case where the test API is not enabled or unavailable.
Do you know the current status of the new test apis in editors other than VS Code? (e.g Theia ID, etc...) Wonder how long we need to handle the case where the test API is not enabled or unavailable.
That's a good point. I don't know, and I hadn't thought about how that should affect refactoring. Steps 1-3 should be backwards compatible, since they only touch the test explorer, which is gated behind a check. Step 4 will be an issue if the API is not available.
@hyangah Given Microsoft's response to https://github.com/microsoft/vscode/issues/130796#issuecomment-906478882, it's clear that the only simple way to maintain backwards compatibility will be to limit utilization of new features. Since we can work around missing APIs, the breaking issue is declarations in package.json
. The only possibility that occurs to me (other than not using new features) would be adding an Action that strips the offending declarations out and repackages the app without them.
I can find no evidence that Theia has any support for the testing API, nor any issues that mention it. To allow us to continue refactoring while maintaining support for Theia, one option would be creating a polyfill that emulates a subset of the testing API. I think I could get by with mostly just TestItem
and TestItemCollection
. Of course, AFAIK we would also have to strip out unsupported declarations in package.json
.
It appears that Theia will not be supporting the testing API for some time (https://github.com/eclipse-theia/theia/issues/10140). In order to allow refactoring to continue, I propose we add a mock testing API/polyfill that can be loaded if the testing API is not present. That would allow us to use TestItem
and friends for all test support, without needing the full testing API. WDYT @hyangah?
On the other hand, does v0.28 work with Theia, now that it requires VSCode 1.59.0 in engines
?
This is not the first time Theia IDE is always lagging sadly. Theia-based IDEs will not be able to load v0.28 due to the engine version check - unless some users play with the hard-coded the engine version (which isn't of course the best practice). Like I mentioned in the other issue, I hope other editor users to pick the right version based on their level of APIs. (Not sure if open-vsx.org or other distributors support that though).
If it's possible to polyfill & mock & refactor without too much maintenance pain or bloat, that will be great.
But what about the incompatible declaration in package.json
? We don't want to publish/manage multiple versions of extensions.
If it's possible to polyfill & mock & refactor without too much maintenance pain or bloat, that will be great.
I think this would be feasible, since the polyfill doesn't need to do much other than create TestItems.
But what about the incompatible declaration in package.json? We don't want to publish/manage multiple versions of extensions.
If you claimed the golang
namespace on openvsx.org and updated the release workflow to publish to openvsx.org, you could modify the openvsx workflow to modify/remove the offending statements in package.json
.
But... then you do have two different versions, and users of VSCodium and Gitpod.io may be upset that their version is missing features, even though their IDE is capable of using them. And, if we keep @types/vscode
at 1.59 but do some shenanigans to allow the extension to be loaded by older versions, there's a pretty high likelihood that someone will add some other feature dependent on the 1.59 API, which will cause frustrating bugs.
If you claimed the golang namespace on openvsx.org and updated the release workflow to publish to openvsx.org, you could modify the openvsx workflow to modify/remove the offending statements in package.json.
We decided not to publish in open-vsx.org ourselves yet because it involves work more than just implementing the workflow (e.g. internal process for a new product launch). I got ok'd from google cloud shell ide project (currently based on theia ide) about moving independent of theia ide's support status. I don't know how many users use this extension through other theia based editors at this moment. Sadly I see diminishing returns for developing workarounds for it. Correct me otherwise.
Yeah, I expect the workarounds would only get more cumbersome as time goes on.
goTest
) and I would like to add support for "run test at cursor"VSCode already has commands for run/debug the test at the cursor so all that remains (after #3523) is test{CurrentFile,CurrentPackage,Workspace}
I mentioned this in #1641. I am opening a separate issue for discussing a specific change.
I would like to refactor
goTest
,testUtil
, andgoTestExplorer
to all work together:goTest(tc: TestConfig)
intorunTests(rq: vscode.TestRunRequest, tc: TestConfig)
and remove the properties ofTestConfig
that can easily be determined from theTestRunRequest
.{test,runTest,subTest}AtCursor
andtest{CurrentFile,CurrentPackage,Workspace}
to work in terms of the test API/controller. For example,testAtCursor
would become "Find the TestItem corresponding to the cursor location" andrunTests(new vscode.TestRunRequest([testItemAtCursor]), testConfig)
.