Move test "No Exception!" pattern to 'ShouldBeErrorId' #3161
Conversation
43b49f8 to
9b96469
Compare
It turns out we have a excellent function 'ShouldBeErrorId' in 'Common\Test.Helpers.psm1' I slightly modified this function to be able to passthrow the exception result for later analysis. I searched `throw [",']` in test files for replacing "No Exception!" pattern with 'ShouldBeErrorId'. I did not touch some files that contains their own specific version "No Exception!" pattern or contains specific tests (ex., *-Variable cmdlets with scopes). Also I was forced to resolve some aliases and sometimes fix formatting.
9b96469 to
d9d1639
Compare
|
Class tests is failed on IOS 😕 |
|
restarted iOS |
|
Using reviewable because it helps me track my progress reviewing a PR. Not done, but here are my comments so far. Reviewed 7 of 87 files at r1. test/powershell/Common/Test.Helpers.psm1, line 61 at r1 (raw file):
Is this verbose enough? test/powershell/Common/Test.Helpers.psm1, line 66 at r1 (raw file):
Why are we writing an exception to the output stream? test/powershell/engine/Remoting/RoleCapabilityFiles.Tests.ps1, line 60 at r1 (raw file):
should use single quotes unless you need to expand the string (same in other locations) Comments from Reviewable |
|
It seems the Reviewable is inconvenient to reply.
Yes, see #2973 (comment)
Before the change we was writing true/false to the output stream. Now an exception because sometimes we should check not only FullyQualifiedErrorId but also other properties (it is used in the PR).
Original code is almost always uses double quotes and this pattern is from our guide (Why do we need a guide if we do not comply with it?) |
Can you add a comment to the code to indicate this why we are writing the error to the output stream? Reviewed 1 of 87 files at r1. Comments from Reviewable |
|
Yes, Reviewable is inconvenient to reply, unless you use reviewable. I don't like it unless I'm reviewing a very large PR like this one. It keeps track of this status:
|
|
@JamesWTruher From #3157 (comment)
New Pester version (V4) already has improved support for exceptions and offers a similar style. Only while it is still not ported on Unix. @TravisEz13 I added the comment about writing an exception to output. |
There was a problem hiding this comment.
Can you fix the merge conflict?
I do like the change. I think it should be fine even if we move to a newer version of Pester, and if it isn't, we can help improve Pester based on our experience with this pattern.
I only sampled the changes - I'll let Travis finish his review.
| @@ -1,5 +1,7 @@ | |||
| using namespace System.Diagnostics | |||
|
|
|||
| Import-Module $PSScriptRoot\..\Common\Test.Helpers.psm1 | |||
There was a problem hiding this comment.
It would be nice if this could be less fragile to directory structure - you couldn't move Test.Helpers.psm1 without making many changes. On the other hand, one obvious alternative Import-Module $TestHelpers or something like that would require running something first, like Start-PSPester, which we don't need to do today, you can just run the test directly, like .\Foo.Tests.ps1.
It would be even nicer if it was not necessary.
If $env:PSModulePath is set correctly and the module is a more proper module (in a directory named Test.Helpers), then we could rely on module auto-loading to load the module, except maybe in a test specific to module auto-loading.
There was a problem hiding this comment.
I don't know how to make this a universal.
Can we rely only on CI and Start-PSPester?
There was a problem hiding this comment.
My thinking:
say we have the module structure:
PowerShell
| test
| Modules
| TestHelpers
| TestHelpers.psd1
| TestHelpers.psm1
Then, in Start-PSPester, ensure PowerShell\test\Modules is in $env:PSModulePath.
As long as the commands are exported properly, then the module will be auto-loaded.
Developers not wanting to run Start-PSPester would just ensure they have their module path set correctly.
There was a problem hiding this comment.
Currently our helpers modules is in test\powershell\Common\ folder - maybe we leave them there?
There was a problem hiding this comment.
I suggested higher up because shared tools may be useful for I feel like the directory structure is already a bit too deep and not that intuitive. At any rate, tools feel like they shouldn't been too hidden.
There was a problem hiding this comment.
in what way is our structure not intuitive? In what way is our structure too deep? I don't agree with that at all. I certainly agree that our tools should not be hidden, and don't see how having globally applicable test tools in test/PowerShell/common equates to hiding.
There was a problem hiding this comment.
Clear.
I have a couple more questions.
- We have a few more helpers modules (ex., for parser). Should we move them also in the new shared folder?
- Can we make these modules as submodules general module and load all the modules with a single command? What is the point of importing them in multiple test files?
There was a problem hiding this comment.
Maybe it's just me noticing the carryo 8000 ver from sharing 1 test tree for many products, I'd just prefer a flatter structure and/or one that is easier to find corresponding tests, maybe something like:
test
| Tools
| Modules
| TestHelper.psm1
| System.Management.Automation
| Microsoft.PowerShell.Commands
| Get-Variable.tests.ps1
| Language (or System.Management.Automation.Language)
| Parser.tests.ps1
| Remoting
| Serialization.tests.ps1
| Microsoft.PowerShell.Commands.Utility
| Object (grouping by noun, grouping by verb might make sense too)
| Measure-Object.tests.ps1
| New-Object.tests.ps1
| Microsoft.PowerShell.Commands.Management
| PSReadline
etc.
I realize some tests don't fall neatly into testing an api - and at least System.Management.Automation namespace is overly large, so requires some additional structure, but this gives an overview on what I'd find more intuitive.
There was a problem hiding this comment.
In the PR I moved Test.Helpers.psm1 to test\Tools\Modules folder and modifyed Start-PSPester to load the module.
To change the whole structure we need to open new Issue. @lzybkr Could you move your proposal in this new Issue?
|
@TravisEz13 Could you continue? There are too many files and we get merge conflicts often. |
|
I'm a little worried about using this function, as I won't be able to directly run |
|
@daxian-dbw |
|
One common workflow is to make many changes to code and a specific test (say One solution is to just import |
|
Yes it is convenient. But we can run |
|
@iSazonov I think this idea needs more discussion. It may be better to file an issue and discuss the work. Then do the work in stages to avoid having a large PR which will constantly get merge conflicts. Here is a way you break this PR up:
|
|
Per our discussion I think this PR can be closed? |
|
Let's make it a little later. This will help me move the commits to new PRs. |
|
Can we close this now? |
It turns out we have a excellent function 'ShouldBeErrorId' in
'Common\Test.Helpers.psm1'
I slightly modified this function to be able to passthrow the exception
result for later analysis.
I searched
throw [",']in test files for replacing "No Exception!" patternwith 'ShouldBeErrorId'.
I did not touch some files that contains their own specific version "No
Exception!" pattern or contains specific tests (ex., *-Variable cmdlets
with scopes).
Also I was forced to resolve some aliases and sometimes fix formatting.
This change is