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

Infrastructure for mounting more than one webview at a time #12382

Merged
merged 11 commits into from
Jun 16, 2020

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Jun 16, 2020

For #11445

In order to get a functional test that opens an interactive window and a notebook, I had to refactor all of our webview stuff in the tests to not assume a single instance.

This PR is about creating just this change (test isn't there yet for testing both at the same time).

This was accomplished by creating a new service that mounts the webview.

@rchiodo rchiodo self-assigned this Jun 16, 2020
@rchiodo rchiodo added the no-changelog No news entry required label Jun 16, 2020
@rchiodo
Copy link
Author

rchiodo commented Jun 16, 2020

Forgot to mention, I also upgraded Mocha.

This was necessary to fix this bug:
mochajs/mocha#2148

Skipped tests were causing the mounted web panels to not be cleaned up because the setup function for the skipped tests would run but the teardown wouldn't.

@@ -75,11 +75,11 @@ suite('Daemon', () => {
);
});
teardown(() => {
pythonProc.kill();
pythonProc?.kill();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pythonProc?.kill();
pythonProc?.kill(); // NOSONAR

if (connection) {
connection.dispose();
}
pythonDaemon.dispose();
pythonDaemon?.dispose();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pythonDaemon?.dispose();
pythonDaemon?.dispose(); // NOSONAR

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, guess we have ignored Sonar analysis for some files

const response = waitForMessageResponse(ioc, () => exportButton!.simulate('click'));
await waitForPromise(response, 100);
exportButton!.simulate('click');
await sleep(100);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change and the one below (copyToSource) seem like ordering changes. Were the waits just not needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got rid of waitForMessageResponse. It was rather hacky. However in this case the wait for not doing export still has to be there.


In reply to: 441176542 [](ancestors = 441176542)

@sonarcloud
Copy link

sonarcloud bot commented Jun 16, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rchiodo rchiodo merged commit 9ab353a into master Jun 16, 2020
@rchiodo rchiodo deleted the rchiodo/double_functional branch June 16, 2020 23:14
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants