Skip to content
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

Seeing random text output in integration tests #84283

Closed
alexdima opened this issue Nov 8, 2019 · 12 comments
Closed

Seeing random text output in integration tests #84283

alexdima opened this issue Nov 8, 2019 · 12 comments
Assignees
Labels
Milestone

Comments

@alexdima
Copy link
Member

@alexdima alexdima commented Nov 8, 2019

  • run only \extensions\vscode-api-tests\out\singlefolder-tests by commenting out the rest of the test-integration.(bat|sh)
  • observe random output:
    image

I have tracked this down to this line:
https://github.com/microsoft/vscode/blame/617a3387d41c2df65a899e8aaea773d8057751fc/src/vs/workbench/services/bulkEdit/browser/bulkEditService.ts#L430

It would be good when the integration tests have a clean human readable output to make it easy for us to spot things in there.

@alexdima

This comment has been minimized.

Copy link
Member Author

@alexdima alexdima commented Nov 8, 2019

Hm, maybe we can add an environmental variable to indicate integration tests running and then we can disable the error logging for this case if integration tests are running...

@alexdima

This comment has been minimized.

Copy link
Member Author

@alexdima alexdima commented Nov 8, 2019

I see all kind of other "useful" crap in the output:

  • [vscode.vscode-api-tests] Accessing a resource scoped configuration without providing a resource is not expected. To get the effective value for '[abcLang]', provide the URI of a resource or 'null' for any resource.
  • The task [customTesting, First custom task] uses an undefined task type. The task will be ignored in the future.
  • [Deprecation Warning] method 'TextEditor.hide' is deprecated and should no longer be used. Refer to the documentation for further details.
  • vscode.vscode-api-tests created a webview without a content security policy: https://aka.ms/vscode-webview-missing-csp

@jrieken What do you think... ?

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Nov 8, 2019

Yeah, not so easy... Things that are on the renderer should use the log service and for that could do something, e.g. mute it or at least not print it to the console. In the extension land we do use the console to "communicate" with extension developers. The easiest might really be to disconnect the extension host console when running tests. I do believe that extension host already know when it being run tests - @bpasero can you confirm that?

@jrieken jrieken added the engineering label Nov 9, 2019
@bpasero

This comment has been minimized.

Copy link
Member

@bpasero bpasero commented Nov 9, 2019

Yeah, ext host does know via the --extensionDevTestPath CLI thingy.

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Nov 11, 2019

Adding people based on console.[log|warn|...] usages. Please replace usages with ILogService which can get injected

  • /src/vs/workbench/api/common/extHost.api.impl.ts @jrieken
  • /src/vs/workbench/api/common/extHostConfiguration.ts @sandy081
  • /src/vs/workbench/api/common/extHostDecorations.ts @jrieken
  • /src/vs/workbench/api/common/extHostDiagnostics.ts @jrieken
  • /src/vs/workbench/api/common/extHostDocumentData.ts @jrieken
  • /src/vs/workbench/api/common/extHostExtensionActivator.ts @alexandrudima
  • /src/vs/workbench/api/common/extHostExtensionService.ts @alexandrudima
  • /src/vs/workbench/api/common/extHostLanguageFeatures.ts @jrieken
  • /src/vs/workbench/api/common/extHostMessageService.ts @jrieken
  • /src/vs/workbench/api/common/extHostRequireInterceptor.ts @jrieken
  • /src/vs/workbench/api/common/extHostSCM.ts @joaomoreno
  • /src/vs/workbench/api/common/extHostTask.ts @alexr00
  • /src/vs/workbench/api/common/extHostTextEditor.ts @jrieken
  • /src/vs/workbench/api/common/extHostTreeViews.ts @alexr00
  • /src/vs/workbench/api/common/extHostWebview.ts @mjbvz
  • /src/vs/workbench/api/node/extHostCLIServer.ts @aeschli
  • /src/vs/workbench/api/node/extHostDebugService.ts @weinand
  • /src/vs/workbench/api/node/extHostExtensionService.ts @alexandrudima
  • /src/vs/workbench/api/node/extHostOutputService.ts @sandy081
  • /src/vs/workbench/api/node/extHostStoragePaths.ts @jrieken
  • /src/vs/workbench/api/node/extHostTask.ts @alexr00
