[edk2-devel] ACPI table generator unit test

[edk2-devel] ACPI table generator unit test

[yodagump via groups.io]

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

Hi,

I plan to use EDK2 unit test framework (https://github.com/SophiaWang-Google/edk2/tree/master/UnitTestFrameworkPkg) to test ACPI table generator, such as https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c

However, ACPI table generator only has two non-static function exposed:
EFI_STATUS
EFIAPI
AcpiGtdtLibConstructor (
IN  EFI_HANDLE        ImageHandle,
IN  EFI_SYSTEM_TABLE  *SystemTable
)

EFI_STATUS
EFIAPI
AcpiGtdtLibDestructor (
IN  EFI_HANDLE        ImageHandle,
IN  EFI_SYSTEM_TABLE  *SystemTable
)

So if we want to use GoogleTest gMock to unit test ACPI table generator, I guess we need to expose API :
STATIC
EFI_STATUS
EFIAPI
BuildGtdtTable (
IN  CONST ACPI_TABLE_GENERATOR                  *CONST  This,
IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST  AcpiTableInfo,
IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
OUT       EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
)
to non-static

Please let me whether this is the approach upstream target for, or any better solutions on this?

Thank you,
Sophia


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121226): https://edk2.groups.io/g/devel/message/121226
Mute This Topic: https://groups.io/mt/111930977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 2692 bytes --]

Re: [edk2-devel] ACPI table generator unit test

[Sami Mujawar via groups.io]

