From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web12.13164.1576533815749920738 for ; Mon, 16 Dec 2019 14:03:36 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: jiewen.yao@intel.com) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Dec 2019 14:03:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,323,1571727600"; d="png'150?scan'150,208,217,150";a="240187076" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga004.fm.intel.com with ESMTP; 16 Dec 2019 14:03:33 -0800 Received: from fmsmsx114.amr.corp.intel.com (10.18.116.8) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 16 Dec 2019 14:03:33 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX114.amr.corp.intel.com (10.18.116.8) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 16 Dec 2019 14:03:32 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.109]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.29]) with mapi id 14.03.0439.000; Tue, 17 Dec 2019 06:03:31 +0800 From: "Yao, Jiewen" To: "Kinney, Michael D" , "devel@edk2.groups.io" , "bret.barkelew@microsoft.com" , Andrew Fish CC: "rfc@edk2.groups.io" Subject: Re: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!) Thread-Topic: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!) Thread-Index: AQHVoQS12ZW6E6MMrkasWCUh3oP5e6enceDwgALivYCAAAlHOoADZyKbgAsoujaAAYecgIACcgEAgACPPeA= Date: Mon, 16 Dec 2019 22:03:30 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503F8A361C@shsmsx102.ccr.corp.intel.com> References: ,<0F460E19-2A3B-4FEE-8B6F-BE65B9C5F1D4@apple.com>,<15DD3E3A746110DF.27746@groups.io>, <74D8A39837DF1E4DA445A8C0B3885C503F8A1CEB@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNmZkZWMzZjAtMTAzYS00ZjAzLWI1MmItOTg2ODY5OGMyZjhjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoidkxVdzE3Tk9QK3hITDc0cndmbTMzbkZrc0dFQVZlTU1FNitcL0V2N3BaVFk2MUtJWEhIUHlDb3RlUVNEMzhLTUcifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=True;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2019-12-04T18:23:45.2427147Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Privileged x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jiewen.yao@intel.com X-Groupsio-MsgNum: 52262 Content-Language: en-US Content-Type: multipart/related; boundary="_004_74D8A39837DF1E4DA445A8C0B3885C503F8A361Cshsmsx102ccrcor_"; type="multipart/alternative" --_004_74D8A39837DF1E4DA445A8C0B3885C503F8A361Cshsmsx102ccrcor_ Content-Type: multipart/alternative; boundary="_000_74D8A39837DF1E4DA445A8C0B3885C503F8A361Cshsmsx102ccrcor_" --_000_74D8A39837DF1E4DA445A8C0B3885C503F8A361Cshsmsx102ccrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable No problem for me. Thank you Yao Jiewen From: Kinney, Michael D Sent: Tuesday, December 17, 2019 5:28 AM To: Yao, Jiewen ; devel@edk2.groups.io; bret.barkele= w@microsoft.com; Andrew Fish ; Kinney, Michael D Cc: rfc@edk2.groups.io Subject: RE: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (No= w with docs!) Hi Jiewen, Many of your responses are related to fuzz testing. Can we focus current infrastructure on host based unit testing and minimiz= ing the content to support that testing type. Then, add a new subdirectory below Test in the future to support fuzz test= ing. This way, lib instances can be tuned to the test infrastructure/subsy= stem in use. Trying to make one version of the host libs for multiple test= methodologies it making is more complex and hander to maintain. Thanks, Mike From: Yao, Jiewen > Sent: Saturday, December 14, 2019 4:38 PM To: devel@edk2.groups.io; bret.barkelew@micro= soft.com; Andrew Fish >; Kinney, Michael D > Cc: rfc@edk2.groups.io Subject: RE: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (No= w with docs!) Answer the question 5,6,7,8, which are related to HBFA. For other MSFT unit test, I don't have strong opinion. I am OK with that. Thank you Yao Jiewen From: devel@edk2.groups.io > On Behalf Of Bret Barkelew via Groups.Io Sent: Saturday, December 14, 2019 8:47 AM To: devel@edk2.groups.io; Andrew Fish >; Kinney, Michael D > Cc: rfc@edk2.groups.io Subject: Re: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (No= w with docs!) Mike, I think I've gotten all the feedback here and all the action items from ou= r call. The current branch should be good to go, minus the couple of things= that Intel was going to look into. Thanks! - Bret From: Bret Barkelew Sent: Friday, December 6, 2019 2:21 PM To: devel@edk2.groups.io; Andrew Fish; Kinney, Michael D Cc: rfc@edk2.groups.io Subject: RE: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (No= w with docs!) 1. I see that MdePkg adds a dependency on UnitTestPkg. This makes Unit= TestPkg the root package when building and running host based unit tests. = This makes sense, but good to highlight that all packages that use host bas= ed tests will introduce a new package dependency on UnitTestPkg. * Since the dependency only applies to unit tests, can we update th= e 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. 1. I see UnitTestPkg declares 6 new lib classes. Are all 6 classes int= ended 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 con= ceivably more in the future), we don't see how we could make them private. 1. 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 b= e moved into the 'Test' directory? * Can move. 1. 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 kno= w. 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 t= hat depends on read/writing these registers would not be compatible with ho= st based testing. Can we change to ASSERT (FALSE)? * PatchInstructionX86() - I suspect this will not work in a host ba= sed environment because it is self modifying code. Should it be ASSERT (FA= LSE)? * Libraries were taken directly from the Intel work in HBFA. They w= orked so we kept them. We're just as interested in the answers to these que= stions as you are. [Jiewen] Here is the answer: Some tools requires source build, such AFL-CC or KLEE to get the result or= optimization. They do analysis besides compiling. These tools only work on= C-language. That why we need non-asm build. Some test case need access CRx/DRx just as the pre-condition and post-cond= ition, such as MTRR test. We cannot use ASSERT. Generally, a big problem of current based lib is that it mixes the hardwar= e register access (such as CR/DR, CPUIR, MSR, mode switch) into a common so= ftware lib (such as linked list, math, string, checksum). I hope we can ref= actory the based lib to separate the hardware access part, such as a BaseRe= gAccessLib, similar to IoLib, PciLib. Agree on PatchInstructionX86(). We should use ASSERT(). If we encounter a = case that requires this function, we consider how to make it work later. 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()) * Libraries were taken directly from the Intel work in HBFA. They w= orked so we kept them. We're just as interested in the answers to these que= stions as you are. [Jiewen] Here is the answer: This is special optimization for KLEE - a symbolic executio= n tool. We hope to reduce the unnecessary branch. Or the KELL might not res= olve the function. PatchFormat() is an old function. Deadcode can be removed n= ow. In the beginning, PCD is not supported. And we don't find a= use case to control PCD in such detail level. To simplify thing, we just d= on't use it. An on/off control (via HOST_DEBUG_MESSAGE) is good enough. If = there is real use case coming up, we can see how to enable it. 1. MdePkg/HostLibrary/BaseMemoryLibHost * Why can't we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf i= nstead and reduce the number of host specific lib instances? * Libraries were taken directly from the Intel work in HBFA. They w= orked so we kept them. We're just as interested in the answers to these que= stions as you are. [Jiewen] Here is the answer: This is special optimization for fuzzing tool. If we use st= andard C lib, the compiler knows it and don't insert branch analysis. If we= use our own implementation, the compiler has no idea, and try to do analys= is. That is unnecessary. 1. MdePkg/HostLibrary/MemoryAllocationLibHost * Why is are memcpy(), assert(), memset() used in this lib? I woul= d expect this lib instance to match the UefiMemoryAllocationLib with the on= ly the use of malloc() and free() to replace the UEFI Boot Services calls. * Libraries were taken directly from the Intel work in HBFA. They w= orked so we kept them. We're just as interested in the answers to these que= stions as you are. [Jiewen] Here is the answer: Similar to 7, using standard-C function can reduce the unne= cessary analysis for analysis tool. 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. W= ould it make more sense to use 'Host' as a prefix instead of a postfix in t= he lib instance names? * I don't know if there's a 1:1 relationship with these. For some l= ibrary classes (that you might have host versions of), the class itself is = the PeiSomethingLib, and the host version would be the PeiSomethingLibHost.= No? 1. Unicode vs ASCII strings * I see InitUnitTestFramework(), CreateUnitTestSuite(), and AddTest= Case() all take Unicode strings for the labels. I also see extra code to c= onvert gEfiCallerBaseName from ASCII to Unicode to use it as a short name o= f 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 She= ll as in the Host, going with Unicode strings seemed to match the art for S= hell apps (since the framework was written for Shell first). 1. Will InitUnitTestFramework() and CreateUnitTestSuite() always be cal= led in pairs? If so, can we combine these to a single API? * I see the SafeIntLib example create a single framework and multip= le test suites. Perhaps we can have a single CreateUnitTestSuite() API whe= re Framework is an IN/OUT and if it is passed in as NULL, the Framework han= dle is created. i. It's not = always in pairs. You would only have a single framework init, but could hav= e multiple suites. * I see a pattern where the CreateUnitTestSuite() 'Package' paramet= er is used as a prefix to every call to AddTestCase() in the 'ClassName' pa= rameter. 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 othe= rs, 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. 1. UnitTestPkg/Include/UnitTestTypes.h * I see several hard coded string lengths. Since this runs in a ho= st environment and strings can always be allocated, can the hard coded leng= ths be removed? Update the structs to use pointers to strings instead of s= tring arrays, and the framework can manage alloc() and free()? i. Given tha= t 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 co= de artifacts are used so developers know what parameters must be unique? C= an we just decide to use a specific hash algorithm/size and the structure c= an be a pointer to an allocated buffer instead of a fixed size array to mak= e it easy to change the algorithm/size in the future? i. Fingerpri= nts are unique IDs to make sure that serialize/unserialized state matches t= he tests upon re-entry. I'm not married to MD5, but it needs to be predicta= bly unique, and carried with the framework. I would not want to make any re= quirements on CryptoLib, since these aren't crypto-strong. * Update all the strings to be ASCII? See Unicode vs ASCII above. i. Ideally n= ot, unless there's a strong use case. * UNIT_TEST_SAVE_TEST - The last field is a variable sized array, s= o it can be a formal field. i. Not oppos= ed, 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 arr= ay, so it can be a formal field. * UNIT_TEST_SAVE_HEADER - Can the last 3 fields be changed to point= ers and be formal fields? * Do the structures really need to be packed? Specially with the c= hanges 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(), S= aveFrameworkStateAndQuit(), SaveFrameworkStateAndReboot(), SetFrameworkBoot= NextDevice(). I do not see any Load() functions, so how would a set of tes= ts be resumed? If these do not apply to host based tests, should these be = split out to a different lib class? i. I'll impr= ove the documentation around these functions. I will acknowledge, however, = that this is an interface that I expect to evolve as we figure out what kin= ds of tests the community wants support for "out of the box". If we want to= easily enable tests around - for example - ExitBootServices, we will likel= y want to tweak this going forward to its simplest form. The version we hav= e 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 sh= ould use an approved implementation such as OpenSSL. i. Happy to = discuss another implementation, but this is not crypto-strong. It's just fo= r uniqueness. A sufficiently long CRC would probably work, too. 1. UnitTestPkg/Library/UnitTestTerminationLibTbd * Do we really need this lib instance now? i. This is h= ere so that we can figure out what this should look like for a host. Host o= bviously wouldn't reset, but could conceivably quit. Or maybe that doesn't = make any sense for a host. - Bret From: devel@edk2.groups.io > on behalf of Bret Barkelew via Groups.Io = > Sent: Wednesday, December 4, 2019 10:24:21 AM To: Andrew Fish >; devel@edk2.grou= ps.io >; Kinney, Michael D > Cc: rfc@edk2.groups.io > Subject: Re: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (No= w 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 Sent: Wednesday, December 4, 2019 9:50 AM To: devel@edk2.groups.io; Kinney, Michael D Cc: Bret Barkelew; rfc@edk2.groups.io<= mailto:rfc@edk2.groups.io> Subject: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now wi= th 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/MdePkg= Test.dsc, etc.) directory structure. It looks like for implementation speci= fic tests we skip the Test directory and drop the UnitTest or FuzzTest dire= ctly into modules directory. Maybe we should follow the same pattern as *Pk= g/Test and have a Test directory? This will help minimize the number of "re= served" directories we need for managing the testing. Have a standardized d= irectory 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/GU= ID [1] seems a little more complex. It is easy enough to write tests for th= e 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 fo= r drivers could get quite complex and let us not get bogged down in all tha= t 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 > wrote: Hi Bret, Thanks for posting this content. Host based unit testing is a very valuab= le addition to the CI checks. I have the following comments: 1. I see that MdePkg adds a dependency on UnitTestPkg. This makes Unit= TestPkg the root package when building and running host based unit tests. = This makes sense, but good to highlight that all packages that use host bas= ed tests will introduce a new package dependency on UnitTestPkg. * Since the dependency only applies to unit tests, can we update th= e 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 int= ended 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 b= e 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 t= hat depends on read/writing these registers would not be compatible with ho= st based testing. Can we change to ASSERT (FALSE)? * PatchInstructionX86() - I suspect this will not work in a host ba= sed environment because it is self modifying code. Should it be ASSERT (FA= LSE)? 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 i= nstead and reduce the number of host specific lib instances? 1. MdePkg/HostLibrary/MemoryAllocationLibHost * Why is are memcpy(), assert(), memset() used in this lib? I woul= d expect this lib instance to match the UefiMemoryAllocationLib with the on= ly 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. W= ould it make more sense to use 'Host' as a prefix instead of a postfix in t= he lib instance names? 1. Unicode vs ASCII strings * I see InitUnitTestFramework(), CreateUnitTestSuite(), and AddTest= Case() all take Unicode strings for the labels. I also see extra code to c= onvert gEfiCallerBaseName from ASCII to Unicode to use it as a short name o= f 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 cal= led in pairs? If so, can we combine these to a single API? * I see the SafeIntLib example create a single framework and multip= le test suites. Perhaps we can have a single CreateUnitTestSuite() API whe= re Framework is an IN/OUT and if it is passed in as NULL, the Framework han= dle is created. * I see a pattern where the CreateUnitTestSuite() 'Package' paramet= er is used as a prefix to every call to AddTestCase() in the 'ClassName' pa= rameter. 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 ho= st environment and strings can always be allocated, can the hard coded leng= ths be removed? Update the structs to use pointers to strings instead of s= tring 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 co= de artifacts are used so developers know what parameters must be unique? C= an we just decide to use a specific hash algorithm/size and the structure c= an be a pointer to an allocated buffer instead of a fixed size array to mak= e 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, s= o it can be a formal field. * UNIT_TEST_SAVE_CONTEXT - - The last field is a variable sized arr= ay, so it can be a formal field. * UNIT_TEST_SAVE_HEADER - Can the last 3 fields be changed to point= ers and be formal fields? * Do the structures really need to be packed? Specially with the c= hanges 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(), S= aveFrameworkStateAndQuit(), SaveFrameworkStateAndReboot(), SetFrameworkBoot= NextDevice(). I do not see any Load() functions, so how would a set of tes= ts 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 sh= ould use an approved implementation such as OpenSSL. 1. UnitTestPkg/Library/UnitTestTerminationLibTbd * Do we really need this lib instance now? Thanks, Mike From: devel@edk2.groups.io > On Behalf Of Bret Barkelew via Groups.Io Sent: Thursday, November 21, 2019 11:39 PM To: devel@edk2.groups.io 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 framewor= k for unittesting staged as well. Target intercept date would be immediatel= y after the 2019/11 stabilization, so we wanted to go ahead and get comment= s now. The host unittest framework consists of five primary pieces: - The test library (Cmocka) - Support libraries (Found in UnitTestPkg) - The test support plugins (HostUnitTestComilerPlugin, HostUnitTestDxeComp= leteCheck, 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 We also have a demo build pipeline at: https://dev.azure.com/tianocore/edk2-ci-play/_build?definitionId=3D36&_a= =3Dsummary The current demo branch contains a single test in MdePkg for SafeIntLib (m= odule file MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.i= nf). This test is automatically detected by the HostUnitTestComilerPlugin a= nd 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 r= eason 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=3D2813= "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 th= e community to decide whether the test case is correct or the library is co= rrect, 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 pr= oject 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 unne= cessary). 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 And a usage guide here: https://github.com/corthon/edk2-staging/blob/edk2-= host-test_v2/UnitTestPkg/ReadMe.md Once again, would love to get this into EDK2 master after stabilization, a= nd most of this has previously been shopped around in other discussion thre= ads. 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 again= st the core libraries that can be run as part of any CI pass. Thanks! - Bret --_000_74D8A39837DF1E4DA445A8C0B3885C503F8A361Cshsmsx102ccrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

