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

Factor out path-related classes for better testing. #9352

Merged
merged 15 commits into from
Jan 8, 2020

Conversation

ericsnowcurrently
Copy link
Member

(for #8995)

This adds back in some of the refactored code and high-level functional tests reverted in #8970. It also adds more tests and gets things ready for #8542 (where we will drop the pathutils module, etc.).

@ericsnowcurrently ericsnowcurrently added the no-changelog No news entry required label Dec 31, 2019
@codecov-io
Copy link

codecov-io commented Dec 31, 2019

Codecov Report

Merging #9352 into master will increase coverage by 0.02%.
The diff coverage is 67.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9352      +/-   ##
==========================================
+ Coverage   60.93%   60.95%   +0.02%     
==========================================
  Files         529      530       +1     
  Lines       28580    28632      +52     
  Branches     4339     4338       -1     
==========================================
+ Hits        17415    17454      +39     
- Misses      10214    10227      +13     
  Partials      951      951
Impacted Files Coverage Δ
src/client/common/types.ts 100% <ø> (ø) ⬆️
src/client/common/platform/constants.ts 100% <ø> (ø) ⬆️
src/client/common/platform/types.ts 100% <100%> (ø) ⬆️
src/client/common/platform/pathUtils.ts 61.9% <53.84%> (+1.03%) ⬆️
src/client/common/platform/fileSystem.ts 26.87% <60%> (-0.57%) ⬇️
src/client/common/platform/fs-paths.ts 71.92% <71.92%> (ø)

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 734193b...f589160. Read the comment docs.

@karrtikr
Copy link

Can you please also manually run CI against this PR?

@ericsnowcurrently
Copy link
Member Author

@karrtikr Will do. What motivated you to ask? (it's not a normal step)

@DonJayamanne
Copy link

Your previous PR broke master when merged. When making large changes I'd recommend running CI against a PR.
It's not because you've done anything wrong, just that the functional tests are more extensive and thin on CI (as they are slow they dont run on PR pipelines)...

@ericsnowcurrently
Copy link
Member Author

Agreed. The failing functional tests are tests that I added. 😄 I just hadn't run them on Windows and apparently that did not happen during PR validation. (and FWIW, the change wasn't that large; most of it was tests)

Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

If the CI has no failing tests for this PR, feel free to merge it.

@ericsnowcurrently
Copy link
Member Author

I'm pretty sure those 2 failures are not due to this PR. I'll double-check tomorrow before merging.

@sonarcloud
Copy link

sonarcloud bot commented Jan 8, 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

@ericsnowcurrently
Copy link
Member Author

Yep, the Windows tests have been timing out on master and the OSX tests all passed when I re-ran CI.

@ericsnowcurrently ericsnowcurrently merged commit d42551a into microsoft:master Jan 8, 2020
@ericsnowcurrently ericsnowcurrently deleted the fs-paths branch January 8, 2020 18:51
@lock lock bot locked as resolved and limited conversation to collaborators Jan 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants