Refactor testing support

[firelizzard18]

I mentioned this in #1641. I am opening a separate issue for discussing a specific change.

I would like to refactor goTest, testUtil, and goTestExplorer to all work together:

• Rework goTest(tc: TestConfig) into runTests(rq: vscode.TestRunRequest, tc: TestConfig) and remove the properties of TestConfig that can easily be determined from the TestRunRequest.
• Refactor {test,runTest,subTest}AtCursor and test{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" and runTests(new vscode.TestRunRequest([testItemAtCursor]), testConfig).

[firelizzard18]

@hyangah @suzmue, is this something you support? I don't want to start work if this is not a direction you want to go.

[hyangah]

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!

[firelizzard18]

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.

[gopherbot]

Change https://golang.org/cl/343878 mentions this issue: src/goTest: prepare for refactoring TestConfig

[firelizzard18]

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

1. Create NewTestConfig as a subset of TestConfig
   • Add include, exclude, and profileKind from TestRunRequest¹
   • Drop dir, functions - determined from include/exclude
   • Drop applyCodeCoverage - determined from profileKind
   • Drop includeSubDirectories - including a module or workspace will run recursively
   • Drop isMod - 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 chain
2. Create a function, computeTestConfig that turns a single-invocation NewTestConfig into a TestConfig
3. Update the test explorer to use computeTestConfig
   • The test explorer will still be responsible for splitting a test run request into single-invocation chunks
4. Update the various calls to goTest to use computeTestConfig
   • This can be done progressively across multiple CLs
5. Update goTest to use TestConfig directly and incorporate the functionality of computeTestConfig into it (or more likely, make it a helper function).
6. Eliminate TestConfig/drop the 'New' from NewTestConfig

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

[firelizzard18]

CL 343878 has an initial implementation of computeTestConfig

[hyangah]

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.

[firelizzard18]

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.

[firelizzard18]

@hyangah Given Microsoft's response to microsoft/vscode#130796 (comment), 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.

[firelizzard18]

It appears that Theia will not be supporting the testing API for some time (eclipse-theia/theia#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?

[hyangah]

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.

[firelizzard18]

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.

[hyangah]

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.

[firelizzard18]

Yeah, I expect the workarounds would only get more cumbersome as time goes on.

[firelizzard18]

#3523 currently includes the first item (reworking/merging goTest) and I would like to add support for "run test at cursor"

[firelizzard18]

VSCode already has commands for run/debug the test at the cursor so all that remains (after #3523) is test{CurrentFile,CurrentPackage,Workspace}

[firelizzard18]

This will be OBE when Go Companion's testing code is merged so I'm going to close it.