No problem for me.

 

Thank you

Yao Jiewen

 

From: Ki= nney, Michael D <michael.d.kinney@intel.com>
Sent: Tuesday, December 17, 2019 5:28 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;= bret.barkelew@microsoft.com; Andrew Fish <afish@apple.com>; Kinney, = Michael D <michael.d.kinney@intel.com>
Cc: rfc@edk2.groups.io
Subject: RE: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test = RFC (Now with docs!)

 

Hi Jiewen,

 

Many of your responses are related to fuzz testing.=

 

Can we focus current infrastructure on host based u= nit testing and minimizing the content to support that testing type.

 

Then, add a new subdirectory below Test in the futu= re to support fuzz testing.  This way, lib instances can be tuned to t= he test infrastructure/subsystem in use.  Trying to make one version o= f the host libs for multiple test methodologies it making is more complex and hander to maintain.

 

Thanks,

 

Mike

 

From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Saturday, December 14, 2019 4:38 PM
To: devel@edk2.groups.io; bret.barkelew@microsoft.com; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: rfc@edk2.groups.io Subject: RE: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test = RFC (Now with docs!)

 

Answer the question 5,6,7,8, which are related to H= BFA.

 

For other MSFT unit test, I don’t have strong= opinion. I am OK with that.

 

Thank you

