Skip to content

Treat Native notebook tests as VS Code tests #14282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 6, 2020

Conversation

DonJayamanne
Copy link

@DonJayamanne DonJayamanne commented Oct 6, 2020

Basically rename ds.test.ts to native.vscode.test.ts
Two of the native notebook tests are failing (fixed one & disabled the other).

Make it easier to migrate ds.test.ts to .vscode.test.ts.
Both are tests running in VSCode, hecne have a common way to do it.

@DonJayamanne DonJayamanne added the no-changelog No news entry required label Oct 6, 2020
@DonJayamanne DonJayamanne changed the title Native notebook tests (rename ds.test.ts to native.vscode.test.ts) Treat Native notebook tests as VS Code tests Oct 6, 2020
@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2020

Codecov Report

Merging #14282 into main will increase coverage by 0.13%.
The diff coverage is 64.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #14282      +/-   ##
==========================================
+ Coverage   59.75%   59.88%   +0.13%     
==========================================
  Files         697      709      +12     
  Lines       38649    39334     +685     
  Branches     5577     5698     +121     
==========================================
+ Hits        23094    23555     +461     
- Misses      14364    14539     +175     
- Partials     1191     1240      +49     
Impacted Files Coverage Δ
src/client/activation/common/activatorBase.ts 14.41% <0.00%> (+1.19%) ⬆️
...rc/client/activation/jedi/multiplexingActivator.ts 17.74% <0.00%> (ø)
src/client/activation/node/languageServerProxy.ts 26.58% <0.00%> (ø)
src/client/activation/refCountedLanguageServer.ts 41.30% <0.00%> (ø)
src/client/activation/types.ts 100.00% <ø> (ø)
src/client/common/process/pythonDaemon.ts 14.28% <0.00%> (ø)
src/client/common/utils/localize.ts 96.24% <ø> (ø)
src/client/datascience/baseJupyterSession.ts 57.33% <ø> (+0.52%) ⬆️
.../datascience/interactive-common/interactiveBase.ts 5.78% <0.00%> (+<0.01%) ⬆️
...ient/datascience/interactive-ipynb/nativeEditor.ts 7.78% <0.00%> (ø)
... and 79 more

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 7049770...fbea55c. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Oct 6, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@7049770). Click here to learn what that means.
The diff coverage is 64.19%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #14282   +/-   ##
=======================================
  Coverage        ?   59.89%           
=======================================
  Files           ?      709           
  Lines           ?    39334           
  Branches        ?     5698           
=======================================
  Hits            ?    23558           
  Misses          ?    14535           
  Partials        ?     1241           
Impacted Files Coverage Δ
src/client/activation/common/activatorBase.ts 14.41% <0.00%> (ø)
...rc/client/activation/jedi/multiplexingActivator.ts 17.74% <0.00%> (ø)
src/client/activation/node/languageServerProxy.ts 26.58% <0.00%> (ø)
src/client/activation/refCountedLanguageServer.ts 41.30% <0.00%> (ø)
src/client/activation/types.ts 100.00% <ø> (ø)
src/client/common/process/pythonDaemon.ts 14.28% <0.00%> (ø)
src/client/common/utils/localize.ts 96.24% <ø> (ø)
src/client/datascience/baseJupyterSession.ts 57.33% <ø> (ø)
.../datascience/interactive-common/interactiveBase.ts 5.78% <0.00%> (ø)
...ient/datascience/interactive-ipynb/nativeEditor.ts 7.78% <0.00%> (ø)
... and 57 more

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 7049770...2e7aeec. Read the comment docs.

@@ -156,7 +160,7 @@ export class CellExecution {
if (!this.started) {
await this.dequeue();
}
this._result.resolve(this.cell.metadata.runState);
await this.completedDurToCancellation();
Copy link
Author

Choose a reason for hiding this comment

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

Due to changes in API, we need to ensure cell status is updated.

@@ -72,7 +73,7 @@ suite('DataScience Install IPyKernel (slow) (install)', () => {
const installed = createDeferred();

// Confirm it is installed.
sinon.stub(installer, 'install').callsFake(async function (product: Product) {
const showInformationMessage = sinon.stub(installer, 'install').callsFake(async function (product: Product) {
Copy link
Author

Choose a reason for hiding this comment

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

We need to revert the stubs in each test.

const appShell = api.serviceContainer.get<IApplicationShell>(IApplicationShell);
const showInformationMessage = sinon
.stub(appShell, 'showInformationMessage')
.callsFake(function (message: string) {
Copy link
Author

Choose a reason for hiding this comment

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

Better & easier test, than updating the settings

Copy link
Author

Choose a reason for hiding this comment

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

Also can be used to confirm prompt is displayed.

@@ -37,6 +37,7 @@ suite('DataScience - VSCode Notebook - (Trust)', function () {
let testIPynb: Uri;
const disposables: IDisposable[] = [];
suiteSetup(async function () {
return this.skip();
Copy link
Author

Choose a reason for hiding this comment

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

THis is the only other native notebook test that is failing.
Trusted notebooks don't work, hence disabled @IanMatthewHuff /cc

@@ -115,10 +103,23 @@ suite('DataScience - VSCode Notebook - Restart/Interrupt/Cancel/Errors (slow)',
assertVSCCellHasErrors(cell);
}
});
test('Restarting kernel will cancel cell execution & we can re-run a cell', async () => {
test('Restarting kernel will cancel cell execution & we can re-run a cellxxx', async () => {

Choose a reason for hiding this comment

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

is the celxxx meant to be there?

Copy link
Author

Choose a reason for hiding this comment

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

damn

@DonJayamanne DonJayamanne merged commit c248197 into microsoft:main Oct 6, 2020
@DonJayamanne DonJayamanne deleted the nativeNotebookTests branch October 6, 2020 23:51
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 6, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

DonJayamanne added a commit that referenced this pull request Oct 30, 2020
* Remove cell index property and use build in prop (#14239)
* Update other cells in cell execution (#14240)
* Tests for prompting to install missing ipykernel (#14266)
* Treat Native notebook tests as VS Code tests (#14282)
* Fixes to blowing away of kernel info & not using right startup info (… …
* Default cell language for native notebooks (#14314)
* Ignore formatting in ipynb when dealing with trust (#14333)
* Fixes to trust service (#14352)
* Change `IPython kernel` to `Jupyter kernel` (#14375)
* Trust for native notebooks (#14353)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants