-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix random failures on functional tests #14331
Conversation
.vscode/settings.json
Outdated
@@ -45,7 +45,7 @@ | |||
"source.fixAll.eslint": true, | |||
"source.fixAll.tslint": true | |||
}, | |||
"python.languageServer": "Microsoft", | |||
"python.languageServer": "Pylance", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to change this in the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh probably not. I'll put it back
Codecov Report
@@ Coverage Diff @@
## main #14331 +/- ##
==========================================
+ Coverage 59.75% 59.88% +0.13%
==========================================
Files 697 709 +12
Lines 38649 39339 +690
Branches 5577 5700 +123
==========================================
+ Hits 23094 23559 +465
- Misses 14364 14539 +175
- Partials 1191 1241 +50
Continue to review full report at Codecov.
|
@@ -49,18 +96,28 @@ export class KernelLauncher implements IKernelLauncher { | |||
return kernelProcess; | |||
} | |||
|
|||
private async getKernelConnection(): Promise<IKernelConnection> { | |||
private async getPorts(): Promise<number[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really minor, but maybe not the exact same name as the portfinder function that we are using? Or change the other one to portfinderGetPorts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
@@ -177,14 +177,20 @@ export abstract class BasePythonDaemon { | |||
return Object.keys(options).every((item) => daemonSupportedSpawnOptions.indexOf(item as any) >= 0); | |||
} | |||
protected sendRequestWithoutArgs<R, E, RO>(type: RequestType0<R, E, RO>): Thenable<R> { | |||
return Promise.race([this.connection.sendRequest(type), this.connectionClosedDeferred.promise]); | |||
if (this.proc && this.proc.exitCode === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (this.proc && this.proc.exitCode === null) { | |
if (this.proc && typeof this.proc.exitCode !== 'number') { |
Else if we have undefined
, then above condition doesn't work. I feel that's better than hardcoding null
.
Optional change requets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The api says it returns a number or null. Never undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just that usage of null
is frowned upon & comparing against null
isn't a good practice either.
I guess its old code that they can't remove..
public static async cleanupStartPort() { | ||
try { | ||
// Destroy the file | ||
const port = await KernelLauncher.startPortPromise; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I like this.
I thought we decided we'd only use such code for the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, all good, I can see it is
from pathlib import Path | ||
import re | ||
|
||
parser = argparse.ArgumentParser(description="Parse a test log into its parts") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logParser? Or the args?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is used (I mentioned it in the PR description) to split a test output file into synchronized parts so you can debug it.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
* Splitting test log * Fix problem with kernels ports being reused * Make kernel launcher port round robin only for testing * Make formatters change only apply during testing * Add news entry * Apply black formatting * Code review feedback and skip flakey remote password test * Another flakey test * More CR feedback * Missed a spot
* Fix two problems with escaping (#14228) * Remove unneeded cell keys when exporting (#14241) * Remove transient output when exporting from the interactive window * Add news entry * Test was failing with true jupyter (#14261) * Potential fix for ipywidget flakiness (#14281) * Try running tests with space in root path (#14113) * Add test with a space (only works on flake) * Push to insiders.yml only * Remove test that doesn't really do anything * REmove unused bits * Change path to have unicode too * Get test to run * Set root path differently * Valid dir * A different way * Another way * Try creating the directory first * Another try * Only one env * Pass parameters correctly * Try without unicode * Set working directory directly on xvfb actions * Working-directory not workingDirectory * Cached ts files output * Remove test with space branch for insiders * Update vscode-python-pr-validation.yaml (#14285) REmove missing branch? Might make it work again * Get rid of AZDO yamls. Not used anymore * Dont run on push (#14307) * Fix random failures on functional tests (#14331) * Splitting test log * Fix problem with kernels ports being reused * Make kernel launcher port round robin only for testing * Make formatters change only apply during testing * Add news entry * Apply black formatting * Code review feedback and skip flakey remote password test * Another flakey test * More CR feedback * Missed a spot * More of the functional tests are failing (#14346) * Splitting test log * Fix problem with kernels ports being reused * Make kernel launcher port round robin only for testing * Make formatters change only apply during testing * Add news entry * Apply black formatting * Code review feedback and skip flakey remote password test * Another flakey test * More CR feedback * Missed a spot * Some more log parser changes and try to get interrupt to be less flakey * Fix interrupt killing kernel and add more logging for export * More logging * See if updating fixes the problem * Dont delete temp files * Upload webview output to figure out trust failure * Add name to step * Try another way to upload * Upload doesn't seem to work * Try a different way to upload * Try without webview logging as this makes the test pass * Try fixing test another way. Any logging is making the test pass * Compile error * Add more logging to figure out why raw kernel did not start (#14374) * Some more logging * Some more logging * Move PR changes into pr.yml * Fix multiprocessing problems with setting __file__ (#14376) * Fix multiprocessing problems with setting __file__ * Update news entry * Problem with wait for idle not propagating outwards * Fix unnecessary ask for python extension install * Don't error on warning for kernel install
For https://github.com/microsoft/vscode-python/issues/14290
Root cause was the port used by the kernels were overlapping.
Fixed this by using a file to keep track of ports in use (poor man's named mutex). File is cleaned up on exit and not disposed as dispose would be called in between each test.
Also added a python file to parse test log files. It has the following parameters:
--testoutput - parses the log looking for test lines (they all have ansi coloring) and prints them
--split - splits the testoutput file into multiple files based on process ids logged so you can debug test failures that are in parallel
package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed).