Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature Request: Expected, Actual, and Because as Test Property Fields #1993

Closed
JustinGrote opened this issue Jun 15, 2021 · 16 comments · Fixed by #2381
Closed

Feature Request: Expected, Actual, and Because as Test Property Fields #1993

JustinGrote opened this issue Jun 15, 2021 · 16 comments · Fixed by #2381
Labels
Milestone

Comments

@JustinGrote
Copy link
Contributor

JustinGrote commented Jun 15, 2021

Summary of the feature request

The vscode proposed test UI has a very nice view window showing expected and actual results including a diff
image

I would like to be able to display this similar information from Pester, but currently since the error is just a string, it would require a lot of parsing.

How should it work? (optional)

Ideally a failure would add 3 parameters either to the data of the exception or to the testresult itself:
Example with Should -Be: Expected $false, but got $true

  1. Expected - $false
  2. Actual - $true

Example with Should -BeOfType: Expected the value to have type [int] or any of its subtypes, but got $true with type [bool].

  1. Expected: [int]
  2. Actual: [bool]

Also support Because, which would probably be shown as a comment to Expected

As a workaround a regex parsing of "Expected x, but y" would probably suffice for most scenarios

@nohwnd
Copy link
Member

nohwnd commented Jun 15, 2021

Sounds good. We already transport some data on the exception (in the data dictionary of the exception), this would be adding more items. We should make sure we save the serialized version of the object, to avoid holding references to the real object.

https://github.com/pester/Pester/blob/main/src/functions/assertions/Should.ps1#L17-L18

@fflaten
Copy link
Collaborator

fflaten commented Jun 15, 2021

IIRC that function isn't called anymore. It's been converted to a c# method in Pester.Factory.

Concept probably still applies though.

@nohwnd
Copy link
Member

nohwnd commented Jun 15, 2021

Yeah, the concept should be okay, the data are imho attached to the data dictionary on the exeception which is defined on the Exception base class so any .net exception has it.

@JustinGrote
Copy link
Contributor Author

JustinGrote commented Jun 15, 2021

There is some information in targetObject but nothing specific to the error other than the message.

I think the most appropriate storage for this is $error.exception.data (currently blank), I'll go the parse route for now until this is available.

$x.errorrecord.targetobject

Key         Value
---         -----
Message     Expected $false, but got $true.
File        C:\Users\JGrote\Projects\vscode-pester-test-adapter\sample\Tests\Sample.tests.ps1
Line        5
LineText            It 'Context False' {$true | Should -be $false}
Terminating True

@nohwnd nohwnd added this to the 5.4 milestone Jun 16, 2021
@ArmaanMcleod
Copy link
Contributor

ArmaanMcleod commented Jul 11, 2021

I'd be happy to PR this @nohwnd @JustinGrote.

A lot of the Should functions return a PSCustomObject with Succeeded/FailureMessage properties. Thinking we can can include Expected/Actual/Because here and adding these to the error record with the CreateShouldErrorRecord call inside Invoke-Assertion. Would also need to modify CreateErrorRecord to include these new properties inside the targetObject dictionary.

This would actually be very useful for the work I'm doing. I am also just parsing the message with regex and being able to access these properties inside the exception would be great.

@nohwnd
Copy link
Member

nohwnd commented Jul 12, 2021

Exception is a universal carrier of failure info. You can throw exceptions from your own assertions if you don't want to use Should. That is why I am suggesting to use Data on Exception object.

@JustinGrote
Copy link
Contributor Author

6 month follow up :)

@nohwnd
Copy link
Member

nohwnd commented Apr 22, 2022

@JustinGrote hello from 4 more months into the future :D Someone needs to just implement this. Maybe you yourself? 😉

JustinGrote added a commit to JustinGrote/_PR-Pester that referenced this issue Jul 19, 2023
nohwnd added a commit that referenced this issue Apr 16, 2024
* Feature Request: Expected, Actual, and Because as Test Property Fields
Fixes #1993

* Add lingering "Be" because value

* Add missing because argument

* Add a strong type to ShouldResult

* Fix issue with HaveCount having a typed ExpectedValue parameter

* Update src/functions/assertions/Should.ps1

Co-authored-by: Frode Flaten <3436158+fflaten@users.noreply.github.com>

* Refactor to guard clauses to ensure success records do not have additional properties

* CreateShouldErrorRecord to object type

* Add ExpectResult and strong typing to Should -Invoke

* Remove null failuremessage

---------

Co-authored-by: Frode Flaten <3436158+fflaten@users.noreply.github.com>
Co-authored-by: Jakub Jareš <me@jakubjares.com>
@JustinGrote
Copy link
Contributor Author

@nohwnd thank you!

@nohwnd
Copy link
Member

nohwnd commented Apr 25, 2024

Thank you :)

@nohwnd
Copy link
Member

nohwnd commented Apr 25, 2024

It is now released in beta if you want to promote or try it further.

@fflaten
Copy link
Collaborator

fflaten commented Apr 26, 2024

@nohwnd The github release tag + notes are missing this + 3 other commits.
image

The published build is fine.

@nohwnd
Copy link
Member

nohwnd commented Apr 26, 2024 via email

@nohwnd
Copy link
Member

nohwnd commented Apr 26, 2024

I've found only 2 commits missing, the tag was on wrong commit. not sure what other 2 I a missing, maybe those are the two for dropping powershell support and revert of that , which are linked at the bottom of the release notes?

@fflaten
Copy link
Collaborator

fflaten commented Apr 26, 2024

I think it was the azure-pipelines commits. But all good now, thanks 🙂

@nohwnd
Copy link
Member

nohwnd commented Apr 26, 2024

Yeah I think I made a commit locally into main, and then pulled because there was extra work, but instead of forwarding it merged, and I did not want that, so I reset to the remote state, cherry-pick the changes and push.

But before this I've tagged the latest commit, but that commit did not end up on main, because I cherry picked it.

And I do recall that I've reapplied the tag, but I probably did it wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants