Fix issues with LiveShare and triple quotes by rchiodo · Pull Request #5084 · microsoft/vscode-python

rchiodo · 2019-04-02T21:08:26Z

For #5012 and bugs found while testing the fix for 5012 around liveshare

Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
Title summarizes what is changing
Has a news entry file (remember to thank yourself!)
Has sufficient logging.
Has telemetry for enhancements.
Unit tests & system/integration tests are added/updated
Test plan is updated as appropriate
package-lock.json has been regenerated by running npm install (if dependencies have changed)
The wiki is updated with any design decisions/details.

Add test for tabs to make sure they stay in comments

Needed to save a copy of a cell before sending it. Otherwise subsequent messages overwrite the cell in use.

rchiodo · 2019-04-02T21:09:29Z

  MOCHA_FILE: '$(Build.ArtifactStagingDirectory)/test-junit.xml' # All test files will write their JUnit xml output to this file, clobbering the last time it was written.
  MOCHA_REPORTER_JUNIT: true # Use the mocha-multi-reporters and send output to both console (spec) and JUnit (mocha-junit-reporter).
-  # VSC_PYTHON_FORCE_LOGGING: true - Enable this to turn on console output for the logger
+  VSC_PYTHON_FORCE_LOGGING: true # Enable this to turn on console output for the logger


VSC_PYTHON_FORCE_LOGGING [](start = 2, length = 24)

This is from the Monday meeting. I said I'd enable this on my next submission. #Resolved

rchiodo · 2019-04-02T21:10:50Z

