Re-enable self cert tests for notebooks #11197
Conversation
| @@ -149,7 +150,7 @@ export class NotebookStarter implements Disposable { | |||
| if (exitCode !== 0) { | |||
| throw new Error(localize.DataScience.jupyterServerCrashed().format(exitCode?.toString())); | |||
| } else { | |||
| throw new Error(localize.DataScience.jupyterNotebookFailure().format(err)); | |||
| throw new WrappedError(localize.DataScience.jupyterNotebookFailure().format(err), err); | |||
There was a problem hiding this comment.
Found that the error message didn't contain the stack trace.
Added a new error to ensure the stack trace is persisted in the new error, much like in .NET.
| @@ -400,7 +388,7 @@ suite('DataScience notebook tests', () => { | |||
| '-m', | |||
| 'jupyter', | |||
| 'notebook', | |||
| `--config=${configFile}`, | |||
| '--NotebookApp.open_browser=False', | |||
There was a problem hiding this comment.
@IanMatthewHuff I removed the config file as all it was doing was setting two flags.
Removing one of them fixed it for mac.
However I feel we might need c.NotebookApp.ip = '0.0.0.0' for non mac scenarios. Do you remember why you added it?
I can add it back here (conditionally based on the OS).
There was a problem hiding this comment.
Look just a bit down that thread. Glad that I put that info in the old PR or I would have forgotten.
There was a problem hiding this comment.
No config file does more than that. It overrides the user's default config file. We had all sorts of problems with config files being wrong for us to run.
There was a problem hiding this comment.
Oh wait this was for the test only? Never mind. I should have looked at the file.
There was a problem hiding this comment.
Yes the config is only used in tests (used here), hence safe to delete.
DonJayamanne
requested review from
DavidKutu,
greazer,
IanMatthewHuff and
rchiodo
| @@ -400,7 +388,7 @@ suite('DataScience notebook tests', () => { | |||
| '-m', | |||
| 'jupyter', | |||
| 'notebook', | |||
| `--config=${configFile}`, | |||
| '--NotebookApp.open_browser=False', | |||
| this.usablePythonInterpreter = undefined; | ||
| } | ||
| } | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. |
There was a problem hiding this comment.
This is showing as entirely different for me?
There was a problem hiding this comment.
How about ignoring white space.
DonJayamanne
force-pushed
the
fixCertTests
branch
from
1080566 to
2627f54
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
yay tests are passing, was difficult to tell cuz there are other failures and its a 44k line long log file. |
For #10447
Will run flaky test before I merge this.