Implement a preferences UI editor smoke test for Preferences turns off editor line numbers and verifies the live change

[bpasero]

Update

This was originally a test failure, but my suggestion in the end would be to come up with a settings smoke test that leverages the settings UI editor if possible.

Original:

https://dev.azure.com/monacotools/Monaco/_build/results?buildId=151025&view=logs&j=672276a2-8d3a-5fab-615d-090c51352f92&t=0699ae84-7245-5a45-5eee-80b086af2725&l=71

1) VSCode Smoke Tests (Electron)
       Preferences
         turns off editor line numbers and verifies the live change:
     Error: Timeout: get elements '.line-numbers' after 20 seconds.
      at poll (D:\a\1\s\test\automation\src\code.ts:109:10)
      at Code.waitForElements (D:\a\1\s\test\automation\src\code.ts:258:10)
      at Context.<anonymous> (src\areas\preferences\preferences.test.ts:23:4)

//cc @sandy081

[isidorn]

Also hit this https://dev.azure.com/monacotools/Monaco/_build/results?buildId=151056&view=logs&j=672276a2-8d3a-5fab-615d-090c51352f92&t=0699ae84-7245-5a45-5eee-80b086af2725

[bpasero]

I yield because I think this is somewhere in preferences/configuration land.

Analyzing what goes on, evidence by the screenshot is that line numbers are indeed not off:

As such, this is a correct test failure.

However, the smoke-test-runner.log shows that the steps for writing to the settings file have been followed:

[2022-01-20T08:56:13.715Z] dispatchKeybinding ctrl+shift+p
[2022-01-20T08:56:13.821Z] isActiveElement .quick-input-widget .quick-input-box input
[2022-01-20T08:56:13.828Z] setValue .quick-input-widget .quick-input-box input >workbench.action.openSettingsJson
[2022-01-20T08:56:13.980Z] getElements .quick-input-widget .quick-input-list .monaco-list-row.focused .monaco-highlighted-label
[2022-01-20T08:56:14.029Z] isActiveElement .quick-input-widget .quick-input-box input
[2022-01-20T08:56:14.069Z] dispatchKeybinding enter
[2022-01-20T08:56:14.176Z] getElements .quick-input-widget
[2022-01-20T08:56:14.198Z] click  .monaco-editor[data-uri$="settings.json"] .view-lines > .view-line:nth-child(1)
[2022-01-20T08:56:14.501Z] isActiveElement  .monaco-editor[data-uri$="settings.json"] textarea
[2022-01-20T08:56:14.544Z] dispatchKeybinding right
[2022-01-20T08:56:14.650Z] getElements  .monaco-editor[data-uri$="settings.json"]
[2022-01-20T08:56:14.674Z] isActiveElement  .monaco-editor[data-uri$="settings.json"] textarea
[2022-01-20T08:56:14.686Z] typeInEditor  .monaco-editor[data-uri$="settings.json"] textarea "editor.lineNumbers": "off",
[2022-01-20T08:56:14.743Z] getElements  .monaco-editor[data-uri$="settings.json"] .view-lines
[2022-01-20T08:56:14.771Z] dispatchKeybinding ctrl+s

So I end up being puzzled how this change could have not traveled to the editor. Is this depending on file events?

Full runner log: smoke-test-runner.log.zip

[bpasero]

I pushed a change to skip this test for now as it failed more in the past 24h.

[bpasero]

Oh, I had to skip the entire suite because I think the second test depends on the first (?)

[sandy081]

@bpasero I cant repro it and I cannot speculate what would have gone wrong. I can see that setting was changed and got updated but editor did not pick it. There could be many reasons like change events or could be editor issue. May I know what is recommended for such scenarios?

[bpasero]

How about some temporary logging only for this case? We could also temporarily expose a method on the window.driver that you can call to collect some more info, like e.g. the state of configuration at the time.

[sandy081]

I can add more logs for sure, but I do not know how the window.driver thingy works.

Can I enable these tests again and I will have this issue open to add more logs? Since we rely on automated smoke tests and it would be important to have as less skipped tests as possible during the release so that we don't miss some real obvious bugs?

[bpasero]

but I do not know how the window.driver thingy works

If you search for capturePage you can see an end to end of a method that is accessible from the smoke tests all the way into the running VSCode app where the method is implemented. Methods exposed from driver.ts can be used from smoke tests via the driver's IPC channel. You should be familiar with that way of implementing an IPC channel as it is the same used for all our other IPC communications 👍

Can I enable these tests again

Yes, please do.

[bpasero]

Failed in https://dev.azure.com/monacotools/Monaco/_build/results?buildId=152826&view=results

[sandy081]

I can reproduce it locally but not consistently. Here is the screenshot.

It seems, settings got saved but the editor has not picked up the setting. Looks like the editor has missed the event. @bpasero Is there any logging in the workbench editor while listening and updating the settings?

[bpasero]

@sandy081 no logging at the moment, the code is here:

vscode/src/vs/workbench/browser/parts/editor/textEditor.ts

Lines 67 to 72 in c8e2fd7

	this._register(this.textResourceConfigurationService.onDidChangeConfiguration(() => {
	const resource = this.getActiveResource();
	const value = resource ? this.textResourceConfigurationService.getValue<IEditorConfiguration>(resource) : undefined;
	return this.handleConfigurationChangeEvent(value);
	}));

We rely on a onDidChangeConfiguration event being fired when settings change.

[sandy081]

@bpasero Is it possible to get the recorded screenshots for this test from the build? I remember we used to have this before?

[sandy081]

I can confirm that when this test is failing, the configuration service is triggering the change event and the line numbers settings is off - I added a log statement in the location @bpasero pointed

TBH, I do not understand the consistency of this failure. One thing I observed is that, it is failing when I am switching focus from the editor. I am also not sure exactly at which point of time the focus has to be switched.

It seems handleConfigurationChangeEvent is not updating the change immediately if the editor is not visible.

vscode/src/vs/workbench/browser/parts/editor/textEditor.ts

Lines 84 to 90 in c8e2fd7

	protected handleConfigurationChangeEvent(configuration?: IEditorConfiguration): void {
	if (this.isVisible()) {
	this.updateEditorConfiguration(configuration);
	} else {
	this.hasPendingConfigurationChange = true;
	}
	}

This seems to be causing it too

[bpasero]

@sandy081

Is it possible to get the recorded screenshots for this test from the build?

Only from the web smoke tests that use Playwright unfortunately.

This seems to be causing it too

Can you send me over the steps you use to reproduce locally?

[sandy081]

As said, it was non trivial to repro this consistently. I have to say I am lucky to repro it locally after many trials and playing around with switching the focus, I could repro it and confirm that change event is being triggered. (I logged the configuration object during these trials). I will probably do more trials to understand more. I mentioned it here so that you might get a hunch of what is happening in editor land.

[bpasero]

Yeah please continue to log if you have a repro, the idea of the isVisible check was to buffer the setting change events if the editor is not visible and then only apply them once the editor becomes visible.

I wonder if we switch to the editor too soon after the configuration change has happened and then fail to apply it?

But I wonder: isn't the setting applied to the opened settings file as well? Why do we open another editor? Can we not just drop that part?