Skip to content

Conversation

@rchiodo
Copy link

@rchiodo rchiodo commented Sep 20, 2019

For #7367

First couple of tests for native editor. Did a bunch of refactoring to get shared code between interactive window and the native editor.

@codecov-io
Copy link

codecov-io commented Sep 20, 2019

Codecov Report

Merging #7513 into master will increase coverage by 0.07%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7513      +/-   ##
==========================================
+ Coverage    58.7%   58.78%   +0.07%     
==========================================
  Files         494      493       -1     
  Lines       21838    21810      -28     
  Branches     3503     3496       -7     
==========================================
  Hits        12821    12821              
+ Misses       8216     8188      -28     
  Partials      801      801
Impacted Files Coverage Δ
...ce/interactive-window/interactiveWindowProvider.ts 17.1% <0%> (-0.23%) ⬇️
src/client/datascience/webViewHost.ts 10.47% <0%> (+0.19%) ⬆️
.../datascience/interactive-common/interactiveBase.ts 6.25% <0%> (-0.05%) ⬇️
src/client/datascience/jupyter/jupyterNotebook.ts 5.93% <0%> (-0.09%) ⬇️
src/datascience-ui/interactive-common/mainState.ts

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 6a57280...938e646. Read the comment docs.

@@ -0,0 +1,329 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
'use strict';
Copy link
Author

Choose a reason for hiding this comment

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

strict [](start = 5, length = 6)

This file was originally the interactiveWindowTestHelpers.tsx, but I modified it to be more generic so we could reuse most of it.

// ansiToHtml is different between the tests running and webpack. figure out which one
// tslint:disable-next-line: no-any
if (ansiToHtml instanceof Function) {
CellOutput.ansiToHtmlClass_ctor = ansiToHtml;
Copy link
Member

Choose a reason for hiding this comment

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

Is this the commonjs vs ES6 default import issue?

Copy link
Author

Choose a reason for hiding this comment

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

Yep. Had the same problem with memoize in the variable explorer.
Tests use one format and webpack uses the other.


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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I ran into this with node-fetch as well. I think this solution seems fine to me. But I did see one site that I was looking into with node-fetch that was mentioning that webpack could be configured to pick main config over module config. Not sure if that is really worth looking into, but it might make this code unneeded. node-fetch/node-fetch#450


In reply to: 326733133 [](ancestors = 326733133,326731645)

// Export should cause exportCalled to change to true
const exportButton = findButton(wrapper, NativeEditor, 5);
await waitForMessageResponse(() => exportButton!.simulate('click'));
assert.equal(exportCalled, true, 'Export should have been called');
Copy link
Member

Choose a reason for hiding this comment

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

Can't you just use a Times on the setup function and then do a verify on that? Seems less fragile than tracking an extra bool.

Copy link
Author

Choose a reason for hiding this comment

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

That would probably be better. I just copied this from what I had before for the interactive window.


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

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

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

:shipit:

@rchiodo rchiodo merged commit 9dec031 into master Sep 20, 2019
@rchiodo rchiodo deleted the rchiodo/native_functional branch September 20, 2019 18:07
@lock lock bot locked as resolved and limited conversation to collaborators Sep 27, 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.

5 participants