[-- Attachment #1: Type: text/plain, Size: 2520 bytes --]

Hi Sophia,

Unfortunately, I am not conversant with the Google Test framework. I will need to get around to understanding it at some point.

However, there is a recent PR that adds some tests. Can you take a look at the patch at: https://github.com/tianocore/edk2/pull/10833/commits/4eeeb1434820718ace1670d9429a867fe5e8c9b7
to see if it helps, please?

Regards,

Sami Mujawar
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of yodagump via groups.io <yodagump=google.com@groups.io>
Date: Thursday, 27 March 2025 at 04:10
To: devel@edk2.groups.io <devel@edk2.groups.io>
Subject: [edk2-devel] ACPI table generator unit test
Hi,

I plan to use EDK2 unit test framework (https://github.com/SophiaWang-Google/edk2/tree/master/UnitTestFrameworkPkg) to test ACPI table generator, such as https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c

However, ACPI table generator only has two non-static function exposed:
EFI_STATUS
EFIAPI
AcpiGtdtLibConstructor (
  IN  EFI_HANDLE        ImageHandle,
  IN  EFI_SYSTEM_TABLE  *SystemTable
  )

EFI_STATUS
EFIAPI
AcpiGtdtLibDestructor (
  IN  EFI_HANDLE        ImageHandle,
  IN  EFI_SYSTEM_TABLE  *SystemTable
  )

So if we want to use GoogleTest gMock to unit test ACPI table generator, I guess we need to expose API :
STATIC
EFI_STATUS
EFIAPI
BuildGtdtTable (
  IN  CONST ACPI_TABLE_GENERATOR                  *CONST  This,
  IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST  AcpiTableInfo,
  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
  OUT       EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
  )
to non-static

Please let me whether this is the approach upstream target for, or any better solutions on this?

Thank you,
Sophia


IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121229): https://edk2.groups.io/g/devel/message/121229
Mute This Topic: https://groups.io/mt/111930977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 7560 bytes --]

Re: [edk2-devel] ACPI table generator unit test

[Sophia Wang via groups.io]

[-- Attachment #1: Type: text/plain, Size: 3552 bytes --]

Thanks Sami for the reply.

https://github.com/tianocore/edk2/pull/10833/files is indeed adding mock
configuration manager protocols for unit tests. But the issue for unit test
code cannot call the build function of ACPI table generator still exists.
Currently we only have two non-static function exposed

EFI_STATUS
EFIAPI
AcpiGtdtLibConstructor (
  IN  EFI_HANDLE        ImageHandle,
  IN  EFI_SYSTEM_TABLE  *SystemTable
  )

EFI_STATUS
EFIAPI
AcpiGtdtLibDestructor (
  IN  EFI_HANDLE        ImageHandle,
  IN  EFI_SYSTEM_TABLE  *SystemTable
  )

And IMO, we need to expose more functions as non-static for the ACPI table
unit test purpose. Please feel free to share your insights.

Best,
Sophia

On Thu, Mar 27, 2025 at 6:37 AM Sami Mujawar <Sami.Mujawar@arm.com> wrote:

> Hi Sophia,
>
>
>
> Unfortunately, I am not conversant with the Google Test framework. I will
> need to get around to understanding it at some point.
>
>
>
> However, there is a recent PR that adds some tests. Can you take a look at
> the patch at:
> https://github.com/tianocore/edk2/pull/10833/commits/4eeeb1434820718ace1670d9429a867fe5e8c9b7
>
> to see if it helps, please?
>
>
>
> Regards,
>
>
>
> Sami Mujawar
>
> *From: *devel@edk2.groups.io <devel@edk2.groups.io> on behalf of yodagump
> via groups.io <yodagump=google.com@groups.io>
> *Date: *Thursday, 27 March 2025 at 04:10
> *To: *devel@edk2.groups.io <devel@edk2.groups.io>
> *Subject: *[edk2-devel] ACPI table generator unit test
>
> Hi,
>
>
>
> I plan to use EDK2 unit test framework (
> https://github.com/SophiaWang-Google/edk2/tree/master/UnitTestFrameworkPkg)
> to test ACPI table generator, such as
> https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Library/Acpi/Arm/AcpiGtdtLibArm/GtdtGenerator.c
>
>
>
> However, ACPI table generator only has two non-static function exposed:
>
> EFI_STATUS
> EFIAPI
> AcpiGtdtLibConstructor (
>   IN  EFI_HANDLE        ImageHandle,
>   IN  EFI_SYSTEM_TABLE  *SystemTable
>   )
>
> EFI_STATUS
> EFIAPI
> AcpiGtdtLibDestructor (
>   IN  EFI_HANDLE        ImageHandle,
>   IN  EFI_SYSTEM_TABLE  *SystemTable
>   )
>
>
>
> So if we want to use GoogleTest gMock to unit test ACPI table generator, I
> guess we need to expose API :
>
> STATIC
> EFI_STATUS
> EFIAPI
> BuildGtdtTable (
>   IN  CONST ACPI_TABLE_GENERATOR                  *CONST  This,
>   IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST  AcpiTableInfo,
>   IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
>   OUT       EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
>   )
>
> to non-static
>
>
>
> Please let me whether this is the approach upstream target for, or any
> better solutions on this?
>
>
>
> Thank you,
>
> Sophia
>
>
>
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121231): https://edk2.groups.io/g/devel/message/121231
Mute This Topic: https://groups.io/mt/111930977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 7928 bytes --]

Re: [edk2-devel] ACPI table generator unit test

[Paul Fagerburg via groups.io]

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

How does someone write unit tests for functions that are declared ` STATIC ( https://github.com/tianocore/edk2/blob/3907f8a0bab35c90a0d4ee0352a6fdec04b35bd2/BaseTools/Source/C/Include/Common/BaseTypes.h#L26 ) `? Clearly there are some functions where the answer is "you don't, leave it alone, there's a good reason we didn't export it." But in cases where there is a lot of logic in a static function, it's a really good idea to test that. In the past, I've seen something like

```
#ifdef UNIT_TEST
#define STATIC
#else
#define STATIC static
#endif
```

to enable unit tests to call static functions. Would something like this be acceptable upstream?

-paul


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121238): https://edk2.groups.io/g/devel/message/121238
Mute This Topic: https://groups.io/mt/111930977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 1624 bytes --]

Re: [edk2-devel] ACPI table generator unit test

[Sophia Wang via groups.io]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

Gentle ping on this thread.

As Paul mentioned, there are only two non-static functions for ACPI table generator - AcpiHmatLibConstructor() and
AcpiHmatLibDestructor()

In order to do the unit test, we need to expose at least one STATIC to be non-STATIC.
CONST
ACPI_TABLE_GENERATOR  HmatGenerator.

So we can use
https://github.com/tianocore/edk2/pull/10833/commits/4eeeb1434820718ace1670d9429a867fe5e8c9b7
to register HMAT generator (or other generators).

Currently https://github.com/tianocore/edk2/pull/10833/commits/4eeeb1434820718ace1670d9429a867fe5e8c9b7 solution is not functional given no function call on RegisterAcpiTableGenerator().

I guess we all need to wait for upstream's decision on STATIC and NON_STATIC exposure for ACPI table generator unit test.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121246): https://edk2.groups.io/g/devel/message/121246
Mute This Topic: https://groups.io/mt/111930977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 3257 bytes --]

Re: [edk2-devel] ACPI table generator unit test

[Ard Biesheuvel via groups.io]

cc Sami and Pierre

On Wed, 9 Apr 2025 at 21:17, Sophia Wang via groups.io
<yodagump=google.com@groups.io> wrote:
>
> Gentle ping on this thread.
>
> As Paul mentioned, there are only two non-static functions for ACPI table generator - AcpiHmatLibConstructor() and
> AcpiHmatLibDestructor()
>
> In order to do the unit test, we need to expose at least one STATIC to be non-STATIC.
> CONST
> ACPI_TABLE_GENERATOR  HmatGenerator.
>
> So we can use
> https://github.com/tianocore/edk2/pull/10833/commits/4eeeb1434820718ace1670d9429a867fe5e8c9b7
> to register HMAT generator (or other generators).
>
> Currently https://github.com/tianocore/edk2/pull/10833/commits/4eeeb1434820718ace1670d9429a867fe5e8c9b7 solution is not functional given no function call on RegisterAcpiTableGenerator().
>
> I guess we all need to wait for upstream's decision on STATIC and NON_STATIC exposure for ACPI table generator unit test.
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121254): https://edk2.groups.io/g/devel/message/121254
Mute This Topic: https://groups.io/mt/111930977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] ACPI table generator unit test