Yao Jiewen

 

From: devel@edk2.groups.io <deve= l@edk2.groups.io> On Behalf Of Bret Barkelew via Groups.Io
Sent: Saturday, December 14, 2019 8:47 AM
To: devel@edk2.groups.io; Andrew Fish <afish@apple.com&g= t;; Kinney, Michael D <mic= hael.d.kinney@intel.com>
Cc: rfc@edk2.groups.io Subject: Re: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test = RFC (Now with docs!)

 

Mike,

 

I think I’ve gotten all the feedback here and= all the action items from our call. The current branch should be good to g= o, minus the couple of things that Intel was going to look into.=

 

Thanks!

 

- Bret

 

From: Bret Barkelew
Sent: Friday, December 6, 2019 2:21 PM
To: devel@edk2.groups.io; Andrew Fish; Kinney, Mic= hael D
Cc: rfc@edk2.groups.io Subject: RE: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test = RFC (Now with docs!)

 

  1. I see that MdePkg adds a dependency on UnitTestPkg.  This makes UnitT= estPkg the root package when building and running host based unit tests.&nb= sp; This makes sense, but good to highlight that all packages that use host= based tests will introduce a new package dependency on UnitTestPkg.
    1. Since the dependency only applies to unit tests, can we update the Depende= ncyCheck plugin to support listing dependencies for FW components separate = from dependencies for unit tests?
    2. Can move. Capability is there, but mistakenly ad= ded to the wrong section.
  1. I see UnitTestPkg declares 6 new lib classes.  Are all 6 classes inte= nded 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 m= ake them private?
    1. Because there are different instances in multipl= e places (and conceivably more in the future), we don’t see how we co= uld make them private.
  1. In the MdePkg, I see a new top level directory called ‘HostLibrary&#= 8217;. Since these lib instances are only used from a host based test, can = they be moved into  the ‘Test’ directory?
    2. <= /ol>
      1. Can move.
    1. MdePkg/MdePkgTest.dsc. 
      1. Can this DSC file be moved into the ‘Test’ directory?

           = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   Yes=

      1. I see this DSC file only supports VS today.  How much work is it to a= dd GCC support?

           = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   Don’= t know. This is an item on our list, but lower priority and not a blocker.<= /span>

    1. MdePkg/HostLibrary/BaseLibHost
      1. 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?
        2. I see the purpose of this lib instance is to call the
      2. 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.
      3. 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 b= ased testing.  Can we change to ASSERT (FALSE)?
      4. PatchInstructionX86() – I suspect this will not work in a host based= environment because it is self modifying code.  Should it be ASSERT (= FALSE)?
      5. Libraries were taken directly from the Intel wor= k in HBFA. They worked so we kept them. We’re just as interested in t= he answers to these questions as you are.

    [Jiewen] Here is the answer:

    Some tools requi= res source build, such AFL-CC or KLEE to get the result or optimization. Th= ey do analysis besides compiling. These tools only work on C-language. That= why we need non-asm build.

    Some test case n= eed access CRx/DRx just as the pre-condition and post-condition, such as MT= RR test. We cannot use ASSERT.

    Generally, a big= problem of current based lib is that it mixes the hardware register access= (such as CR/DR, CPUIR, MSR, mode switch) into a common software lib (such = as linked list, math, string, checksum). I hope we can refactory the based lib to separate the hardware access par= t, such as a BaseRegAccessLib, similar to IoLib, PciLib.

    Agree on PatchIn= structionX86(). We should use ASSERT(). If we encounter a case that require= s this function, we consider how to make it work later.

     

    1. MdePkg/HostLibrary/DebugLibHost
      1. What is ‘#ifndef TEST_WITH_KLEE’
      2. What is the ‘PatchFormat()’ function?  It is always disab= led with if (0).
      3. Are the PCDs to set the debug message levels disabled on purpose? (DebugPr= intEnabled(), DebugPrintLevelEnabled(), DebugCodeEnabled())
        4. =
      4. Libraries were taken directly from the Intel wor= k in HBFA. They worked so we kept them. We’re just as interested in t= he answers to these questions as you are.

     

    [Jiewen] Here is the answer:

            &nb= sp;      This is special optimization for KLEE = 211; a symbolic execution tool. We hope to reduce the unnecessary branch. O= r the KELL might not resolve the function.

            &nb= sp;      PatchFormat() is an old function. Deadcod= e can be removed now.

            &nb= sp;      In the beginning, PCD is not supported. A= nd we don’t find a use case to control PCD in such detail level. To s= implify thing, we just don’t use it. An on/off control (via HOST_DEBU= G_MESSAGE) is good enough. If there is real use case coming up, we c= an see how to enable it.

     

     

    1. MdePkg/HostLibrary/BaseMemoryLibHost
      1. Why can’t we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf inst= ead and reduce the number of host specific lib instances?
      2. Libraries were taken directly from the Intel wor= k in HBFA. They worked so we kept them. We’re just as interested in t= he answers to these questions as you are.

    [Jiewen] Here is the answer:

            &nb= sp;      This is special optimization for fuzzing = tool. If we use standard C lib, the compiler knows it and don’t inser= t branch analysis. If we use our own implementation, the compiler has no id= ea, and try to do analysis. That is unnecessary.

     

     

    1. MdePkg/HostLibrary/MemoryAllocationLibHost
      1. Why is are memcpy(), assert(), memset() used in this lib?  I would ex= pect this lib instance to match the UefiMemoryAllocationLib with the only t= he use of malloc() and free() to replace the UEFI Boot Services calls.
      2. Libraries were taken directly from the Intel wor= k in HBFA. They worked so we kept them. We’re just as interested in t= he answers to these questions as you are.

    [Jiewen] Here is the answer:

            &nb= sp;      Similar to 7, using standard-C function c= an reduce the unnecessary analysis for analysis tool.

     

    1. Host library instance naming conventions.
      1. The current naming convention uses the environment as a prefix(e.g. Pei, S= mm, 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 post= fix in the lib instance names?
      2. I don’t know if there’s a 1:1 relati= onship with these. For some library classes (that you might have host versi= ons of), the class itself is the PeiSomethingLib, and the host version woul= d be the PeiSomethingLibHost. No?
    1. Unicode vs ASCII strings
      1. I see InitUnitTestFramework(), CreateUnitTestSuite(), and AddTestCase() al= l take Unicode strings for the labels.  I also see extra code to conve= rt 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.
      2. No strong feelings, but we already have a bunch = of tests written this way. Since the UnitTestLib is an abstraction that wor= ks as well in Shell as in the Host, going with Unicode strings seemed to ma= tch the art for Shell apps (since the framework was written for Shell first).
    1. Will InitUnitTestFramework() and CreateUnitTestSuite() always be called in= pairs?  If so, can we combine these to a single API? 
      1. I see the SafeIntLib example create a single framework and multiple test s= uites.  Perhaps we can have a single CreateUnitTestSuite() API where F= ramework is an IN/OUT and if it is passed in as NULL, the Framework handle = is created.

           = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   It’s= not always in pairs. You would only have a single framework init, but coul= d have multiple suites.

      1. I see a pattern where the CreateUnitTestSuite() ‘Package’ para= meter is used as a prefix to every call to AddTestCase() in the ‘Clas= sName’ 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?

             = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   Right now,= our tests just coincidentally share similar names with the packages, but w= e don’t feel this is axiomatic and don’t want to force this nam= ing 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 ‘F= ramework’.  Can we use something other than ‘Fw’.&nb= sp; It may be confused with ‘Firmware’. 
        1. No real arguments. Wouldn’t fight a find-r= eplace, but it’ll just be a bunch of touches as we commit.
      1. UnitTestPkg/Include/UnitTestTypes.h
        1. I see several hard coded string lengths.  Since this runs in a host e= nvironment 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()?

             = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   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.

        1. How are Fingerprints used?  The idea of using as hash as a unique ide= ntifier is a good idea.  How is the hash calculated?  What unit t= est code artifacts are used so developers know what parameters must be uniq= ue?  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 fi= xed size array to make it easy to change the algorithm/size in the future?<= o:p>

             = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   Fingerprin= ts are unique IDs to make sure that serialize/unserialized state matches th= e tests upon re-entry. I’m not married to MD5, but it needs to be pre= dictably unique, and carried with the framework. I would not want to make any requ= irements on CryptoLib, since these aren’t crypto-strong.<= /o:p>

        1. Update all the strings to be ASCII?  See Unicode vs ASCII above.=

             = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   Ideally no= t, unless there’s a strong use case.

        1. UNIT_TEST_SAVE_TEST – The last field is a variable sized array, so i= t can be a formal field.

             = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   Not oppose= d, but don’t really want to make the change myself. Is there a style = guide that this is breaking?

        1. UNIT_TEST_SAVE_CONTEXT - – The last field is a variable sized array,= so it can be a formal field.
        2. UNIT_TEST_SAVE_HEADER – Can the last 3 fields be changed to pointers= and be formal fields?
        3. Do the structures really need to be packed?  Specially with the chang= es suggested above?  Is the intent to potentially share data stored on= disk between different host execution environments that may have different= width architectures?

             = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   That’= ;s an interesting point. Could you draw up your suggestion and submit a PR?=

      1. UnitTestPkg – UnitTestLib.h
        1. Can you provide more context for the APIs SaveFrameworkState(), SaveFramew= orkStateAndQuit(), SaveFrameworkStateAndReboot(), SetFrameworkBootNextDevic= e().  I do not see any Load() functions, so how would a set of tests b= e resumed?  If these do not apply to host based tests, should these be split out to a different lib class?

             = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   I’ll= improve the documentation around these functions. I will acknowledge, howe= ver, that this is an interface that I expect to evolve as we figure out wha= t kinds of tests the community wants support for “out of the box̶= 1;. If we want to easily enable tests around – for example – Ex= itBootServices, we will likely want to tweak this going forward to its simp= lest form. The version we have here was sufficient to enable the test cases that we’ve currently written.

      1. UnitTestPkg/Library/UnitTestLib
        1. I see an implementation of MD5.  We should not do our own.  We s= hould use an approved implementation such as OpenSSL.

             = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   Happy to d= iscuss another implementation, but this is not crypto-strong. It’s ju= st for uniqueness. A sufficiently long CRC would probably work, too.=

      1. UnitTestPkg/Library/UnitTestTerminationLibTbd
        1. Do we really need this lib instance now?

             = ;            &n= bsp;            = ;            &n= bsp;            = ; i. &nbs= p;   This is he= re so that we can figure out what this should look like for a host. Host ob= viously wouldn’t reset, but could conceivably quit. Or maybe that doe= sn’t make any sense for a host.

       

       

       

      - Bret

       

      From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf o= f Bret Barkelew via Groups.Io <bret.barkelew=3Dmicrosoft.com@groups.io>
      Sent: Wednesday, December 4, 2019 10:24:21 AM
      To: Andrew Fish <afish@apple.= com>; devel@edk2.groups.io <devel= @edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com>
      Cc: rfc@edk2.groups.io &l= t;rfc@edk2.groups.io>
      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 l= ook and update the draft. I’ll ping back ASAP.

       

      - Bret

       

      From: An= drew Fish
      Sent: Wednesday, December 4, 2019 9:50 AM
      To: devel@edk2.groups.io; Kinney, Michael D
      Cc: Bret Barkelew; rfc@edk2.groups.io
      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/ (&nbs= p;*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 m= odules 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 d= ifferentiate the code from tests while doing `git grep` or other forms of c= ode spelunking. 

       

      A Host-Based Unit Test for a Library makes sense a= s 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 enou= gh 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 thi= nk 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 ge= t 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 <<= a href=3D"mailto:michael.d.kinney@intel.com">michael.d.kinney@intel.com= > 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. <= span class=3D"xapple-converted-space"> This makes sense, but good to highlight that all packages that use host based tests = will introduce a new package dependency on UnitTestPkg.
        1. Since the dependency only applies to unit tests, can we update the Depende= ncyCheck plugin t= o support listing dependencies for FW components separate from dependencies for unit tests?
      1. I see UnitTestPkg declares 6 new lib classes. &= nbsp;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 t= he UnitTestPkg ca= n 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.d= sc. 
        1. Can this DSC file be moved into the ‘Test’ directory? 
        2. I see this DSC file only supports VS today.  How much work is it to add GCC support?
      1. MdePkg/HostLibrary<= /span>/BaseLibHost
        1. Why are there 2 INFs. &= nbsp;One with ASM and one without ASM?  Can we just use the one with ASM and assume NASM is installed?
        2. I see the purpose of this lib instance is to call the
        3. SetJump()/LongJump<= /span>().  So these= implementations always work? &= nbsp;It looks like it assumes BASE_LIBRARY_JUMP_BUFFER is identical to the host OS user mode application= setjmp()/longjmp() state.<= /li>
        4. Why are DRx and<= span class=3D"xapple-converted-space"> = CRx registers emulated?  I woul= d think and code that depends on read/writing these registers would not be = compatible with host based testing.  Can we change to ASSERT (FALSE)?
        5. 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<= /span>/DebugLibHost
        1. What is ‘#ifndef TEST_WITH_KLEE’
        2. What is the ‘PatchFormat()’ fun= ction?  It is alway= s disabled with if (0).
        3. Are the PCDs to set the debug message levels disabled on purpose? (DebugPrintEnabled(), DebugPrintLevelEnabled(= ), DebugCodeEnabled())
      1. MdePkg/HostLibrary<= /span>/BaseMemoryLibHost
        1. Why can’t we use = MdePkg/Library/BaseM= emoryLib/BaseMemoryLib/inf instead an= d reduce the number of host specific lib instances?
      1. MdePkg/HostLibrary<= /span>/MemoryAllocationLibHost
        1. Why is are memcpy(), assert(), memset() used in this lib?&= nbsp; I would expect this lib instance to match the UefiMemoryAllocationLib with the only the use of mal= loc() and free() to replace the UEFI Boot Services calls.
      1. Host library instance naming conventions.
        1. The current naming convention uses the environment as a prefix(e.g. Pei, S= mm, 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
        1. I see InitUnitTestFramework(), CreateUnitTestSuite(), and Ad= dTestCase() all take Unicode strings for the labels.  I also see extra code to convert gEfiCallerBaseNam= e 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 t= o Unicode if needed.
      1. Will InitUnitTestFramework() and CreateUnitTestSuite() alway= s be called in pairs?  If so, can we combine these to a single API? 
        1. 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/O= UT and if it is passed in as NULL, the Framework handle is created.
        2. I see a pattern where the CreateUnitTestSuite() ‘Package̵= 7; parameter is used as a prefix to every call to AddTestCase() in the ‘ClassName’ parameter.&= nbsp; Can we simplify Ad= dTestCase() 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’ varia= ble as a shorthand for ‘Framework’.  Can we use something other than ‘Fw’.  It may be confused with ‘Firmware’.  
        2. UnitTestPkg/Include/UnitTestTypes.h
          1. I see several hard coded string lengths.  Since this runs in a host environment and strings c= an 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 ca= n manage alloc() and free()?
          2. How are Fingerprints used? &nb= sp;The idea of using as hash as a unique identifier is a good idea.&= nbsp; How is the hash ca= lculated?  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 fu= ture?
          3. Update all the strings to be ASCII?  See Unicode vs ASCII above.
          4. UNIT_TEST_SAVE_TEST – The last field is a variable sized array, so i= t can be a formal field.
          5. UNIT_TEST_SAVE_CONTEXT - – The last field is a variable sized array,= so it can be a formal field.
          6. UNIT_TEST_SAVE_HEADER – Can the last 3 fields be changed to pointers= and be formal fields?
          7. Do the structures really need to be packed?  Specially with the changes suggested above? = ; Is the intent to poten= tially share data stored on disk between different host execution environments that may have different width architectures?
            8. =
        1. UnitTestPkg – UnitTestLib.h
          1. Can you provide more context for the APIs SaveFrameworkState(), Sav= eFrameworkStateAndQuit(),&nbs= p;SaveFrameworkStateAndReboot(), SetF= rameworkBootNextDevice().  I do not see any Load() functions, so how would a set of tests be resumed?&= nbsp; If these do not ap= ply to host based tests, should these be split out to a different lib class= ?
        1. UnitTestPkg/Library/UnitTestLib
          1. I see an implementation of MD5.  We should not do our own.  We should use an approved implementation such as Ope= nSSL.
        1. UnitTestPkg/Library/UnitTestTerminationLibTbd
          1. Do we really need this lib instance now?

         

        Thanks,

         

        Mike

         

        From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Bar= kelew via Groups.Io
        Sent: Thursday, = November 21, 2019 11:39 PM
        To: devel@edk2.groups= .io
        Subject: [edk2-d= evel] EDK2 Host-Based Unit Test RFC (Now with docs!)

         

        Now that CI has lan= ded in edk2/master, we'd like to get the basic framework for unittesting st= aged 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, HostUnitTestDxeComp= leteCheck, 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/edk= 2-host-test_v2
        We also have a demo build pipeline at:
        https://dev.azure.com/tianoc= ore/edk2-ci-play/_build?definitionId=3D36&_a=3Dsummary

        The current demo branch contains a single test in MdePkg for SafeIntLib (m= odule file MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.i= nf). This test is automatically detected by the HostUnitTestComilerPlugin a= nd 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 r= eason 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=3D2813
        "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 th= e community to decide whether the test case is correct or the library is co= rrect, 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 pr= oject 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 no= t explicitly firmware code, so it seems unnecessary). Maybe in a new edk2-t= est2 repo or something like that.

        There’s an RF= C doc here: = https://github.com/corthon/edk2-staging/blob/= edk2-host-test_v2/Readme-RFC.md

        And a usage guide h= ere: https://github.com/corthon/edk2-staging/blob= /edk2-host-test_v2/UnitTestPkg/ReadMe.md


        Once again, would love to get this into EDK2 master after stabilization, a= nd most of this has previously been shopped around in other discussion thre= ads. 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 ru= n as part of any CI pass.

        Thanks!
        - Bret

       

       

       

--_000_74D8A39837DF1E4DA445A8C0B3885C503F8A361Cshsmsx102ccrcor_-- --_004_74D8A39837DF1E4DA445A8C0B3885C503F8A361Cshsmsx102ccrcor_ Content-Type: image/png; name="image002.png" Content-Description: image002.png Content-Disposition: inline; filename="image002.png"; size=241; creation-date="Mon, 16 Dec 2019 22:03:30 GMT"; modification-date="Mon, 16 Dec 2019 22:03:30 GMT" Content-ID: Content-Transfer-Encoding: base64 iVBORw0KGgoAAAANSUhEUgAAArUAAAADCAYAAACNrPcAAAAAAXNSR0IArs4c6QAAAAlwSFlzAAAS dAAAEnQB3mYfeAAAABl0RVh0U29mdHdhcmUATWljcm9zb2Z0IE9mZmljZX/tNXEAAABxSURBVGhD 7dCxDYAwDAXR72AkOtZlBoZiAyZAoqVOQwExZo5IdxOcnm/7EUpdpnxSMhECCCCAAAIIIIAAAp0J uA9ew7Tovc+iyTv7ZxcBBBBAAAEEEEAAAXmLNhfZmhpb6oMEAQQQQAABBBBAAIHuBH7uQhUt1L8M PgAAAABJRU5ErkJggg== --_004_74D8A39837DF1E4DA445A8C0B3885C503F8A361Cshsmsx102ccrcor_--