Allow testCaseRecorder to be nullable

[bjaspan]

No description provided.

[bjaspan]

This isn't done yet.

[bjaspan]

Commit 6d3f4ec is what I was aware was missing.

[pokey]

Hmm this all feels kinda wrong to me:

• I don't like returning a null test case recorder from the engine; we should inject it from externally instead
• The commands will still appear in the menu, which is not great; we should use a when clause context to deactivate them. Arguably this is not much worse than today, where the commands appear in the menu but will throw an error message
• If we register commands in package.json, it feels like bad practice not to register them in code. We should deactive them with a when clause context to deactivate them in package.json and then have a dummy impl that throws an exception if they happen to use a workaround and run them programmatically

In our original slack discussion, I thought it was just limited to runCommand; didn't see the full picture here. I don't like the idea of moving in a suboptimal direction here

Do you have an easier hack you can do in your patch for now?

[bjaspan]

Well, since I'm not just removing the symbol & code for TestCaseRecorder entirely, I have to define it to be something, so I'm locally patching it to be an empty class. I could instead locally patch it to be a no-op class. This gets pretty close to just defining an interface and injecting it.

I like the idea of moving it (and perhaps other test code) to a separate package. I haven't considered what boilerplate is involved in doing that myself yet.

[bjaspan]

Do you think the test case recorder commands should be disabled when isWeb or when some Google/Cider-specific context key is set? isWeb would be cleaner since the when clause will appear in packages.json, but I'm not sure if you aspire to have the recorder work on the web.

[pokey]

Do you think the test case recorder commands should be disabled when isWeb or when some Google/Cider-specific context key is set? isWeb would be cleaner since the when clause will appear in packages.json, but I'm not sure if you aspire to have the recorder work on the web.

The test case recorder only works when you're running the extension in development mode, so I'm happy to have it switched off on web. I don't think we need to support our devs recording new test cases from web; certainly not in the near term

Btw is isWeb a thing? I thought it was virtualWorkspace

[bjaspan]

It looks like either would work: https://code.visualstudio.com/api/references/when-clause-contexts#available-context-keys.

[pokey]

ah ok cool. I think in this case virtual filesystem is more specific because that's the primary reason we can't use the test case recorder; we have nowhere to put the tests. But I don't feel strongly

[pokey]

Closing in favour of #2002