Tests API implementation

[D-Zaq]

What it does

Initial setup for the VSCode tests API implementation by the students team from Polytechnique Montreal.

Issue adressed

Feature description of the issue: Add tests namespace from VSCode API #10669

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[paul-marechal]

If you look at the CI output there are compilation errors, please fix.

[mathblin]

Thank you for the feedback. Like you both said, it was related to the package file in plugin-ext.
We were able to make it compile locally from a fresh clone.
We'll rebase the history for the conflicts soon.

[JonasHelming]

@tsmaeder

[mathblin]

Can anybody confirm that this kind of import is ok in Theia?
import { parse } from '@theia/monaco-editor-core/esm/vs/base/common/marshalling';

see in packages/plugin-ext/src/plugin/type-converters.ts

@colin-grant-work , @paul-marechal

Also, after doing the git clean -xffd command, we could compile without errors. However, we saw there was another commit on master today. We will rebase later on.

[colin-grant-work]

Can anybody confirm that this kind of import is ok in Theia?
import { parse } from '@theia/monaco-editor-core/esm/vs/base/common/marshalling';

Yes, that should work fine, assuming the corresponding function is in fact available in the monaco-editor-core code.

[paul-marechal]

You currently have errors related to some import: D:\a\theia\theia\node_modules\@theia\monaco-editor-core\src\vs\base\common\marked\marked.js (from the CI build failures).

Note the /src/ part in the path: you should not import anything from there as it is where the TypeScript source files are located, and you want to import JavaScript files instead. You may want to replace src by esm.

[paul-marechal]

Suggested change

	import * as marked from '@theia/monaco-editor-core/src/vs/base/common/marked/marked';
	import * as marked from '@theia/monaco-editor-core/esm/vs/base/common/marked/marked';

[D-Zaq]

Hi @paul-marechal, we have in fact tried importing marked with the JavaScript file from esm instead of the Typescript from src like the other imports but that generates compilation errors (as shown in the screenshot below) suggesting to add the marked.d.ts file in the marked directory where marked.js is found following the esm path which is a temporary fix as it would result in the same error with a fresh build. We don't know the reason of this and we still didn't find a permanent solution :

[colin-grant-work]

The reason is that the marked files in esm have no sourcemap because marked isn't compiled from source. marked is a library that VSCode has in-sourced by adding the compiled JS from a particular commit of marked. That means there's no source map in esm, so TS can't find its way back to the d.ts in the sources. There may be some TS mechanism to import from esm (definitely the correct idea) and still point to the d.ts in the src directory. though.

[mathblin]

Could it be possible to launch the pipeline again? We just rebased origin/master and it is working locally for yarn­ and yarn lint

[JonasHelming]

sure, running!

[mathblin]

Can anybody confirm that this kind of import is ok in Theia?
import { parse } from '@theia/monaco-editor-core/esm/vs/base/common/marshalling';

Yes, that should work fine, assuming the corresponding function is in fact available in the monaco-editor-core code.

Thanks for the reply. It is always appreciated and most responses lead us to helping solve the problems that we face.

However, as discussed in the dev-meeting as of 2023-03-21, some tests now fails due to imports like this from monaco-editor-core code. Maybe it is linked to browser code as in code that is not specific to browser maybe?

At this point, we are not sure if we should replace all monaco-editor-core code imports by real code lines in Theia or at least in appropriate parts. We would also like to know if anyone could help us define these appropriate parts. Could it be that these imports from monaco-editor-core are reserved for files that are created in or used by main implementations. For example, files created in or used in this repository.

By the way, once Theia compiled, whether it is with Electron or Browser, the plugins crashes in runtime. We think this crash is related to the same issue that makes the tests fail.

[paul-marechal]

About the plugin host crash, I get the following error in my console when trying this PR:

root ERROR [hosted-plugin: 39724] E:\sources\theia.banana\node_modules\@theia\monaco-editor-core\esm\vs\editor\standalone\browser\standalone-tokens.css:8
.monaco-editor {
^

SyntaxError: Unexpected token '.'
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1032:15)
    at Module._compile (node:internal/modules/cjs/loader:1067:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1157:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:1005:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (E:\sources\theia.banana\node_modules\@theia\monaco-editor-core\esm\vs\editor\standalone\browser\standaloneEditor.js:8:1)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)

