GeezerVison

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.
Review status: 7 of 87 files reviewed at latest revision, 3 unresolved discussions.

test/powershell/Common/Test.Helpers.psm1, line 61 at r1 (raw file):

        {
            & $sb | Out-Null
            Throw "No Exception!"

Is this verbose enough?

test/powershell/Common/Test.Helpers.psm1, line 66 at r1 (raw file):

        {
            $_.FullyQualifiedErrorId | Should Be $FullyQualifiedErrorId | Out-Null
            Write-Output $PSItem

Why are we writing an exception to the output stream?

test/powershell/engine/Remoting/RoleCapabilityFiles.Tests.ps1, line 60 at r1 (raw file):

            $iss = [initialsessionstate]::CreateFromSessionConfigurationFile($PSSessionConfigFile, { $true })
        } | ShouldBeErrorId "PSInvalidOperationException"
        $exc.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should Be "InvalidRoleCapabilityFileExtension"

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.

Is this verbose enough?

Yes, see #2973 (comment)

Why are we writing an exception to the output stream?

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

should use single quotes unless you need to expand the string (same in other locations)

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?)
I agree that the single quotes are more properly. If it will be approved, then we should change our pattern and make changes in the PR.

Why are we writing an exception to the output stream?

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

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.
Review status: 8 of 87 files reviewed at latest revision, 3 unresolved discussions.

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:

Review status: 8 of 87 files reviewed at latest revision, 3 unresolved discussions.
So I know I still have 79 files to review and in the reviewable UI... which files they are.

James Truher (@JamesWTruher) From #3157 (comment)

I have a bit of a problem with this. The functionality of checking the FullyQualifiedErrorId rather than the exception message should be in Pester rather than our environment. Putting this code in our own environment does not help the community, but only increases the burden on our code maintenance.

New Pester version (V4) already has improved support for exceptions and offers a similar style. Only while it is still not ported on Unix.
You can see from this PR there is a great variety in such code, and even errors. Now we could bring all the same pattern. This code is more compact (less than 1100 lines), it is clearer and corresponds to the Pester style. The new pattern will allow us much easier to migrate to Pester V4.

Travis Plunk (@TravisEz13) I added the comment about writing an exception to output.

Travis Plunk (@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 Invoke-Pester .\CmsMessage.Tests.ps1 anymore, unless all test files that use this function explicitly import the helper module at the beginning of the file.

Dongbo Wang (@daxian-dbw)
Preferred way to run tests is to use Start-PSPester. Currently it compile our C# test helper tools. In the PR I added that Test.helper.psm1 is loaded too. Later we can do the same for other test helper modules.
Also see #3161 (comment)

One common workflow is to make many changes to code and a specific test (say Foo.tests.ps1). It's very convenient to just run .\Foo.tests.ps1 without Start-PSPester or Invoke-Pester.

One solution is to just import Test.helper.psm1 in build.psm1.
Another is to set $env:PSModulePath so that Test.helper.psm1 available for auto-loading.

Yes it is convenient. But we can run Start-PSPester once per session to configure environment.
Start-PSPester performs some special settings. If we use it, we carry out tests in exactly the same way as CIs.
I don't like the autoload because if we need to specify the path to the modules, we quickly load them all at once.
We could move the code into the Build module root to run when it loads but then we will have to reload the entire module if we need to apply any updates.

Ilya (@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:

1. Update test Helpers to new pattern
2. Work needed to use pattern broadly (probably need an issue to discuss what this looks like)
3. Update tests to use new pattern

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?