Submit tests for intellisense (replacing functional tests we had) #7565

rchiodo · 2021-09-16T19:33:38Z

I worked on this a week ago and was waiting on the python extension to submit changes I made. They should be there in the python insiders bits.

However I also want these tests in place for the work I'm doing to move intellisense to the jupyter extension. Once these are in, I'll make changes/add to them to test the work I'm doing to create a language server per interpreter.

codecov-commenter · 2021-09-16T20:09:58Z

Codecov Report

Merging #7565 (e3ef4a7) into main (ddda5fa) will increase coverage by 2%.
The diff coverage is 86%.

@@          Coverage Diff           @@
##            main   #7565    +/-   ##
======================================
+ Coverage     66%     68%    +2%     
======================================
  Files        365     364     -1     
  Lines      22659   22555   -104     
  Branches    3444    3429    -15     
======================================
+ Hits       15115   15497   +382     
+ Misses      6260    5733   -527     
- Partials    1284    1325    +41

Impacted Files	Coverage Δ
...atascience/interactive-window/interactiveWindow.ts	`54% <ø> (-1%)`	⬇️
src/client/datascience/notebook/types.ts	`100% <ø> (ø)`
...cience/notebook/intellisense/completionProvider.ts	`61% <66%> (+7%)`	⬆️
.../datascience/notebook/notebookControllerManager.ts	`84% <80%> (-1%)`	⬇️
src/client/datascience/constants.ts	`99% <100%> (+<1%)`	⬆️
src/client/datascience/jupyter/jupyterNotebook.ts	`25% <100%> (+<1%)`	⬆️
...t/datascience/notebook/vscodeNotebookController.ts	`77% <100%> (-3%)`	⬇️
...ient/datascience/jupyter/kernels/kernelSelector.ts	`57% <0%> (-24%)`	⬇️
...ience/notebook/emptyNotebookCellLanguageService.ts	`72% <0%> (-4%)`	⬇️
...rc/client/datascience/notebookStorage/baseModel.ts	`70% <0%> (-4%)`	⬇️
... and 42 more

DonJayamanne · 2021-09-17T00:08:09Z

src/client/datascience/notebook/intellisense/completionProvider.ts

            return [];
