Enhance Remove-Item to work with OneDrive #14902
Conversation
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1
Show resolved
Hide resolved
| #New-Item -ItemType SymbolicLink -Path $alphaLink -Value $alphaDir | ||
| #New-Item -ItemType SymbolicLink -Path $betaLink -Value $betaDir |
There was a problem hiding this comment.
I keep the file style.
There was a problem hiding this comment.
The file style is to keep commented out lines of code?
There was a problem hiding this comment.
Full text is:
# The test depends on the files created in previous test:
#New-Item -ItemType SymbolicLink -Path $alphaLink -Value $alphaDir
#New-Item -ItemType SymbolicLink -Path $betaLink -Value $betaDir
The same style is in previous test, and two tests above too.
It's not very good that the tests depend on each other, but this has already been.
There was a problem hiding this comment.
It seems like that should be in a Context BeforeAll block, and then torn down in an AfterAll block
There was a problem hiding this comment.
As I pointed "It's not very good that the tests depend on each other, but this has already been." I would not want to redo all these tests now. It is better to do in separate PR. See also #14902 (comment)
I opened #15178 for tracking.
| [System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestOn', $true) | ||
| [System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestRecuseOn', $false) |
There was a problem hiding this comment.
Rather than this, these tests are probably better within their own context, with a BeforeAll and AfterAll for the test hooks
There was a problem hiding this comment.
We could but I again keep the file style and follow a style of previous tests.
There was a problem hiding this comment.
Ok, well I think it's time to break with this file's style because it's cleaner to do things this way. We've been very happy to write sweeping style changes elsewhere with no conceivable benefit, and in this case there is a clear benefit
There was a problem hiding this comment.
It will rather add confusion.
Earlier I just made a new FileSystemProviderExtended.Tests.ps1 file when there were many new tests to be created.
If we have to change something, then exactly the same.
We are already aware of many bugs and performance issues in FileSystemProvider. I could fix a lot there including tests. But while my PRs are frozen, I see no reason to change something minor now.
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
We need reviews from some other contributors here. I'm hoping some of the reviewers I've added can provide a review, since I know less about filesystem interactions. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
| #New-Item -ItemType SymbolicLink -Path $alphaLink -Value $alphaDir | ||
| #New-Item -ItemType SymbolicLink -Path $betaLink -Value $betaDir |
There was a problem hiding this comment.
It seems like that should be in a Context BeforeAll block, and then torn down in an AfterAll block
| [System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestOn', $true) | ||
| [System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestRecurseOn', $false) |
There was a problem hiding this comment.
This is also the kind of thing that would be better in a BeforeEach block in a Context
|
🎉 Handy links: |
[7.2.0-preview.5] - 2021-04-14 * Breaking Changes - Make PowerShell Linux deb and RPM packages universal (#15109) - Enforce AppLocker Deny configuration before Execution Policy Bypass configuration (#15035) - Disallow mixed dash and slash in command line parameter prefix (#15142) (Thanks @davidBar-On!) * Experimental Features - `PSNativeCommandArgumentPassing`: Use `ArgumentList` for native executable invocation (breaking change) (#14692) * Engine Updates and Fixes - Add `IArgumentCompleterFactory` for parameterized `ArgumentCompleters` (#12605) (Thanks @powercode!) * General Cmdlet Updates and Fixes - Fix SSH remoting connection never finishing with misconfigured endpoint (#15175) - Respect `TERM` and `NO_COLOR` environment variables for `$PSStyle` rendering (#14969) - Use `ProgressView.Classic` when Virtual Terminal is not supported (#15048) - Fix `Get-Counter` issue with `-Computer` parameter (#15166) (Thanks @krishnayalavarthi!) - Fix redundant iteration while splitting lines (#14851) (Thanks @hez2010!) - Enhance `Remove-Item -Recurse` to work with OneDrive (#14902) (Thanks @iSazonov!) - Change minimum depth to 0 for `ConvertTo-Json` (#14830) (Thanks @kvprasoon!) - Allow `Set-Clipboard` to accept empty string (#14579) - Turn on and off `DECCKM` to modify keyboard mode for Unix native commands to work correctly (#14943) - Fall back to `CopyAndDelete()` when `MoveTo()` fails due to an `IOException` (#15077) * Code Cleanup <details> <summary> <p>We thank the following contributors!</p> <p>@xtqqczze, @iSazonov, @ZhiZe-ZG</p> </summary> <ul> <li>Update .NET to <code>6.0.0-preview.3</code> (#15221)</li> <li>Add space before comma to hosting test to fix error reported by <code>SA1001</code> (#15224)</li> <li>Add <code>SecureStringHelper.FromPlainTextString</code> helper method for efficient secure string creation (#14124) (Thanks @xtqqczze!)</li> <li>Use static lambda keyword (#15154) (Thanks @iSazonov!)</li> <li>Remove unnecessary <code>Array</code> -> <code>List</code> -> <code>Array</code> conversion in <code>ProcessBaseCommand.AllProcesses</code> (#15052) (Thanks @xtqqczze!)</li> <li>Standardize grammar comments in Parser.cs (#15114) (Thanks @ZhiZe-ZG!)</li> <li>Enable <code>SA1001</code>: Commas should be spaced correctly (#14171) (Thanks @xtqqczze!)</li> <li>Refactor <code>MultipleServiceCommandBase.AllServices</code> (#15053) (Thanks @xtqqczze!)</li> </ul> </details> * Tools - Use Unix line endings for shell scripts (#15180) (Thanks @xtqqczze!) * Tests - Add the missing tag in Host Utilities tests (#14983) - Update `copy-props` version in `package.json` (#15124) * Build and Packaging Improvements <details> <summary> <p>We thank the following contributors!</p> <p>@JustinGrote</p> </summary> <ul> <li>Fix <code>yarn-lock</code> for <code>copy-props</code> (#15225)</li> <li>Make package validation regex accept universal Linux packages (#15226)</li> <li>Bump NJsonSchema from 10.4.0 to 10.4.1 (#15190)</li> <li>Make MSI and EXE signing always copy to fix daily build (#15191)</li> <li>Sign internals of EXE package so that it works correctly when signed (#15132)</li> <li>Bump Microsoft.NET.Test.Sdk from 16.9.1 to 16.9.4 (#15141)</li> <li>Update daily release tag format to work with new Microsoft Update work (#15164)</li> <li>Feature: Add Ubuntu 20.04 Support to install-powershell.sh (#15095) (Thanks @JustinGrote!)</li> <li>Treat rebuild branches like release branches (#15099)</li> <li>Update WiX to 3.11.2 (#15097)</li> <li>Bump NJsonSchema from 10.3.11 to 10.4.0 (#15092)</li> <li>Allow patching of preview releases (#15074)</li> <li>Bump Newtonsoft.Json from 12.0.3 to 13.0.1 (#15084, #15085)</li> <li>Update the <code>minSize</code> build package filter to be explicit (#15055)</li> <li>Bump NJsonSchema from 10.3.10 to 10.3.11 (#14965)</li> </ul> </details> * Documentation and Help Content - Merge `7.2.0-preview.4` changes to master (#15056) - Update `README` and `metadata.json` (#15046) - Fix broken links for `dotnet` CLI (#14937) [7.2.0-preview.5]: v7.2.0-preview.4...v7.2.0-preview.5
This reverts commit fafc38f.
|
/azp list |
PR Summary
Before the fix only enumeration (Get-ChildItem) worked on OneDrive.
Now Remove-Item correctly removes folders and files on OneDrive too.
PR Context
Replace #14860
Related #9246
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).