+        if (this.requestLog.has(id)) {
+            // This must be a local call that occurred after a guest call. Just
+            // use the local responses to return the results.
+            return this.localResponses.waitForObservable(code, id);


localResponses [](start = 24, length = 14)

The root change for liveshare was to separate the list of cached responses into two groups - local and guest. Local is used when the guest makes a request before the host can. This happens on export with output and sometimes when clicking/submitting through the input box. This is the root cause of the bug we saw during our demo. #Resolved

rchiodo · 2019-04-02T21:12:12Z

                return (r.pos === pos) &&
-                    (foundId === r.id || !foundId) &&
-                    (code === r.code) &&
-                    (!r.cells || (r.cells && r.cells[0].file === file && r.cells[0].line === line));


Realized we have an id for all requests now so checking file is unnecessary. It was the part of the original bug I found where if the guest submitted first, the file name wouldn't match. #Resolved

rchiodo · 2019-04-02T21:12:49Z

        // Look through all of our responses that are queued up and see if they make a
        // waiting promise resolve
-        for (let i = 0; i < this.responseQueue.length; i += 1) {
-            const response = this.responseQueue[i];


responseQueue [](start = 34, length = 13)

Also realized rechecking here through all responses is redundant as they've already been checked before. Only need to check the new response. #Resolved

rchiodo · 2019-04-02T21:13:12Z

                subscriber.error(this.sessionStartTime, new Error(localize.DataScience.jupyterServerCrashed().format(exitCode.toString())));
                subscriber.complete(this.sessionStartTime);
            } else {
-                const request = this.generateRequest(concatMultilineString(stripComments(subscriber.cell.data.source)), silent);


stripComments [](start = 75, length = 13)

This is the bug fix for the 5102 bug. #Resolved

rchiodo · 2019-04-02T21:14:06Z

        return callback;
    }

+    private getApi() : Promise<vsls.LiveShare | null> {


Refactored this so that the promise doesn't start in the ctor. It was easier to debug when stuff was happening if the promise wasn't starting right away. #Resolved

rchiodo · 2019-04-02T21:15:38Z

-    public send(service: vsls.SharedService) {
-        this.responseQueue.forEach(r => service.notify(LiveShareCommands.serverResponse, r));
+    public send(service: vsls.SharedService, translator: (r: IServerResponse) => IServerResponse) {
+        this.responseQueue.forEach(r => service.notify(LiveShareCommands.serverResponse, translator(r)));


Host to guest file remapping now happens at send time instead of response push time. That's because of the 'local' copy the host requires. #Resolved

IanMatthewHuff · 2019-04-02T21:16:17Z

    }

    private onStatusChanged(_s: Session.ISession, a: Kernel.Status) {
-        traceInfo(`Status changed to ${a} from OnStatusChangedEvent`);


too noisy I assume? #ByDesign

Yeah. Not necessary too as the 'wait for idle' status is pretty much the same thing.

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

IanMatthewHuff · 2019-04-02T21:25:54Z

    }

    private onServerResponse = (args: Object) => {
+        const er = args as IExecuteObservableResponse;


Can't hostJupyterServer pass something that is just an IServerResponse and not an IExecuteObservableResponse? #WontFix

Yep. Exceptions also come through. The trace will just show undefined then.

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

er.Pos and er.Id will be undefined.

In reply to: 271502440 [](ancestors = 271502440,271502241)

Do you think I should skip logging?

In reply to: 271502541 [](ancestors = 271502541,271502440,271502241)

Got it. Just wasn't sure if you wanted undefined printed out. Seems ok.

In reply to: 271502541 [](ancestors = 271502541,271502440,271502241)

If er is not a ExecuteObservableResponse

In reply to: 271502746 [](ancestors = 271502746,271502541,271502440,271502241)

It's probably ok. My brains just says null exception when I see this. Your choice,

In reply to: 271502858 [](ancestors = 271502858,271502746,271502541,271502440,271502241)

IanMatthewHuff

rchiodo added 8 commits March 29, 2019 16:57

Fix multiline string being used as an actual string

56b43ac

Add test for tabs to make sure they stay in comments

Handle case where comment delimiter doesn't match original

2e86096

Merge remote-tracking branch 'origin/master' into rchiodo/triplequoted

f57b079

Merge remote-tracking branch 'origin/master' into rchiodo/triplequoted

2e5ae2a

Fixing up some live share bugs

864eec9

Get live share working again

360c7d9

Get liveshare functional tests to run again

95f0626

Needed to save a copy of a cell before sending it. Otherwise subsequent messages overwrite the cell in use.

Add news entry

530dd95

rchiodo self-assigned this Apr 2, 2019

rchiodo requested a review from IanMatthewHuff April 2, 2019 21:08

rchiodo commented Apr 2, 2019

View reviewed changes

IanMatthewHuff reviewed Apr 2, 2019

View reviewed changes

IanMatthewHuff approved these changes Apr 2, 2019

View reviewed changes

rchiodo merged commit 1575c12 into master Apr 2, 2019

rchiodo deleted the rchiodo/triplequoted branch April 2, 2019 21:31

lock Bot locked as resolved and limited conversation to collaborators Jul 30, 2019

Conversation

rchiodo commented Apr 2, 2019

Uh oh!

rchiodo Apr 2, 2019 • 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!

rchiodo Apr 2, 2019 • 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!

rchiodo Apr 2, 2019 • 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!

rchiodo Apr 2, 2019 • 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!

rchiodo Apr 2, 2019 • 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!

rchiodo Apr 2, 2019 • 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!

rchiodo Apr 2, 2019 • 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!

IanMatthewHuff Apr 2, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Apr 2, 2019 • edited by rchiodo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rchiodo Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

rchiodo Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

rchiodo Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

rchiodo Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

IanMatthewHuff left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rchiodo Apr 2, 2019 •

edited

Loading

rchiodo Apr 2, 2019 •

edited

Loading

rchiodo Apr 2, 2019 •

edited

Loading

rchiodo Apr 2, 2019 •

edited

Loading

rchiodo Apr 2, 2019 •

edited

Loading

rchiodo Apr 2, 2019 •

edited

Loading

rchiodo Apr 2, 2019 •

edited

Loading

IanMatthewHuff Apr 2, 2019 •

edited by rchiodo

Loading

IanMatthewHuff Apr 2, 2019 •

edited by rchiodo

Loading