[PierreGondois via groups.io]

Hello Ard, Sophia,

On 4/10/25 08:58, Ard Biesheuvel wrote:
> cc Sami and Pierre
> 
> On Wed, 9 Apr 2025 at 21:17, Sophia Wang via groups.io
> <yodagump=google.com@groups.io> wrote:
>>
>> Gentle ping on this thread.
>>
>> As Paul mentioned, there are only two non-static functions for ACPI table generator - AcpiHmatLibConstructor() and
>> AcpiHmatLibDestructor()
>>
>> In order to do the unit test, we need to expose at least one STATIC to be non-STATIC.
>> CONST
>> ACPI_TABLE_GENERATOR  HmatGenerator.
>>
>> So we can use
>> https://github.com/tianocore/edk2/pull/10833/commits/4eeeb1434820718ace1670d9429a867fe5e8c9b7
>> to register HMAT generator (or other generators).
>>
>> Currently https://github.com/tianocore/edk2/pull/10833/commits/4eeeb1434820718ace1670d9429a867fe5e8c9b7 solution is not functional given no function call on RegisterAcpiTableGenerator().

I'm not sure I understand why the solution for the DBG2 is not functional.
It seems to work for me with the following command line:
$ stuart_ci_build -c .pytool/CISettings.py TOOL_CHAIN_TAG=GCC5 -a X64 -p DynamicTablesPkg GuidCheck=skip HostUnitTestDscCompleteCheck=skip CharEncodingCheck=skip CompilerPlugin=skip SpellCheck=skip LibraryClassCheck=skip LicenseCheck=skip DscCompleteCheck=skip DependencyCheck=skip EccCheck=skip

With Jeff's patch (i.e. the PR you linked), the RegisterAcpiTableGenerator() function is mocked to save a pointer to the generator.
Once the generator is registered, it is possible to call the BuildAcpiTableEx() callback for instace:
https://github.com/jbrasen/edk2/blob/4eeeb1434820718ace1670d9429a867fe5e8c9b7/DynamicTablesPkg/Library/Acpi/Common/AcpiDbg2Lib/GoogleTest/Dbg2GeneratorGoogleTest.cpp#L354

>>
>> I guess we all need to wait for upstream's decision on STATIC and NON_STATIC exposure for ACPI table generator unit test.
>> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121255): https://edk2.groups.io/g/devel/message/121255
Mute This Topic: https://groups.io/mt/111930977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] ACPI table generator unit test

[Sophia Wang via groups.io]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

Hi Pierre,

RegisterAcpiTableGenerator() never get called in Dbg2GeneratorGoogleTest.cpp

In addition, in order to get the generator by calling RegisterAcpiTableGenerator() -> then call the BuildAcpiTableEx() callback , we need to define Dbg2Generator as non-static, however, in current upstream code, Dbg2Generator is defined as STATIC, please see the code below.

*STATIC*
CONST
ACPI_TABLE_GENERATOR  Dbg2Generator = {
// Generator ID
CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdDbg2),
// Generator Description
L"ACPI.STD.DBG2.GENERATOR",
// ACPI Table Signature
EFI_ACPI_6_3_DEBUG_PORT_2_TABLE_SIGNATURE,
// ACPI Table Revision supported by this Generator
EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION,
// Minimum supported ACPI Table Revision
EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION,
// Creator ID
TABLE_GENERATOR_CREATOR_ID,
// Creator Revision
DBG2_GENERATOR_REVISION,
// Build table function. Use the extended version instead.
NULL,
// Free table function. Use the extended version instead.
NULL,
// Extended Build table function.
BuildDbg2TableEx,
// Extended free function.
FreeDbg2TableEx
};

