Some fixes for nightly by rchiodo · Pull Request #11802 · microsoft/vscode-python

Rich Chiodo (rchiodo) · 2020-05-13T23:26:05Z

Submitting nightly test fixes before they get too big.

Actually found some bugs here too.

Multiple interpreter test was catching a bug. Looks like we regressed switching the interactive window to a different interpreter in between openings

Fix functional tests to always use a valid interpreter (instead of just one with jupyter)

Rich Chiodo (rchiodo) · 2020-05-13T23:27:10Z

  - script: |
      call activate base
-      conda install --quiet -y --file build/ci/conda_base.yml
+      call conda install --quiet -y --file build/ci/conda_base.yml


call [](start = 6, length = 4)

Seems that on windows, conda will create a new command shell if you call it directly. This actually prevents the other commands after this point from running. This was the first bug in our flake tests.

Rich Chiodo (rchiodo) · 2020-05-13T23:27:29Z

 jupyter
 numpy
 matplotlib
+pip


Making sure pip comes from the conda directory

Rich Chiodo (rchiodo) · 2020-05-13T23:27:49Z

 yapf
 pylint
 pycodestyle
-prospector


I don't believe this is used anywhere and it was failing to install anyway.

Rich Chiodo (rchiodo) · 2020-05-13T23:28:11Z