Something is trying to import a CSS file from the plugin host and it is causing this problem.

Note that Theia frontend code can import .css files because these imports are converted to something that works by Webpack upon bundling. Beside the frontend we should never somehow import CSS files in TS/JS scripts.

[paul-marechal]

@colin-grant-work do I guess correctly that @theia/monaco-editor-core is a browser package? As in we'd have issues trying to import it from node runtimes?

[paul-marechal]

@mathblin whatever you import from that package you need to make sure to not import .../browser/... files from code that is expected to run in the plugin host process.

[paul-marechal]

@mathblin I put a breakpoint in node_modules\@theia\monaco-editor-core\esm\vs\editor\standalone\browser\standaloneEditor.js on the require("./standalone-tokens.css"); line and here's the stack I got:

packages\testing\src\common\test-types.ts imports symbols from @theia/monaco-editor-core/esm/vs/editor/editor.api which in turn imports things from these browser APIs.

The fix would be to not import symbols from @theia/monaco-editor-core/esm/vs/editor/editor.api because of its side effects.

[colin-grant-work]

@colin-grant-work do I guess correctly that @theia/monaco-editor-core is a browser package? As in we'd have issues trying to import it from node runtimes?

Not exclusively. It includes a number of folders from the Code OSS repo in common areas, and those should be safe to use in Node contexts. Anything in a browser folder, however, should not be used in Node. In this case, the following imports should work:

import { IRange, Range } from '@theia/monaco-editor-core/esm/vs/editor/common/core/range';
import { IPosition } from '@theia/monaco-editor-core/esm/vs/editor/common/core/position';
import { URI as Uri, UriComponents } from '@theia/monaco-editor-core/esm/vs/base/common/uri';
import { IMarkdownString } from '@theia/monaco-editor-core/esm/vs/base/common/htmlContent';

[mathblin]

Hello @paul-marechal @colin-grant-work @marcdumais-work ,

I was curious to know if anyone could help us fix this issue with inversify:
Since we started implementing the tests-main.ts in packages/plugin-ext/src/main/browser/, we had problems with binding of a class/interface which inversify says it does not exist or has not been binded to what we understand is the container module.
In short, we can compile without errors with the following commands: yarn and yarn browser build. The problem is at run time reproducible with the command yarn browser start and opening the browser at localhost:3000/. We did not test with electron directly, but we expect a similar behaviour.
Here is the screenshot of the problem:

As we can see at the end of the first line, there is a binding problem with the error: Error: No matching bindings found for serviceIdentifier: TestService

The error is also reproductible with a test file. One can go in packages/testing/ folder and use command yarn test. Here is a similar screenshot:

The test is made with the same class:

export class TestService ...

Important: You'll have to use this fork of eclipse theia : https://github.com/D-Zaq/theia
We did not push it in the PR because the PR compiles and doesn't break any code.

Edit: the branch is polymtl/tests-api-main-binding

Well, any help would be appreciated.

[paul-marechal]

@mathblin I only quickly looked over why the tests-frontend-module.spec.ts would be failing and the reason is that TestService has a lot of dependencies that require binding in the Inversify container for an instance of TestService to be resolvable.

