1. I see that MdePkg adds a dependency on UnitTestPkg. This makes
UnitTestPkg the root package when building and running host based unit tests.
This makes sense, but good to highlight that all packages that use host based
tests will introduce a new package dependency on UnitTestPkg.
* Since the dependency only applies to unit tests, can we update the
DependencyCheck plugin to support listing dependencies for FW components
separate from dependencies for unit tests?
* Can move. Capability is there, but mistakenly added to the wrong
section.
2. I see UnitTestPkg declares 6 new lib classes. Are all 6 classes intended
to be used directly from a unit test case? If some of these are only intended
to be used from the framework inside the UnitTestPkg can we make them private?
* Because there are different instances in multiple places (and
conceivably more in the future), we don’t see how we could make them private.
3. In the MdePkg, I see a new top level directory called ‘HostLibrary’.
Since these lib instances are only used from a host based test, can they be
moved into the ‘Test’ directory?
* Can move.
4. MdePkg/MdePkgTest.dsc.
* Can this DSC file be moved into the ‘Test’ directory?
i. Yes
* I see this DSC file only supports VS today. How much work is it to
add GCC support?
i. Don’t know. This
is an item on our list, but lower priority and not a blocker.
1. MdePkg/HostLibrary/BaseLibHost
* Why are there 2 INFs. One with ASM and one without ASM? Can we just
use the one with ASM and assume NASM is installed?
* I see the purpose of this lib instance is to call the
* SetJump()/LongJump(). So these implementations always work? It looks
like it assumes BASE_LIBRARY_JUMP_BUFFER is identical to the host OS user mode
application setjmp()/longjmp() state.
* Why are DRx and CRx registers emulated? I would think and code that
depends on read/writing these registers would not be compatible with host based
testing. Can we change to ASSERT (FALSE)?
* PatchInstructionX86() – I suspect this will not work in a host based
environment because it is self modifying code. Should it be ASSERT (FALSE)?
* Libraries were taken directly from the Intel work in HBFA. They worked
so we kept them. We’re just as interested in the answers to these questions as
you are.
2. MdePkg/HostLibrary/DebugLibHost
* What is ‘#ifndef TEST_WITH_KLEE’
* What is the ‘PatchFormat()’ function? It is always disabled with if
(0).
* Are the PCDs to set the debug message levels disabled on purpose?
(DebugPrintEnabled(), DebugPrintLevelEnabled(), DebugCodeEnabled())
* Libraries were taken directly from the Intel work in HBFA. They worked
so we kept them. We’re just as interested in the answers to these questions as
you are.
3. MdePkg/HostLibrary/BaseMemoryLibHost
* Why can’t we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf
instead and reduce the number of host specific lib instances?
* Libraries were taken directly from the Intel work in HBFA. They worked
so we kept them. We’re just as interested in the answers to these questions as
you are.
4. MdePkg/HostLibrary/MemoryAllocationLibHost
* Why is are memcpy(), assert(), memset() used in this lib? I would
expect this lib instance to match the UefiMemoryAllocationLib with the only the
use of malloc() and free() to replace the UEFI Boot Services calls.
* Libraries were taken directly from the Intel work in HBFA. They worked
so we kept them. We’re just as interested in the answers to these questions as
you are.
5. Host library instance naming conventions.
* The current naming convention uses the environment as a prefix(e.g.
Pei, Smm, Uefi) and the services the lib instance uses as a post fix. Would it
make more sense to use ‘Host’ as a prefix instead of a postfix in the lib
instance names?
* I don’t know if there’s a 1:1 relationship with these. For some
library classes (that you might have host versions of), the class itself is the
PeiSomethingLib, and the host version would be the PeiSomethingLibHost. No?
6. Unicode vs ASCII strings
* I see InitUnitTestFramework(), CreateUnitTestSuite(), and
AddTestCase() all take Unicode strings for the labels. I also see extra code
to convert gEfiCallerBaseName from ASCII to Unicode to use it as a short name
of a test framework. I think it would be simpler if the parameters to these
APIs were ASCII and the framework can convert to Unicode if needed.
* No strong feelings, but we already have a bunch of tests written this
way. Since the UnitTestLib is an abstraction that works as well in Shell as in
the Host, going with Unicode strings seemed to match the art for Shell apps
(since the framework was written for Shell first).
7. Will InitUnitTestFramework() and CreateUnitTestSuite() always be called
in pairs? If so, can we combine these to a single API?
* I see the SafeIntLib example create a single framework and multiple
test suites. Perhaps we can have a single CreateUnitTestSuite() API where
Framework is an IN/OUT and if it is passed in as NULL, the Framework handle is
created.
i. It’s not always
in pairs. You would only have a single framework init, but could have multiple
suites.
* I see a pattern where the CreateUnitTestSuite() ‘Package’ parameter is
used as a prefix to every call to AddTestCase() in the ‘ClassName’ parameter.
Can we simplify AddTestCase() so it only need to pass in the name of the unit
test case, and the framework can append Package and the unit test case name?
i. Right now, our
tests just coincidentally share similar names with the packages, but we don’t
feel this is axiomatic and don’t want to force this naming on others, who may
be trying to bolt into other reporting structures.
1. I see the use of the ‘Fw’ variable as a shorthand for ‘Framework’. Can
we use something other than ‘Fw’. It may be confused with ‘Firmware’.
* No real arguments. Wouldn’t fight a find-replace, but it’ll just be a
bunch of touches as we commit.
2. UnitTestPkg/Include/UnitTestTypes.h
* I see several hard coded string lengths. Since this runs in a host
environment and strings can always be allocated, can the hard coded lengths be
removed? Update the structs to use pointers to strings instead of string
arrays, and the framework can manage alloc() and free()?
i. Given that this
framework is designed to be nimble enough to work in PEI and SMM and other
constrictive environments, we wanted to keep fixed sizing.
* How are Fingerprints used? The idea of using as hash as a unique
identifier is a good idea. How is the hash calculated? What unit test code
artifacts are used so developers know what parameters must be unique? Can we
just decide to use a specific hash algorithm/size and the structure can be a
pointer to an allocated buffer instead of a fixed size array to make it easy to
change the algorithm/size in the future?
i. Fingerprints are
unique IDs to make sure that serialize/unserialized state matches the tests
upon re-entry. I’m not married to MD5, but it needs to be predictably unique,
and carried with the framework. I would not want to make any requirements on
CryptoLib, since these aren’t crypto-strong.
* Update all the strings to be ASCII? See Unicode vs ASCII above.
i. Ideally not,
unless there’s a strong use case.
* UNIT_TEST_SAVE_TEST – The last field is a variable sized array, so it
can be a formal field.
i. Not opposed, but
don’t really want to make the change myself. Is there a style guide that this
is breaking?
* UNIT_TEST_SAVE_CONTEXT - – The last field is a variable sized array,
so it can be a formal field.
* UNIT_TEST_SAVE_HEADER – Can the last 3 fields be changed to pointers
and be formal fields?
* Do the structures really need to be packed? Specially with the
changes suggested above? Is the intent to potentially share data stored on
disk between different host execution environments that may have different
width architectures?
i. That’s an
interesting point. Could you draw up your suggestion and submit a PR?
1. UnitTestPkg – UnitTestLib.h
* Can you provide more context for the APIs SaveFrameworkState(),
SaveFrameworkStateAndQuit(), SaveFrameworkStateAndReboot(),
SetFrameworkBootNextDevice(). I do not see any Load() functions, so how would
a set of tests be resumed? If these do not apply to host based tests, should
these be split out to a different lib class?
i. I’ll improve the
documentation around these functions. I will acknowledge, however, that this is
an interface that I expect to evolve as we figure out what kinds of tests the
community wants support for “out of the box”. If we want to easily enable tests
around – for example – ExitBootServices, we will likely want to tweak this
going forward to its simplest form. The version we have here was sufficient to
enable the test cases that we’ve currently written.
1. UnitTestPkg/Library/UnitTestLib
* I see an implementation of MD5. We should not do our own. We should
use an approved implementation such as OpenSSL.
i. Happy to discuss
another implementation, but this is not crypto-strong. It’s just for
uniqueness. A sufficiently long CRC would probably work, too.
1. UnitTestPkg/Library/UnitTestTerminationLibTbd
* Do we really need this lib instance now?
i. This is here so
that we can figure out what this should look like for a host. Host obviously
wouldn’t reset, but could conceivably quit. Or maybe that doesn’t make any
sense for a host.
- Bret
________________________________
From: [email protected] <[email protected]> on behalf of Bret Barkelew
via Groups.Io <[email protected]>
Sent: Wednesday, December 4, 2019 10:24:21 AM
To: Andrew Fish <[email protected]>; [email protected] <[email protected]>;
Kinney, Michael D <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now
with docs!)
Andrew,
I agree with your points.
Mike,
You’ve got a lot more there. Let me take a look and update the draft. I’ll ping
back ASAP.
- Bret
From: Andrew Fish<mailto:[email protected]>
Sent: Wednesday, December 4, 2019 9:50 AM
To: [email protected]<mailto:[email protected]>; Kinney, Michael
D<mailto:[email protected]>
Cc: Bret Barkelew<mailto:[email protected]>;
[email protected]<mailto:[email protected]>
Subject: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with
docs!)
Mike,
I like and agree with your comments.
On the UnitTestPkg(s) dependency issue I think it would make sense to move as
much as possible into the *Pkg/Test/ ( *Pkg/HostLibrary,
MdePkg/MdePkgTest.dsc, etc.) directory structure. It looks like for
implementation specific tests we skip the Test directory and drop the UnitTest
or FuzzTest directly into modules directory. Maybe we should follow the same
pattern as *Pkg/Test and have a Test directory? This will help minimize the
number of "reserved" directories we need for managing the testing. Have a
standardized directory structure will make it easier to differentiate the code
from tests while doing `git grep` or other forms of code spelunking.
A Host-Based Unit Test for a Library makes sense as you can link directly
against the library and test it. A Host-Based Unit Test for Protocol/PPI/GUID
[1] seems a little more complex. It is easy enough to write tests for the
interfaces but what APIs do you call to get access to the protocol?
Per our conversation at the Stewards meeting I think make sense to try to roll
out the testing of libraries as the 1st phase. The mocking required for drivers
could get quite complex and let us not get bogged down in all that to get
something working.
[1] *Pkg/Test/UnitTest/[Library|Protocol|Ppi|Guid]
Thanks,
Andrew Fish
On Dec 2, 2019, at 3:12 PM, Michael D Kinney
<[email protected]<mailto:[email protected]>> wrote:
Hi Bret,
Thanks for posting this content. Host based unit testing is a very valuable
addition to the CI checks.
I have the following comments:
1. I see that MdePkg adds a dependency on UnitTestPkg. This makes
UnitTestPkg the root package when building and running host based unit tests.
This makes sense, but good to highlight that all packages that use host based
tests will introduce a new package dependency on UnitTestPkg.
* Since the dependency only applies to unit tests, can we update the
DependencyCheck plugin to support listing dependencies for FW components
separate from dependencies for unit tests?
1. I see UnitTestPkg declares 6 new lib classes. Are all 6 classes intended
to be used directly from a unit test case? If some of these are only intended
to be used from the framework inside the UnitTestPkg can we make them private?
2. In the MdePkg, I see a new top level directory called ‘HostLibrary’.
Since these lib instances are only used from a host based test, can they be
moved into the ‘Test’ directory?
3. MdePkg/MdePkgTest.dsc.
* Can this DSC file be moved into the ‘Test’ directory?
* I see this DSC file only supports VS today. How much work is it to
add GCC support?
1. MdePkg/HostLibrary/BaseLibHost
* Why are there 2 INFs. One with ASM and one without ASM? Can we just
use the one with ASM and assume NASM is installed?
* I see the purpose of this lib instance is to call the
* SetJump()/LongJump(). So these implementations always work? It looks
like it assumes BASE_LIBRARY_JUMP_BUFFER is identical to the host OS user mode
applicationsetjmp()/longjmp() state.
* Why are DRx and CRx registers emulated? I would think and code that
depends on read/writing these registers would not be compatible with host based
testing. Can we change to ASSERT (FALSE)?
* PatchInstructionX86() – I suspect this will not work in a host based
environment because it is self modifying code. Should it be ASSERT (FALSE)?
1. MdePkg/HostLibrary/DebugLibHost
* What is ‘#ifndef TEST_WITH_KLEE’
* What is the ‘PatchFormat()’ function? It is always disabled with if
(0).
* Are the PCDs to set the debug message levels disabled on purpose?
(DebugPrintEnabled(), DebugPrintLevelEnabled(), DebugCodeEnabled())
1. MdePkg/HostLibrary/BaseMemoryLibHost
* Why can’t we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf
instead and reduce the number of host specific lib instances?
1. MdePkg/HostLibrary/MemoryAllocationLibHost
* Why is are memcpy(), assert(), memset() used in this lib? I would
expect this lib instance to match the UefiMemoryAllocationLib with the only the
use of malloc() and free() to replace the UEFI Boot Services calls.
1. Host library instance naming conventions.
* The current naming convention uses the environment as a prefix(e.g.
Pei, Smm, Uefi) and the services the lib instance uses as a post fix. Would it
make more sense to use ‘Host’ as a prefix instead of a postfix in the lib
instance names?
1. Unicode vs ASCII strings
* I see InitUnitTestFramework(), CreateUnitTestSuite(), and
AddTestCase() all take Unicode strings for the labels. I also see extra code
to convert gEfiCallerBaseName from ASCII to Unicode to use it as a short name
of a test framework. I think it would be simpler if the parameters to these
APIs were ASCII and the framework can convert to Unicode if needed.
1. Will InitUnitTestFramework() and CreateUnitTestSuite() always be called
in pairs? If so, can we combine these to a single API?
* I see the SafeIntLib example create a single framework and multiple
test suites. Perhaps we can have a single CreateUnitTestSuite() API where
Framework is an IN/OUT and if it is passed in as NULL, the Framework handle is
created.
* I see a pattern where the CreateUnitTestSuite() ‘Package’ parameter is
used as a prefix to every call to AddTestCase() in the ‘ClassName’ parameter.
Can we simplify AddTestCase() so it only need to pass in the name of the unit
test case, and the framework can append Package and the unit test case name?
1. I see the use of the ‘Fw’ variable as a shorthand for ‘Framework’. Can
we use something other than ‘Fw’. It may be confused with ‘Firmware’.
2. UnitTestPkg/Include/UnitTestTypes.h
* I see several hard coded string lengths. Since this runs in a host
environment and strings can always be allocated, can the hard coded lengths be
removed? Update the structs to use pointers to strings instead of string
arrays, and the framework can manage alloc() and free()?
* How are Fingerprints used? The idea of using as hash as a unique
identifier is a good idea. How is the hash calculated? What unit test code
artifacts are used so developers know what parameters must be unique? Can we
just decide to use a specific hash algorithm/size and the structure can be a
pointer to an allocated buffer instead of a fixed size array to make it easy to
change the algorithm/size in the future?
* Update all the strings to be ASCII? See Unicode vs ASCII above.
* UNIT_TEST_SAVE_TEST – The last field is a variable sized array, so it
can be a formal field.
* UNIT_TEST_SAVE_CONTEXT - – The last field is a variable sized array,
so it can be a formal field.
* UNIT_TEST_SAVE_HEADER – Can the last 3 fields be changed to pointers
and be formal fields?
* Do the structures really need to be packed? Specially with the
changes suggested above? Is the intent to potentially share data stored on
disk between different host execution environments that may have different
width architectures?
1. UnitTestPkg – UnitTestLib.h
* Can you provide more context for the APIs SaveFrameworkState(),
SaveFrameworkStateAndQuit(), SaveFrameworkStateAndReboot(),
SetFrameworkBootNextDevice(). I do not see any Load() functions, so how would
a set of tests be resumed? If these do not apply to host based tests, should
these be split out to a different lib class?
1. UnitTestPkg/Library/UnitTestLib
* I see an implementation of MD5. We should not do our own. We should
use an approved implementation such as OpenSSL.
1. UnitTestPkg/Library/UnitTestTerminationLibTbd
* Do we really need this lib instance now?
Thanks,
Mike
From: [email protected]<mailto:[email protected]>
<[email protected]<mailto:[email protected]>> On Behalf Of Bret Barkelew
via Groups.Io
Sent: Thursday, November 21, 2019 11:39 PM
To: [email protected]<mailto:[email protected]>
Subject: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)
Now that CI has landed in edk2/master, we'd like to get the basic framework for
unittesting staged as well. Target intercept date would be immediately after
the 2019/11 stabilization, so we wanted to go ahead and get comments now.
The host unittest framework consists of five primary pieces:
- The test library (Cmocka)
- Support libraries (Found in UnitTestPkg)
- The test support plugins (HostUnitTestComilerPlugin,
HostUnitTestDxeCompleteCheck, HostBasedUnitTestRunner)
- The configuration in the package-based *.ci.yaml file and package-based
Test.dsc
- The tests themselves
We have a demo branch set up at:
https://github.com/corthon/edk2-staging/tree/edk2-host-test_v2<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2-staging%2Ftree%2Fedk2-host-test_v2&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C6eac9932f3f640dd65ac08d778e731e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110806683189828&sdata=JQ%2BoWxlEBOK2sH55cRAPVa3OpAxTsm6eHcdbWSo9CVo%3D&reserved=0>
We also have a demo build pipeline at:
https://dev.azure.com/tianocore/edk2-ci-play/_build?definitionId=36&_a=summary<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci-play%2F_build%3FdefinitionId%3D36%26_a%3Dsummary&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C6eac9932f3f640dd65ac08d778e731e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110806683189828&sdata=wBwn1ehjyTmYNKVSvlEZSXK5qyeu4EPAL7FzdYntnt4%3D&reserved=0>
The current demo branch contains a single test in MdePkg for SafeIntLib (module
file MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.inf). This
test is automatically detected by the HostUnitTestComilerPlugin and run by the
HostBasedUnitTestRunner as part of the CI process.
A few notes about the current demo:
1) The demo currently only works on Windows build chains, but there's no reason
to believe that it can't work equally well on Linux build chains, and are happy
to work with anyone to get it there.
2) The demo currently has four failing conditions that can be seen towards the
end of MdePkg "Build and Test" log file for this build:
https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=2813<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci-play%2F_build%2Fresults%3FbuildId%3D2590&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C6eac9932f3f640dd65ac08d778e731e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110806683199782&sdata=1zd2oq8zRZUn3%2FUiGDuJZEZ5M0srtc2bqoa0%2BLbZB3s%3D&reserved=0>
"WARNING - Test SafeInt16ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:302:
error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeInt32ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:638:
error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeIntnToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:1051:
error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeInt64ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:1456:
error: Failure!"
These failures seem to be legitimate and further work should be done by the
community to decide whether the test case is correct or the library is correct,
but one of them needs to change.
3) Current demo pulls in test collateral from a fork of the edk2-test repo.
This repo is currently *very* heavy due to the history of the UEFI SCT project
and the number of binaries that it pulls down. It's possible that we (the
community) need to select a better place for this code to live. Maybe in edk2
primary (though it's not explicitly firmware code, so it seems unnecessary).
Maybe in a new edk2-test2 repo or something like that.
There’s an RFC doc here:
https://github.com/corthon/edk2-staging/blob/edk2-host-test_v2/Readme-RFC.md<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2-staging%2Fblob%2Fedk2-host-test_v2%2FReadme-RFC.md&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C6eac9932f3f640dd65ac08d778e731e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110806683199782&sdata=1nq1mHjsbSZj4IQM5RBwSrvaQxO5cmDlTvf7VYNDV%2FA%3D&reserved=0>
And a usage guide here:
https://github.com/corthon/edk2-staging/blob/edk2-host-test_v2/UnitTestPkg/ReadMe.md<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2-staging%2Fblob%2Fedk2-host-test_v2%2FUnitTestPkg%2FReadMe.md&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C6eac9932f3f640dd65ac08d778e731e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110806683209750&sdata=0jRQ2Rzr9PWSYJ1YDs7l2aFS3PfAnTbTousYYe8IWTw%3D&reserved=0>
Once again, would love to get this into EDK2 master after stabilization, and
most of this has previously been shopped around in other discussion threads.
This is just where the rubber meets the road and is the minimal subset of code
that needs to land for folks to start contributing unittests against the core
libraries that can be run as part of any CI pass.
Thanks!
- Bret
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#52018): https://edk2.groups.io/g/devel/message/52018
Mute This Topic: https://groups.io/mt/67560953/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
