Skip to content
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

Merged
merged 20 commits into from
Oct 6, 2020
Merged

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Sep 25, 2020

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

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2020

Codecov Report

Merging #14113 into main will decrease coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
...ient/pythonEnvironments/base/info/pythonVersion.ts 66.66% <0.00%> (-4.77%) ⬇️
src/client/common/utils/platform.ts 68.00% <0.00%> (-4.00%) ⬇️
src/client/datascience/notebook/helpers/helpers.ts 36.99% <0.00%> (-3.92%) ⬇️
...nt/datascience/notebookStorage/vscNotebookModel.ts 57.89% <0.00%> (-3.22%) ⬇️
src/client/pythonEnvironments/info/index.ts 40.29% <0.00%> (-1.24%) ⬇️
src/client/common/utils/decorators.ts 79.41% <0.00%> (-0.99%) ⬇️
...t/common/insidersBuild/insidersExtensionService.ts 96.77% <0.00%> (-0.41%) ⬇️
src/client/logging/levels.ts 61.11% <0.00%> (ø)
src/client/logging/logger.ts 62.74% <0.00%> (ø)
src/client/logging/formatters.ts 53.57% <0.00%> (ø)
... and 7 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 1a0c790...7fd5715. Read the comment docs.

@brettcannon
Copy link
Member

@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.

@brettcannon brettcannon removed their request for review September 28, 2020 20:56
@@ -139,6 +139,8 @@ jobs:
steps:
- name: Checkout
uses: actions/checkout@v2
with:
path: 'source path'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
path: 'source path'
path: 'path with a space in it'

Copy link
Member

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?

Copy link
Author

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

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?

Copy link
Author

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?

Copy link
Member

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.

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.

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

@@ -4,6 +4,7 @@ on:
push:
branches:
- main
- rchiodo/test_with_space
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 temporary. The rest of this should be forcing a path with spaces in it though.

Comment on lines +132 to +133
special-working-directory: './path with spaces'
special-working-directory-relative: 'path with spaces'

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?

Copy link
Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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?

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?

Copy link
Author

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.

Copy link
Member

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.

.github/workflows/insiders.yml Show resolved Hide resolved
Copy link

@kimadeline kimadeline left a 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) 🧹

Copy link

@kimadeline kimadeline left a 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.

@rchiodo
Copy link
Author

rchiodo commented Oct 6, 2020

@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?

@kimadeline
Copy link

I believe that was done in #14214

@rchiodo rchiodo added the no-changelog No news entry required label Oct 6, 2020
@rchiodo rchiodo marked this pull request as ready for review October 6, 2020 20:40
@brettcannon
Copy link
Member

@rchiodo Kim is right, we moved over to GH Actions for the insiders build last week.

@rchiodo
Copy link
Author

rchiodo commented Oct 6, 2020

And are you guys both okay with me submitting even if all the tests don't pass on this next run?

@brettcannon
Copy link
Member

@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-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.31%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #14113   +/-   ##
=======================================
  Coverage        ?   59.91%           
=======================================
  Files           ?      709           
  Lines           ?    39334           
  Branches        ?     5698           
=======================================
  Hits            ?    23566           
  Misses          ?    14530           
  Partials        ?     1238           
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...47c6c9c. Read the comment docs.

@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
No Duplication information No Duplication information

@rchiodo rchiodo merged commit abf9a5e into main Oct 6, 2020
@rchiodo rchiodo deleted the rchiodo/test_with_space branch October 6, 2020 22:20
DonJayamanne pushed a commit that referenced this pull request Oct 30, 2020
* 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
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