-    public async createTemporaryFile(suffix: string): Promise<TemporaryFile> {
-        return this.utils.tmp.createFile(suffix);
+    public async createTemporaryFile(suffix: string, mode?: number): Promise<TemporaryFile> {
+        return this.utils.tmp.createFile(suffix, mode);


added an optional mode parameter to try and fix problems with the direct kernel on windows

Rich Chiodo (rchiodo) · 2020-05-13T23:28:45Z

-                    ? await this.notebookProvider.getOrCreateNotebook({ identity: identity.resource, metadata })
+                    ? await this.notebookProvider.getOrCreateNotebook({
+                          identity: identity.resource,
+                          resource,


Seems we weren't passing in the resource for creating the notebook. This is what broke picking an interpreter based on a resource.

If I'm reading this right it seems like this would have been broken for a while for both raw and jupyter then. I'm having trouble mapping this to the actual user scenario that would trigger this issue. Just surprised we've not run into this at all.

Yes it would have been broken.

The user scenario is you

Open multiroot workspace

Open interactive window. It uses current interpreter for whatever folder you have open

Close interactive window.

Switch to another file

Open interactive window. It would continue to use the first interpreter. It shouldn't

That's what the test was supposed to verify.

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

Glad we had a test for it then. Maybe I need to play around with multiroot more often.

Rich Chiodo (rchiodo) · 2020-05-13T23:29:57Z

        sendTelemetryEvent(Telemetry.OpenedInteractiveWindow);
-
-        // Start the server as soon as we open
-        this.ensureConnectionAndNotebook().ignoreErrors();


Additionally this was breaking the create notebook with a resource because for the interactive window, it frequently can't compute the resource until after a user submits code. So starting the notebook before showing the window makes this a race. Moved this to 'show' and now it will always happen after the user adds code.

Rich Chiodo (rchiodo) · 2020-05-13T23:30:52Z

            (variable as any).frameId
        );
-        if (results) {
+        if (results && results.result) {


This was unnecessarily crashing sometimes (when debugpy doesn't return anything because the value may not evaluate). Checking this here lets the value just exist as is.

Rich Chiodo (rchiodo) · 2020-05-13T23:31:21Z


    private async createAndUpdateConnectionFile() {
-        this.connectionFile = await this.file.createTemporaryFile('.json');
+        this.connectionFile = await this.file.createTemporaryFile('.json', 0o777);


I should put a comment here. Not sure if this fixes the direct kernel problem or not, but going to wait and see.

That's odd. So the permission on the temp connection file were preventing the delete and recreate?

Not sure. But the failure point was when it attempted to open it with a restricted set of permissions. After I made this change I haven't seen the failure again, but it could be just luck

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

Rich Chiodo (rchiodo) · 2020-05-13T23:33:13Z

                    suppressShutdownErrors(session.kernel);
                    // Shutdown may fail if the process has been killed
-                    await waitForPromise(session.shutdown(), 1000);
+                    if (!session.isDisposed) {


Not necessary to wait for shutdown if the session is already disposed

Rich Chiodo (rchiodo) · 2020-05-13T23:33:33Z

+        const disposable = this.sessionTerminatedEvent.event(() => response.resolve());
        this.protocolParser.once(`response_${command}`, (resp: any) => {
            this.sendToTrackers(resp);
+            traceInfo(`Received response from debugger: ${JSON.stringify(args)}`);


Added this to trace random failures in debugging. Still working on this.

Rich Chiodo (rchiodo) · 2020-05-13T23:34:15Z

        try {
            await fsextra.remove(filename);
        } catch (err) {
-            if (!(await fsextra.pathExists(filename))) {


I think this is causing random failures on windows for functional tests. But it's not necessary.

Rich Chiodo (rchiodo) · 2020-05-13T23:35:13Z

        return '';
    }

-    private async hasJupyter(interpreter: PythonInterpreter): Promise<boolean | undefined> {


Turns out having jupyter is too lenient. When testing we want only those interpreters that have everything installed that we need. (Seems azure machines can have jupyter in other environments too).

Rich Chiodo (rchiodo) · 2020-05-13T23:35:37Z


-                if (pythonService) {
+                // Skip test for older python and on windows. Getting E_PROTO on windows.
+                if (pythonService && os.platform() !== 'win32') {


For now self certs seems to always fail on windows.

Rich Chiodo (rchiodo) · 2020-05-13T23:37:59Z

It should be noted: this will not fix all of the flake tests. Just some of them (like 7 or so).

I'm going to continue working on fixing the other failures.

Ian Huff (IanMatthewHuff) · 2020-05-13T23:48:50Z

+                        interpreter,
+                        allowEnvironmentFetchExceptions: true
+                    });
+                return pythonProcess.isModuleInstalled('livelossplot'); // Should we check all dependencies?


I'm guessing the comment here is since livelossplot is standing in for everything in conda-functional-requirements.txt? Seems ok for test code, having to check them all seems like a bit of overkill.

Yeah. Livelossplot seemed uncommon for other things to install, so seemed like a good candidate for checking.

This does mean that if you run the functional tests locally, you'll have to install livelossplot into a least one environment for most of the tests to pass with real jupyter.

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

Since we're on the topic, what do you do for setting up your local test env to run all the real python tests? Just this:
conda install --quiet -y --file build/ci/conda_base.yml
python -m pip install --upgrade -r build/conda-functional-requirements.txt

in a Conda env? Wasn't as sure since there were multiple conda envs getting set up as well in the test steps.

No I do it more manually.

conda create --name newEnvName python=3.7 jupyter pip

conda activate newEnvName

python -m pip install --upgrade -r build/conda-functional-requirements.txt

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

Codecov (codecov-io) · 2020-05-13T23:53:27Z

Codecov Report

Merging #11802 into master will decrease coverage by 0.21%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master   #11802      +/-   ##
==========================================
- Coverage   60.65%   60.43%   -0.22%     
==========================================
  Files         628      631       +3     
  Lines       33954    34155     +201     
  Branches     4781     4798      +17     
==========================================
+ Hits        20594    20641      +47     
- Misses      12363    12512     +149     
- Partials      997     1002       +5

Impacted Files	Coverage Δ
src/client/common/platform/types.ts	`100.00% <ø> (ø)`
.../datascience/interactive-common/interactiveBase.ts	`5.57% <0.00%> (+0.01%)`	⬆️
...atascience/interactive-window/interactiveWindow.ts	`15.62% <0.00%> (ø)`
src/client/datascience/types.ts	`100.00% <ø> (ø)`
src/client/datascience/baseJupyterSession.ts	`52.94% <50.00%> (-0.32%)`	⬇️
...datascience/interactive-common/notebookProvider.ts	`48.14% <50.00%> (ø)`
src/client/common/platform/fileSystem.ts	`80.97% <100.00%> (ø)`
src/client/common/platform/fs-temp.ts	`100.00% <100.00%> (ø)`
src/datascience-ui/react-common/arePathsSame.ts	`75.00% <0.00%> (-12.50%)`	⬇️
src/client/common/utils/platform.ts	`64.70% <0.00%> (-11.77%)`	⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db62755...3052fcf. Read the comment docs.

sonarqubecloud · 2020-05-14T00:29:12Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

Rich Chiodo (rchiodo) added 7 commits May 12, 2020 15:53

Fix debugger tests to exit appropriately

3414a6b

Fix functional tests to always use a valid interpreter (instead of just one with jupyter)

Push pip to test environments

32f1483

Try another fix for installing

46f4432

Try again to fix conda install

d523a42

Try to fix connection file problem with raw kernel

3bb2a06

Don't throw on cleanup

f55faf0

Fix multiple interpreters for the interactive window

5f20294

Rich Chiodo (rchiodo) added the no-changelog No news entry required label May 13, 2020

Rich Chiodo (rchiodo) requested review from David Kutugata (DavidKutu), Don Jayamanne (DonJayamanne), Ian Huff (IanMatthewHuff) and Jim Griesmer (greazer) May 13, 2020 23:26

Rich Chiodo (rchiodo) self-assigned this May 13, 2020

Rich Chiodo (rchiodo) commented May 13, 2020

View reviewed changes

Add comment

c97a5c0

Rich Chiodo (rchiodo) commented May 13, 2020

View reviewed changes

Ian Huff (IanMatthewHuff) reviewed May 13, 2020

View reviewed changes

David Kutugata (DavidKutu) approved these changes May 14, 2020

View reviewed changes

Fix kernel launcher test to not fail

4e4cc34

Ian Huff (IanMatthewHuff) approved these changes May 14, 2020

View reviewed changes

Fix unit test failures

3052fcf

Rich Chiodo (rchiodo) merged commit 1c5080e into master May 14, 2020

Rich Chiodo (rchiodo) deleted the rchiodo/nightly_fix branch May 14, 2020 16:20

Conversation

Rich Chiodo (rchiodo) commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rich Chiodo (rchiodo) commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Codecov (codecov-io) commented May 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud Bot commented May 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rich Chiodo (rchiodo) May 13, 2020 •

edited

Loading

Codecov (codecov-io) commented May 13, 2020 •

edited

Loading