+        } else {
+            traceInfoIf(isCI, `Completions found, filtering the list: ${JSON.stringify(result)}.`);


I think we should just turn this into traceIfCI, lol, feels kinda redundant to always pass in isCI. What do you think.
We might have a few places where we'd have some other conditions being passed in, but i think traceIfCIortraceOnCI` is simpler.
Absolutely no need to make that change, just a thought

Yeah sounds like a good idea. I'll add a debt issue

DonJayamanne · 2021-09-17T00:09:16Z

src/test/datascience/notebook/intellisense/diagnostics.vscode.test.ts

+        assert.ok(diagnostics.find((item) => item.message.includes('system')));
+    });
+    test('Markdown cells dont get errors', async () => {
+        // Create two cells, one markdown, make sure not diagnostics in markdown


…ing)

…ifferent way

…tests

…ew to use waitForCondition instead of trying to track messages

rchiodo · 2021-09-21T23:49:48Z

@IanMatthewHuff and @DonJayamanne can you take a second look?

I reworked how we select a kernel in the tests. It now loops on the notebook.selectKernel function. But the default it picks is lot more complicated now. Actually found a bug with this change: #7610

I also reworked how variables are waited for in tests. Instead of guessing the messages to be sent, I switched it to just use waitForCondition which should work regardless of how many messages get passed around.

IanMatthewHuff · 2021-09-21T23:58:40Z

src/client/datascience/notebook/vscodeNotebookController.ts

-            await this.setAsActiveControllerForTests(notebook);
+
+        // Keep track of if this controller is preferred for this notebook
+        if (affinity === NotebookControllerAffinity.Preferred) {


updateNotebookAffinity is called on each document open if you do something like this:
Open foo.ipynb (A kernel in metadata) foo.ipynb added to A perferredWith
Change to kernel B in foo.ipynb
Execute
Close foo.ipynb
Open foo.ipynb (B kernel in metadata) foo.ipynb added to B preferredWith

Seems like that situation would be wrong then if I'm following right.

Not following the mistake? Because A would still be preferred? Yeah seems like map should be the other way. Map between notebook (key) and controller (value)

Yeah, trying to make sure that I'm reading it right. As I see it now PreferredWith is all the notebook documents that this controller is the preferred controller for. Given that the preferred controller could change if you execute and reopen a document you could have the same document in the preferred list for two different controllers. As documents only have one preferred (at least from our extensions perspective) that situation wouldn't be right.

IanMatthewHuff · 2021-09-22T00:01:58Z

src/test/datascience/variableView/variableView.vscode.test.ts

        // Parse the HTML for our expected variables
        const expectedVariables = [
            { name: 'test', type: 'str', length: '11', value: ' MYTESTVALUE' },
            { name: 'test2', type: 'str', length: '12', value: ' MYTESTVALUE2' }
        ];
-        verifyViewVariables(expectedVariables, htmlResult);
+        await waitForVariablesToMatch(expectedVariables, variableView);


Nice, much better.

…est was not finding it even though it was using it to run

DonJayamanne · 2021-09-22T20:12:15Z

.github/workflows/build-test.yml

@@ -43,6 +43,8 @@ env:
  IPYWIDGET_SCREENSHOT_PATH: '*-screenshot.png'
  DISABLE_INSIDERS_EXTENSION: 1 # Disable prompts to install Insiders in tests (else it blocks activation of extension).
  VSC_JUPYTER_INSTRUMENT_CODE_FOR_COVERAGE: true
+  VSIX_NAME_PYTHON: 'ms-python-insiders.vsix'
+  VSC_JUPTYER_PYTHON_EXTENSION_VERSION: 'stable'


Does this mean, if this is stable, then we use Stable VS Code & stable Python extension
& if its insider we use vscode insiders & python insiders?

No it's just a flag for now. It makes it easy to switch this later.

src/client/datascience/interactive-window/interactiveWindow.ts

src/client/datascience/notebook/vscodeNotebookController.ts

DonJayamanne · 2021-09-22T20:49:31Z

src/client/datascience/notebook/types.ts

@@ -13,6 +13,7 @@ export interface INotebookControllerManager {
    // Marked test only, just for tests to access registered controllers
    registeredNotebookControllers(): VSCodeNotebookController[];
    getInteractiveController(): Promise<VSCodeNotebookController | undefined>;
+    getPreferredNotebookController(document: NotebookDocument): VSCodeNotebookController | undefined;


I don't think this is being used.

It's used in the helper.ts file to find the closest match for a kernel.

Basically the the tests that used to call waitForKernelToGetAutoSelected are now instead waiting for the preferred kernel to be auto selected (which effectively is the same as what happened before)

Yes found that.

DonJayamanne · 2021-09-22T20:52:23Z

src/test/datascience/notebook/helper.ts

+                if (!isRightKernel()) {
+                    traceInfoIf(isCI, `Notebook select.kernel command switching to kernel id ${id}: Try ${tryCount}`);
+                    // Send a select kernel on the active notebook editor. Keep sending it if it fails.
+                    await commands.executeCommand('notebook.selectKernel', { id, extension: JVSC_EXTENSION_ID });


I dont think this needs to be ezecuted multiple times, i think once is sufficient, and after that we just need to keep waiting until the right kernel is selected.
Else we keep firing this command multkiple times & i'm concerned we could have other side effects of filing this unnecessarily

That's not what I found. There was a potential race between 'createNotebookController (and results getting to VS code proper)' and the 'notebook.selectKernel' command coming in. This race went away when I did this in a loop.

Previously we didn't even care if it worked (hence the comment in the setAsActiveControllerForTests). And we'd set and forget it. But sometimes it didn't work.

With this loop it seems to always work.

DonJayamanne

Looks good

rchiodo added 3 commits September 16, 2021 11:40

Tests working independently

e2f2fed

Use dev build of python extension

da73812

Fix build error

93b9b06

rchiodo requested a review from a team as a code owner September 16, 2021 19:33

Do it without azure login

8709e6b

IanMatthewHuff approved these changes Sep 16, 2021

View reviewed changes

rchiodo added 9 commits September 16, 2021 14:31

Add some more logging

029a180

Add more logging for fetching completions

add4ede

Add more logging and wait some in the test

9b49dab

Update logging

75ef2bf

Fix linter

8a2b7fd

Dont use old jedi

9e4b56a

See if csv is actually loading

c9801c2

Print out current folder during test

b8be50b

Cell output not in string form

fa76afe

DonJayamanne reviewed Sep 17, 2021

View reviewed changes

DonJayamanne approved these changes Sep 17, 2021

View reviewed changes

rchiodo added 11 commits September 17, 2021 08:49

Fix read csv code to work with default notebookFileRoot

7a20d2a

Add screenshot and some more logging (looks like execution isn't work…

87cf668

…ing)

Actually capture a screenshot

d549f32

Skip another test and make sure selected controller is accurate

997320f

Fix compile errors

4123cf1

Add more logging and try turning off python extension experiments a d…

9d6f251

…ifferent way

Add verbose logging to VS code

a63969d

Add VS code logs

852a557

Change the way waitForKernelAutoSelect works. Have it loop.

4657383

Update timeout for affinity. Can't wait too long

1fc9570

Fix non python cases

a329aea

rchiodo added 5 commits September 21, 2021 13:43

Refactor how selection changes occur

787ac1f

Fix some test problems and more logging

13149c8

Two more fixes

5600040

Merge remote-tracking branch 'origin/main' into rchiodo/intellisense_…

c54ea19

…tests

Disable some more tests that actually found a bug. Update variable vi…

6bc65f8

…ew to use waitForCondition instead of trying to track messages

rchiodo requested review from IanMatthewHuff and DonJayamanne September 21, 2021 23:48

IanMatthewHuff reviewed Sep 21, 2021

View reviewed changes

IanMatthewHuff reviewed Sep 22, 2021

View reviewed changes

rchiodo added 4 commits September 21, 2021 17:19

Rework how preferred is cached

b830e93

Switch back to stable python

2166345

Fix linter

23f5559

Comment out intellisense tests for now

a93bf3a

IanMatthewHuff self-requested a review September 22, 2021 18:18

rchiodo added 2 commits September 22, 2021 11:21

Rework how notebook controller is found for the interactive window. T…

19ecf09

…est was not finding it even though it was using it to run

disable some more tests and try to see why interactive test is failing

9755d6c

DonJayamanne reviewed Sep 22, 2021

View reviewed changes

rchiodo added 2 commits September 22, 2021 13:38

Add screenshot to test failure

0183f64

Dont check env name, may not be there

e3ef4a7

DonJayamanne reviewed Sep 22, 2021

View reviewed changes

src/client/datascience/interactive-window/interactiveWindow.ts Show resolved Hide resolved

DonJayamanne reviewed Sep 22, 2021

View reviewed changes

src/client/datascience/notebook/vscodeNotebookController.ts Show resolved Hide resolved

DonJayamanne reviewed Sep 22, 2021

View reviewed changes

DonJayamanne approved these changes Sep 22, 2021

View reviewed changes

IanMatthewHuff approved these changes Sep 22, 2021

View reviewed changes

rchiodo merged commit b7241b5 into main Sep 22, 2021

rchiodo deleted the rchiodo/intellisense_tests branch September 22, 2021 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Submit tests for intellisense (replacing functional tests we had) #7565

Submit tests for intellisense (replacing functional tests we had) #7565

rchiodo commented Sep 16, 2021

codecov-commenter commented Sep 16, 2021 •

edited

Loading

DonJayamanne Sep 17, 2021

rchiodo Sep 17, 2021

DonJayamanne Sep 17, 2021

rchiodo commented Sep 21, 2021

IanMatthewHuff Sep 21, 2021

rchiodo Sep 22, 2021

IanMatthewHuff Sep 22, 2021

IanMatthewHuff Sep 22, 2021

DonJayamanne Sep 22, 2021

rchiodo Sep 22, 2021

DonJayamanne Sep 22, 2021

rchiodo Sep 22, 2021

rchiodo Sep 22, 2021

DonJayamanne Sep 22, 2021

DonJayamanne Sep 22, 2021

rchiodo Sep 22, 2021

rchiodo Sep 22, 2021

DonJayamanne left a comment

Submit tests for intellisense (replacing functional tests we had) #7565

Submit tests for intellisense (replacing functional tests we had) #7565

Conversation

rchiodo commented Sep 16, 2021

codecov-commenter commented Sep 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchiodo commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 16, 2021 •

edited

Loading