From test-service-impl.ts:

    constructor(
        @inject(IContextKeyService) contextKeyService: IContextKeyService,
        @inject(IInstantiationService) instantiationService: IInstantiationService,
        @inject(IStorageService) private readonly storage: IStorageService,
        // @IEditorService private readonly editorService: IEditorService,
        @inject(ITestProfileService) private readonly testProfiles: ITestProfileService,
        @inject(INotificationService) private readonly notificationService: INotificationService,
        // @IConfigurationService private readonly configurationService: IConfigurationService,
        @inject(ITestResultService) private readonly testResults: ITestResultService,
        @inject(IWorkspaceTrustRequestService) private readonly workspaceTrustRequestService: IWorkspaceTrustRequestService,
    ) {

This is all the dependencies of your components, and all need to have a valid binding for TestService to be instantiable.

[paul-marechal]

Now I'm also a bit queasy about some of the service identifiers you are using: IContextKeyService seems to come from @theia/monaco-editor-core and is a VS Code specific abstraction. Inversify seems ok with its value being used as a service identifier because it happens to be a function, but something tells me you should be using Theia's bindings and abstractions over things coming straight from the monaco editor? I'm just not really sure about directly using these symbols.

edit: Note that even if you try to inject using these symbols, unless you have them bound in this runtime's Inversify container then nothing will happen and Inversify will fail to resolve the injections and instantiations.

In this specific case we have a ContextKeyService component from @theia/core that you should be able to use.

[paul-marechal]

Tried looking at the TestService dependencies and where to look:

• I don't think you should need IInstantiationService, or convert what it is used for by using Inversify instead?
• I found a StorageService API within Theia but it doesn't seem to do everything you want it to?
• It seems like you have an @injectable() implementation for ITestProfileService, so you should use that.
• I think instead of INotificationService Theia has a MessageService API that fulfills the same role?
• There seems to be an @injectable() implementation for ITestResultService, use that.
• I don't think Theia supports an API similar to what IWorkspaceTrustRequestService does yet.

For better or for worse, we define and implement our own APIs/abstractions instead of directly using what third-party libraries might use (e.g. @theia/monaco-editor-core is a third-party library).

I think it's especially important when creating whole new components like TestService. If I understand correctly the current dependencies are directly inspired from the VS Code implementation? Then it makes sense why you'd try to find the same abstractions from @theia/monaco-editor-core, but Theia offers a different internal API and we must use that instead.

[colin-grant-work]

To supplement what @paul-marechal has said, it is possible to integrate services available from @theia/monaco-editor-core into the inversify dependency injection system, but it is tricky. You can see a couple of places where we've done it by searching for decorate(injectable() in the monaco package, but you have to be very careful, because, with dependency injection, instantiating one thing may instantiate a bunch of other things you didn't realize were necessary, and you have to take their lifecycles (and whether you - or anyone else - might want to override them) into account. So trying to use as many things as possible that are implemented in Theia rather than in monaco-editor-core is likely the right way to go, or delegating, but carefully.

[mathblin]

Thank you all for the quick responses. We'll look into them in the next few days and we'll also try to answer your comments (which are helpful) as fast as possible.

[mathblin]

Now I'm also a bit queasy about some of the service identifiers you are using: IContextKeyService seems to come from @theia/monaco-editor-core and is a VS Code specific abstraction. Inversify seems ok with its value being used as a service identifier because it happens to be a function, but something tells me you should be using Theia's bindings and abstractions over things coming straight from the monaco editor? I'm just not really sure about directly using these symbols.

edit: Note that even if you try to inject using these symbols, unless you have them bound in this runtime's Inversify container then nothing will happen and Inversify will fail to resolve the injections and instantiations.

In this specific case we have a ContextKeyService component from @theia/core that you should be able to use.

I think we can use the ContextKeyService as you suggested. It does not make me any compile errors if I use it instead of IInstantiationService (with some internal changes in Tests API of course). However, I am not sure of the behaviour thereafter. To be honest, even if we succeed in making all these changes, there will probably be a lot to modify after. We'll do our best anyway.

[mathblin]

are implemented in Theia rather than in monaco

I think you are absolutely right about avoiding the use of monaco and focus more on what's already implemented in Theia. At first, we used it because we saw some examples of it (that we didn't fully understand). However, it gave us more problems than expected. We are still figuring out ways to avoid it, which in part would mean importing a lot of new lines of code or changing what we already imported.

For the first point, I don't think we should try the decorate(injectable() solution unless we have no other choice. We'll keep it in mind though. Maybe we'll make something out of it.

[MatthewKhouzam]

How do we re-trigger? the ECA looks fixed.

[JonasHelming]

I triggered

[D-Zaq]

@JonasHelming can we re-triggre again 🙏🏼 ?

[KevinClapperton]

There was an update to the function declaration called createRunProfile in this PR. The current code doesn't migrates the new implementation that goes with the updated declaration. I will see what I can do on Tuesday

[KevinClapperton]

I restored the commit before the changes from the new Test API stub, but keep the correction of headers, comments and links to imported code. Also, made sure to double check the build, tests and lint.

[tsmaeder]

Incorporated and superseded by #12935