-
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
Try running tests with space in root path #14113
Conversation
Codecov Report
@@ Coverage Diff @@
## main #14113 +/- ##
==========================================
- Coverage 59.96% 59.85% -0.12%
==========================================
Files 703 704 +1
Lines 38936 39015 +79
Branches 5626 5644 +18
==========================================
+ Hits 23349 23353 +4
- Misses 14380 14439 +59
- Partials 1207 1223 +16
Continue to review full report at Codecov.
|
@rchiodo you can temporarily edit the YAML file to trigger on PRs to see it run. You could also try temporarily removing the guards against the specific branch and repo to also get a run in this PR. |
.github/workflows/insiders.yml
Outdated
@@ -139,6 +139,8 @@ jobs: | |||
steps: | |||
- name: Checkout | |||
uses: actions/checkout@v2 | |||
with: | |||
path: 'source path' |
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.
path: 'source path' | |
path: 'path with a space in it' |
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.
Would some Unicode be worth it?
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.
Yeah good idea. I'll add a unicode character and a space
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 we need to open issues for the 2.7 test failures when using Unicode characters?
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.
Yeah I'd say so. Unless we don't care about supporting 2.7 with people with unicode characters in their paths?
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, we aren't quite ready to drop 2.7 support, so if there's a Unicode issue there then we should fix them.
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.
Sounds good, I'll take care of opening Unicode issues.
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.
Meta issue for all Unicode-related failures in for the test job of the Insiders workflow: #14292
.github/workflows/insiders.yml
Outdated
@@ -4,6 +4,7 @@ on: | |||
push: | |||
branches: | |||
- main | |||
- rchiodo/test_with_space |
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 temporary. The rest of this should be forcing a path with spaces in it though.
special-working-directory: './path with spaces' | ||
special-working-directory-relative: 'path with spaces' |
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.
What is the benefit of having a separate './path with spaces'
env variable, instead of using 'path with spaces'
everywhere?
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.
So I don't mess up with the name. Just a variable replacement.
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.
special-working-directory: './path with spaces' | |
special-working-directory-relative: 'path with spaces' | |
special-working-directory: './path with 🌌s' | |
special-working-directory-relative: 'path with 🌌s' |
If we want to do some Unicode path and spaces, this might help?
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.
Why not use env.special-working-directory-relative
everywhere, instead of having env.special-working-directory
and env.special-working-directory-relative
?
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.
Cause they're different? I wasn't sure how to concat a string in the yaml file so I just made to variables.
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.
Adding something with Unicode like Karthik suggested is probably a good idea.
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.
Looks good to me!
Good to go once the housekeeping items get dealt with (temp branch condition at the top, update this with latest and greatest main, optional news item) 🧹
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.
Actually let me just pre-approve it and you can do the cleanup and merge afterwards.
@kimadeline I thought Brett wanted to wait to submit this stuff when you had insider's completely moved over to actions? Is that already done? |
I believe that was done in #14214 |
@rchiodo Kim is right, we moved over to GH Actions for the insiders build last week. |
And are you guys both okay with me submitting even if all the tests don't pass on this next run? |
@rchiodo yeah, we will prioritize appropriately between our teams to get them fixed (we don't have anything pressing that has to go out to insiders users right now). |
Codecov Report
@@ Coverage Diff @@
## main #14113 +/- ##
=======================================
Coverage ? 59.91%
=======================================
Files ? 709
Lines ? 39334
Branches ? 5698
=======================================
Hits ? 23566
Misses ? 14530
Partials ? 1238
Continue to review full report at Codecov.
|
Kudos, SonarCloud Quality Gate passed!
|
* 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
Not sure if this will work or not. @karthiknadig do you know if I can run a PR against the Github actions to try this out?
Note: This is to make sure this bug doesn't occur again:
https://github.com/microsoft/vscode-python/issues/14083