@alexr00 alexr00 self-assigned this Nov 11, 2019
jrieken added a commit that referenced this issue Nov 11, 2019
jrieken added a commit that referenced this issue Nov 11, 2019
jrieken added a commit that referenced this issue Nov 11, 2019
sandy081 added a commit that referenced this issue Nov 11, 2019
weinand added a commit that referenced this issue Nov 11, 2019
jrieken added a commit that referenced this issue Nov 11, 2019
jrieken added a commit that referenced this issue Nov 11, 2019
alexr00 added a commit that referenced this issue Nov 12, 2019
mjbvz added a commit that referenced this issue Nov 13, 2019
joaomoreno added a commit that referenced this issue Nov 13, 2019
related to #84283
alexdima added a commit that referenced this issue Nov 21, 2019
@jrieken jrieken assigned aeschli and sandy081 and unassigned alexr00 Dec 6, 2019
@jrieken jrieken added this to the December 2019 milestone Dec 6, 2019
jrieken added a commit that referenced this issue Dec 9, 2019
@jrieken jrieken removed their assignment Dec 9, 2019
jrieken added a commit that referenced this issue Dec 9, 2019
sandy081 added a commit that referenced this issue Dec 10, 2019
@sandy081 sandy081 removed their assignment Dec 10, 2019
bpasero added a commit that referenced this issue Dec 10, 2019
sandy081 added a commit that referenced this issue Dec 10, 2019
@jrieken jrieken self-assigned this Dec 20, 2019
@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Dec 20, 2019

@bpasero re #87414 (comment) - it seems that calling ILogService#error during integration tests prints to the console? As the "runner" of tests can you prevent that from happening?

@bpasero

This comment has been minimized.

Copy link
Member

@bpasero bpasero commented Dec 20, 2019

@sandy081 can that be configured somehow?

Tests are executed from here:

private async _doHandleExtensionTests(): Promise<void> {

@bpasero

This comment has been minimized.

Copy link
Member

@bpasero bpasero commented Dec 20, 2019

So, commenting out the following line removes various output for me:

consoleFn = console.error;

However, this is also the place where we report the output from the test run itself (e.g. if a test succeeds or not) so it may be a bit risky to simply block everything.

@sandy081

This comment has been minimized.

Copy link
Member

@sandy081 sandy081 commented Dec 30, 2019

AFIK it cannot be configured

@jrieken

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Jan 3, 2020

re #84283 (comment) it feels like test results and logging should be separated. I don't see another way of getting this fixed. Now, the logging service instead of console is used (and believe that's a good thing) but ironically that made those console messages worst

@jrieken jrieken removed their assignment Jan 3, 2020
bpasero added a commit that referenced this issue Jan 6, 2020
@bpasero

This comment has been minimized.

Copy link
Member

@bpasero bpasero commented Jan 6, 2020

I have pushed a small change which will make errors render properly when logged. Instead of seeing the raw file operation error as JSON, the output is now easier to read using the message of the error:

image

Still, I am not seeing a good way to prevent this output from showing up in the integration tests. The integration tests run VSCode in full and as such there is potential for a lot more output in the tests compared to normal unit tests. If we had console.log usages in our unit tests we would see the same. I think this output is still useful to diagnose issues from users so just removing it does not seem like a good option either.

I ran a quick test how the integration tests looked like in earlier versions (prior 1.39) and I am seeing the same output, though well hidden, similar to this output which seems to be coming from the renderer Chrome console:

image

I think the move towards ILogService is good anyway and there are still some usages looking at vs/workbench/api (@jrieken not sure if your list in #84283 (comment) covered them already):

image

@alexdima since you are the author of the issue, I am open for ideas how to improve this further if you have any.

@aeschli aeschli removed their assignment Jan 14, 2020
bpasero added a commit that referenced this issue Jan 15, 2020
@bpasero

This comment has been minimized.

Copy link
Member

@bpasero bpasero commented Jan 15, 2020

There is now a way in extension tests to disable logging for the duration of the test (withLogDisabled). Using this for some of the tests, the file service output is no longer printed.

Filed #88671 and #88672 for remaining output.

@bpasero bpasero closed this Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.