Feature Request: ShouldResult property

[JustinGrote]

Fixes #1993

PR Summary

PR Checklist

• PR has meaningful title
• Summary describes changes
• PR is ready to be merged
  • If not, use the arrow next to Create Pull Request to mark it as a draft. PR can be marked Ready for review when it's ready.
• Tests are added/update (if required)
• Documentation is updated/added (if required)

[JustinGrote]

@fflaten @nohwnd please review

[fflaten]

Thanks again for looking into this 👏
Can't test and fully review for a few days, but added some initial thoughts.

[JustinGrote]

@fflaten @nohwnd ready for re-review.

I strong typed the result object for all Should results, and kept the info in the TargetObject without exposing it outwardly. Plugins should still be able to access the info as needed.

[nohwnd]

Here it populates the values no matter if the should fails, in some other places it only populates them when should fails. What is the desired way?

[JustinGrote]

Defer to @fflaten but seems to me it makes sense to not populate it if it's successful for minor performance benefit, the only use that info would be is troubleshooting (e.g. it said successful but it shouldn't have been)

[fflaten]

Wouldn't be exposed on success atm. so should avoid unnecessary cost imo 🙂

[JustinGrote]

All assertions should now be adjusted to not include this property on a success. Maybe we need tests for this?

[nohwnd]

Thanks for the PR, I am slightly worried about degrading perf by re-formatting the data where we don't need to, because the number of interactive runs where the functionality will be used is way smaller than the total of runs where this won't be used, and everyone will pay the price.

Maybe conditioning it based on option, or env var, or both is the best way forward here. Some options (like CI output) already use this pattern.

And then taking care to not reformat the same data in the assertions.

[JustinGrote]

@fflaten @nohwnd The strong typing already showed me that Should -Invoke is actually defined in Mock and not in its own assertion, and was emitting pscustomobjects from there. There's also somewhere that multiple "trues" along with the final object are being sent and that's what's causing my latest tests to fail after merging @fflaten's change.

Given this, to avoid some more potential regressions I propose that the builtin assertions I have modified will continue to emit the strong typed ShouldResult, but invoke-assertion and CreateShouldErrorRecord should still have loose pscustomobject input (via a constructor override) to facilitate both loose and strong typed objects for the targetobject field. This will also address @nohwnd's concern we might break custom Should statements that include additional metadata.

[fflaten]

The strong typing already showed me that Should -Invoke is actually defined in Mock and not in its own assertion

Right. Not aware of the history there. I've been considering a PR to move those, Invoke-Pester and a couple others functions not used during import to separate files to make it easier to find them and be consistent - at least for public functions.

There's also somewhere that multiple "trues" along with the final object

Sounds like a bug and another benefit of strongly typing 🙂

It should've been typed already as the output requirements are documented and cast works. There's no way to access additional properties through Should, and hopefully users would stick to either Should -MyCustomAssertion (required output) or direct calls to function MyCustomAssertion (no required output besides throw on error) consistently.

However, I guess there's a slight chance someone leverages the loose mapping to include something for ex. unit-testing MyCustomAssertion-function directly. So probably smart to keep it loose until 6.0 and/or Better Should. Specifically #1423 might shakes things up either way.

[JustinGrote]

I dropped the CreateShouldErrorRecord method back to object (34b4744) but all builtin items will still use the result type and I'll also fix up the shouldInvoke output as well to include actual/expected.

[nohwnd]

I've run it bunch of times, it does not seem to impact the speed negatively. Okay to merge imho.

@fflaten wanna have one last look?

[JustinGrote]

@nohwnd thank you!

[nohwnd]

Thank you both :)