[PATCH] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail

[PATCH] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail

[Jeshua Smith]

Update the UnitTestAssertStatusEqual error message to print out the
expected value in addition to the seen value.

Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
---
 UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
index dc05bbd438..322daf318a 100644
--- a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
+++ b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
@@ -290,7 +290,7 @@ UnitTestAssertStatusEqual (
 {
   CHAR8  TempStr[MAX_STRING_SIZE];
 
-  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:%p)", Description, (VOID *)Status);
+  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:0x%llx expected:0x%llx)", Description, Status, Expected);
   _assert_true ((Status == Expected), TempStr, FileName, (INT32)LineNumber);
 
   return (Status == Expected);
-- 
2.25.1

Re: [edk2-devel] [PATCH] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail

[Michael D Kinney]

Hi Jeshuas,

This is a good idea to show the expected value.

%p was used on purpose because unit tests can be built for 32-bit or 64-bit and
the EFI_STATUS is same as RETURN_STATUS which is same as UINTN.  UINTN is 32-bits
for 32-bit unit test apps and 64-bit for 64-bit unit test apps.  %p prints a 
pointer sized value, which happens to match the UINTN for support CPU archs.

A couple options to consider:
1) Keep using %p instead of %llx.
2) Use UINT64 local variables to hold Status and Expected values and use %llx.
3) Use the MdePkg PrintLib to convert Status and Expected to string names and
   update message to show the name of the status value instead of the hex value.
4) Don't add a dependency on PrintLib and instead convert to string names in
   this same C file.

Best regards,

Mike


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeshua Smith via groups.io
> Sent: Wednesday, November 30, 2022 12:39 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; mikuback@linux.microsoft.com; sean.brogan@microsoft.com; Jeshua Smith
> <jeshuas@nvidia.com>
> Subject: [edk2-devel] [PATCH] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail
> 
> Update the UnitTestAssertStatusEqual error message to print out the
> expected value in addition to the seen value.
> 
> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
> ---
>  UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> index dc05bbd438..322daf318a 100644
> --- a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> +++ b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> @@ -290,7 +290,7 @@ UnitTestAssertStatusEqual (
>  {
>    CHAR8  TempStr[MAX_STRING_SIZE];
> 
> -  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:%p)", Description, (VOID *)Status);
> +  snprintf (TempStr, sizeof (TempStr), "UT_ASSERT_STATUS_EQUAL(%s:0x%llx expected:0x%llx)", Description, Status,
> Expected);
>    _assert_true ((Status == Expected), TempStr, FileName, (INT32)LineNumber);
> 
>    return (Status == Expected);
> --
> 2.25.1
> 
> 
> 
> 
> 

Re: [edk2-devel] [PATCH] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail

[Jeshua Smith]

Hi Mike,

Thanks for the explanation. That makes sense.

I think Option 1 is the simplest in this case, and I'll resubmit the patch with that.
Option 2 seems like an unnecessary workaround, since I can't think of a good reason to make the printed value a 64 bit value on 32-bit platforms.
Option 3 is interesting. The similar implementation of the function in Assert.c already uses the %r capability of PrintLib, but since this is the AssertCmocka.c implementation I assume the intent is to stick with only libc as a dependency.
Option 4 would perhaps be nicest, but has the downside of becoming an additional place to maintain the EFI_STATUS codes. I suppose the status codes will likely never change value, but if for some reason they did I can image that updating this lookup table might end up being missed, resulting in a very frustrating debug for someone who finds that the error message told them the wrong status code.

Jeshua

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@intel.com> 
Sent: Wednesday, November 30, 2022 1:57 PM
To: devel@edk2.groups.io; Jeshua Smith <jeshuas@nvidia.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: mikuback@linux.microsoft.com; sean.brogan@microsoft.com
Subject: RE: [edk2-devel] [PATCH] UnitTestFrameworkPkg/UnitTestLib: Print expected Status on ASSERT fail

External email: Use caution opening links or attachments


Hi Jeshuas,

This is a good idea to show the expected value.

%p was used on purpose because unit tests can be built for 32-bit or 64-bit and the EFI_STATUS is same as RETURN_STATUS which is same as UINTN.  UINTN is 32-bits for 32-bit unit test apps and 64-bit for 64-bit unit test apps.  %p prints a pointer sized value, which happens to match the UINTN for support CPU archs.

A couple options to consider:
1) Keep using %p instead of %llx.
2) Use UINT64 local variables to hold Status and Expected values and use %llx.
3) Use the MdePkg PrintLib to convert Status and Expected to string names and
   update message to show the name of the status value instead of the hex value.
4) Don't add a dependency on PrintLib and instead convert to string names in
   this same C file.

Best regards,

Mike


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jeshua 
> Smith via groups.io
> Sent: Wednesday, November 30, 2022 12:39 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; 
> mikuback@linux.microsoft.com; sean.brogan@microsoft.com; Jeshua Smith 
> <jeshuas@nvidia.com>
> Subject: [edk2-devel] [PATCH] UnitTestFrameworkPkg/UnitTestLib: Print 
> expected Status on ASSERT fail
>
> Update the UnitTestAssertStatusEqual error message to print out the 
> expected value in addition to the seen value.
>
> Signed-off-by: Jeshua Smith <jeshuas@nvidia.com>
> ---
>  UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> index dc05bbd438..322daf318a 100644
> --- a/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> +++ b/UnitTestFrameworkPkg/Library/UnitTestLib/AssertCmocka.c
> @@ -290,7 +290,7 @@ UnitTestAssertStatusEqual (  {
>    CHAR8  TempStr[MAX_STRING_SIZE];
>
> -  snprintf (TempStr, sizeof (TempStr), 
> "UT_ASSERT_STATUS_EQUAL(%s:%p)", Description, (VOID *)Status);
> +  snprintf (TempStr, sizeof (TempStr), 
> + "UT_ASSERT_STATUS_EQUAL(%s:0x%llx expected:0x%llx)", Description, 
> + Status,
> Expected);
>    _assert_true ((Status == Expected), TempStr, FileName, 
> (INT32)LineNumber);
>
>    return (Status == Expected);
> --
> 2.25.1
>
>
>
> 
>