I doubt Dbg2GeneratorGoogleTest.cpp really tests the BuildAcpiTableEx() callback, please correct me if I am wrong.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121257): https://edk2.groups.io/g/devel/message/121257
Mute This Topic: https://groups.io/mt/111930977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 3855 bytes --]

Re: [edk2-devel] ACPI table generator unit test

[PierreGondois via groups.io]

Hello Sophia

On 4/10/25 18:46, Sophia Wang via Groups.Io wrote:
> Hi Pierre,
> RegisterAcpiTableGenerator() never get called in Dbg2GeneratorGoogleTest.cpp
> In addition, in order to get the generator by calling RegisterAcpiTableGenerator() -> then call the BuildAcpiTableEx() callback, we need to define Dbg2Generator as non-static, however, in current upstream code, Dbg2Generator is defined as STATIC, please see the code below.
> *STATIC*
> CONST
> ACPI_TABLE_GENERATOR  Dbg2Generator = {
>    // Generator ID
>    CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdDbg2),
>    // Generator Description
>    L"ACPI.STD.DBG2.GENERATOR",
>    // ACPI Table Signature
>    EFI_ACPI_6_3_DEBUG_PORT_2_TABLE_SIGNATURE,
>    // ACPI Table Revision supported by this Generator
>    EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION,
>    // Minimum supported ACPI Table Revision
>    EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION,
>    // Creator ID
>    TABLE_GENERATOR_CREATOR_ID,
>    // Creator Revision
>    DBG2_GENERATOR_REVISION,
>    // Build table function. Use the extended version instead.
>    NULL,
>    // Free table function. Use the extended version instead.
>    NULL,
>    // Extended Build table function.
>    BuildDbg2TableEx,
>    // Extended free function.
>    FreeDbg2TableEx
> };
> I doubt Dbg2GeneratorGoogleTest.cpp really tests the BuildAcpiTableEx() callback, please correct me if I am wrong.

When modifying the test file, the test fails, so I assume the Generator is registered
--- a/DynamicTablesPkg/Library/Acpi/Common/AcpiDbg2Lib/GoogleTest/Dbg2GeneratorGoogleTest.cpp
+++ b/DynamicTablesPkg/Library/Acpi/Common/AcpiDbg2Lib/GoogleTest/Dbg2GeneratorGoogleTest.cpp
@@ -577,7 +577,7 @@ protected:
      for (UINT32 i = 0; i < config.DeviceCount; i++) {
        CM_ARCH_COMMON_DBG2_DEVICE_INFO  device = { 0 };
        device.AddressResourceToken = mDevices.size () + 1;
-      device.PortType             = EFI_ACPI_DBG2_PORT_TYPE_NET;
+      device.PortType             = 999;
        device.PortSubtype          = 0;
        device.AccessSize           = EFI_ACPI_6_3_DWORD;
        CopyMem (device.ObjectName, "DBG2", sizeof (device.ObjectName) - 1);


and I think you can see the results in Build/CI_BUILDLOG.txt or Build/DynamicTablesPkg/BUILDLOG_DynamicTablesPkg.txt:

"""
INFO - [----------] 6 tests from Dbg2GeneratorAdversarialTest
INFO - [ RUN      ] Dbg2GeneratorAdversarialTest.InvalidMemoryRangeBaseAddress
INFO - DBG2: Register Generator. Status = Success
INFO - INFO: GetEArchCommonObjSerialDebugPortInfo: Platform does not implement EArchCommonObjSerialDebugPortInfo. Status = Not Found
INFO - ERROR: DBG2: Memory range base address is 0. Index = 0
INFO - ERROR: DBG2: Failed to populate device 0. Status = Invalid Parameter
INFO - DBG2: Deregister Generator. Status = Success
INFO - [       OK ] Dbg2GeneratorAdversarialTest.InvalidMemoryRangeBaseAddress (0 ms)
INFO - [ RUN      ] Dbg2GeneratorAdversarialTest.MemoryRangeTooLarge
INFO - DBG2: Register Generator. Status = Success
INFO - INFO: GetEArchCommonObjSerialDebugPortInfo: Platform does not implement EArchCommonObjSerialDebugPortInfo. Status = Not Found
INFO - ERROR: DBG2: Memory range length too large. Length = 0
INFO - ERROR: DBG2: Failed to populate device 0. Status = Invalid Parameter
INFO - DBG2: Deregister Generator. Status = Success
"""


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#121258): https://edk2.groups.io/g/devel/message/121258
Mute This Topic: https://groups.io/mt/111930977/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-