Skip to content

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Oct 7, 2019

For #7372

Provide some error checking around server not running.
Also fixed the code so that if an error occurs during startup, we show the error in the output of the first cell.

image

@rchiodo rchiodo self-assigned this Oct 7, 2019
@codecov-io
Copy link

codecov-io commented Oct 7, 2019

Codecov Report

Merging #7804 into master will increase coverage by <.01%.
The diff coverage is 26.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7804      +/-   ##
==========================================
+ Coverage   59.15%   59.15%   +<.01%     
==========================================
  Files         498      498              
  Lines       22254    22256       +2     
  Branches     3578     3579       +1     
==========================================
+ Hits        13164    13166       +2     
  Misses       8266     8266              
  Partials      824      824
Impacted Files Coverage Δ
...ient/datascience/editor-integration/codewatcher.ts 57.21% <0%> (ø) ⬆️
.../datascience/interactive-common/interactiveBase.ts 6.22% <0%> (-0.03%) ⬇️
...ractive-window/interactiveWindowCommandListener.ts 60.3% <0%> (ø) ⬆️
...rc/client/datascience/errorHandler/errorHandler.ts 67.64% <35.71%> (ø) ⬆️
src/client/datascience/jupyter/jupyterExecution.ts 52.53% <70%> (ø) ⬆️
src/client/common/utils/platform.ts 88.23% <0%> (+11.76%) ⬆️

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 987b4de...27674c9. Read the comment docs.

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Adding some comments and possibly displaying errors in native editor before handling error might help.

await this.errorHandler.handleError(exc);

// Make this error our cell output
this.sendCellsToWebView([

Choose a reason for hiding this comment

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

Why not send the error message to the view then display the error message (e.g. errorHandler.handleError could end up displaying another UI, and its good to have the errors already visible in the native editor at that point.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea.


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

}

public resetLoad() {
this.waitingForLoadRender = false;

Choose a reason for hiding this comment

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

What does this method do?

Copy link
Author

Choose a reason for hiding this comment

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

Rests the state of loading so that the tests can reload the same control over again. I'll add a comment. reset sounds like a better name


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

export function closeNotebook(editor: INotebookEditor, wrapper: ReactWrapper<any, Readonly<{}>, React.Component>): Promise<void> {
const reactEditor = getMainPanel<NativeEditor>(wrapper, NativeEditor);
if (reactEditor) {
reactEditor.stateController.resetLoad();

Choose a reason for hiding this comment

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

If we're closing an editor, then why call it resetLoad, why not reset or dispose?

Copy link
Author

Choose a reason for hiding this comment

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

dispose isn't appropriate as the react control is still active. But reset works.


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

@rchiodo rchiodo closed this Oct 7, 2019
@rchiodo rchiodo reopened this Oct 7, 2019
@rchiodo rchiodo merged commit 5f9bf45 into master Oct 8, 2019
@rchiodo rchiodo deleted the rchiodo/startup_tests branch October 8, 2019 15:59
rchiodo added a commit that referenced this pull request Oct 10, 2019
* New tests

* Add some more functional tests for the notebook editor
Make sure when an error occurs on running a cell it's displayed to the user.

* Review feedback
@lock lock bot locked as resolved and limited conversation to collaborators Oct 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants