Add test for -WhatIf for New-FileCatalog#8966
Conversation
| { | ||
| PerformAction(paths, catalogFilePath); | ||
| } | ||
|
|
There was a problem hiding this comment.
Please remove this newline
|
|
||
| if (ShouldProcess(catalogFilePath)) | ||
| string action = StringUtil.Format(Modules.CreatingModuleManifestFile, catalogFilePath); | ||
|
|
| $result | Should -Be "Valid" | ||
| } | ||
|
|
||
| It "TestCatalogWhatIfForNewFileCatalog"{ |
There was a problem hiding this comment.
| It "TestCatalogWhatIfForNewFileCatalog"{ | |
| It "New-FileCatalog -WhatIf does not create file" { |
There was a problem hiding this comment.
The rest of the file has the smooshed test titles... do you think we should move away from what the rest of the file does, @rjmholt?
There was a problem hiding this comment.
Generally speaking in newer tests we opt for not smooshing them. Probably this test file should have a followup PR to make the test titles more readable across the whole file when we're done here. 🙂
| } | ||
|
|
||
| if (ShouldProcess(catalogFilePath)) | ||
| string action = StringUtil.Format(Modules.CreatingModuleManifestFile, catalogFilePath); |
There was a problem hiding this comment.
I noticed here you are using the
Modules.CreatingModuleManifestFile
property which comes from the file:
PowerShell/src/System.Management.Automation/resources/Modules.resx
Rather than using that one, you should add a new entry in that resx file called like CreatingFileCatalog
and use it instead.
NOTE: In order to rebuild the resx files... instead of running
Start-PSBuild
you run
Start-PSBuild -Clean
There was a problem hiding this comment.
Actually @TylerLeonhardt if all you're doing is regenerating your resx files you want to Start-PSBuild -ResGen
But yeah clean build is easy and covers it for ya. 😄
|
I wonder if this fix works. If it works it is a tricky bug in ShouldProcess() and it is better fix the bug. |
|
@iSazonov the fix does work (hence the added test). And I agree with you that the underlying bug should be investigated and fixed. That said, one thing that this fix does is align the ShouldProcess call to the same call as So I still think this PR is valid. The student also followed up on the original issue regarding this bug in ShouldProcess |
The test is passed without the fix. |
|
@TylerLeonhardt @rjmholt @iSazonov @vexx32 Looks like there is a conversation about our code update / PR. However, I will leave at that here, and look forward to working with your team in the future. Thanks to Rob and Tyler for their mentorship and guidance at the hackathon. I learned a lot about PowerShell, GitHub, and open source. |
|
@TylerLeonhardt @rjmholt Can you repro? If no please close the PR. |
|
This already seems to work in |
|
The test addition is worth something... |
30f5498 to
2bcc1cf
Compare
3cd968d to
ea4f4df
Compare
|
Have patched this up |
PR Summary
fixes #2866.
Implemented by @mjanko5 (me) and @MarekZietek with the guidance of @TylerLeonhardt and @rjmholt at HackIllinois 2019.
PR Context
The new code adds a second parameter to the ShouldProcess method to CatalogCommands.cs, which makes -WhatIf work for the New-FileCatalog command.
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.[feature]to your commit messages if the change is significant or affects feature tests