From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (NAM10-MW2-obe.outbound.protection.outlook.com [40.107.94.101]) by mx.groups.io with SMTP id smtpd.web11.2080.1576284423786427499 for ; Fri, 13 Dec 2019 16:47:04 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=A7ebQ5TQ; spf=pass (domain: microsoft.com, ip: 40.107.94.101, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MBgUm8JDX4UlRYZn6z3U3sO+gghFjUALJ4MkvuR66Mx6QyXKwsCmpKupuHAhhw9CHdgiwWEk4zQSZNoyprFpDONsmzr5xgYvHz8YUd5Oo9PbrzLZjtbiqrDFMHmFeiVT0yrhHhM+eAIu9tXwJhiH1+bo4rMfOhRVOVqDSMIrOfWJunvWDyRP7R5kHuHh5d1d82k/tzMqElZ73KJdBe+hTt8a+VCMVO2ye8YxRU4dL1W2JmtrbBkPeOjGyPJTyhjbCcJBKJTYlPEwTXQN/DXUM+ad5oof2JPbXBhwubT47M+1gXC2+6cMwLi7ZYm5V3sSdnJKCZfHn57KwWMvJAV/5A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=kPEG6Ytnun3z+I923JyvZIlOSK/us5ZDe1xIJ7x3QJ0=; b=S9kbVm9InZvmx6V0ArTsdPv5cahmKPVmt+7ggFWvZX1GeluHMWj9ogep+EAUvcYpSfBTFNjRYj1MJDST5Go+RZD3+OBqqtto3ZOe4m+JV+qEhJLIaHE3cgBUvrQbBDYoeUls0CjRlhjH4QioQg3JloVexQy1k6vBQm2E37cLjpO5zQI5GrGrXF9UgWJmSTjbCHUpF3WhqZWZ/xgx6omouvXA6V4nC5Bv5006w+lJ2XhLN8Ma9wx3FnaShquJV7dn3ZFkEI0vpAPAGJKsEFSjpH0xwlX15N4L4AfFZZ4dRQrzFCaWof4pYFqpyWvo6JHcwOVIsxDCFG/xAjvhq5Lisw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=microsoft.com; dmarc=pass action=none header.from=microsoft.com; dkim=pass header.d=microsoft.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=kPEG6Ytnun3z+I923JyvZIlOSK/us5ZDe1xIJ7x3QJ0=; b=A7ebQ5TQp5wAgThnCxXn9Cz7lWo662JGXiU+4evjN6luiqEIF4dmB+Ab5PGczG2ng/iNKB4Q+KrYr/II57YYWa8qLQqBvIZe8Nu3eqLrxgaslAjrMRGeuU/fyVMwHLdvJukaq8jIGz0m1rdkjIvNDvwftZ6IV438p0pXLplwdq0= Received: from CH2PR21MB1400.namprd21.prod.outlook.com (20.180.10.144) by CH2PR21MB1382.namprd21.prod.outlook.com (20.180.10.203) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2559.8; Sat, 14 Dec 2019 00:46:59 +0000 Received: from CH2PR21MB1400.namprd21.prod.outlook.com ([fe80::a147:c42f:38de:e8bd]) by CH2PR21MB1400.namprd21.prod.outlook.com ([fe80::a147:c42f:38de:e8bd%4]) with mapi id 15.20.2559.009; Sat, 14 Dec 2019 00:46:59 +0000 From: "Bret Barkelew" 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 (Now with docs!) Thread-Topic: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!) Thread-Index: AQHVoQS12ZW6E6MMrkasWCUh3oP5e6enceDwgALivYCAAAlHOoADZyKbgAsoujY= Date: Sat, 14 Dec 2019 00:46:58 +0000 Message-ID: References: ,<0F460E19-2A3B-4FEE-8B6F-BE65B9C5F1D4@apple.com>,<15DD3E3A746110DF.27746@groups.io>, In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: 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 authentication-results: spf=none (sender IP is ) smtp.mailfrom=Bret.Barkelew@microsoft.com; x-originating-ip: [2001:4898:80e8:f:bdaa:b0a:c50c:ee40] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: d8f44312-d586-4eff-7677-08d7802f2075 x-ms-traffictypediagnostic: CH2PR21MB1382:|CH2PR21MB1382: x-ld-processed: 72f988bf-86f1-41af-91ab-2d7cd011db47,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:350; x-forefront-prvs: 025100C802 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(376002)(396003)(366004)(346002)(39860400002)(136003)(174864002)(189003)(199004)(186003)(71200400001)(66576008)(6506007)(86362001)(9686003)(8936002)(52536014)(30864003)(4326008)(53546011)(91956017)(76116006)(66946007)(76236002)(81156014)(110136005)(55016002)(2906002)(966005)(478600001)(5660300002)(64756008)(66446008)(66476007)(33656002)(8676002)(81166006)(316002)(8990500004)(7696005)(66556008)(10290500003)(579004)(569006);DIR:OUT;SFP:1102;SCL:1;SRVR:CH2PR21MB1382;H:CH2PR21MB1400.namprd21.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;MX:1;A:1; received-spf: None (protection.outlook.com: microsoft.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: wngHt9P7VmMCCnYfXZLd2ofFu9A+AFW6f1u+ZxUPeCSUGsyN0Uay4/1psZ7Hg2H+pHnLzUmxj98ThfomaLDNNaTMm4rR5vP/zMsec3IcXXTWWpFk5zHaBdu75BrF4EKuZiiXrRY8NeCiqvkSMfes6flFle8aeGvGahMHmQMJnl6R/WJt99R2/X/+QLJW9cvWQ5fupMlxSm4QguCyPGkSRWR7cD9q2ia6J2leDFcRYQNTSIa+3ZqVCQnVHrcmQ1gvtWl1wakzLoFNPYPBYXgg20JMRMDFRQ/1yKXYEXjccac9wTUbXbGnqRnh3KKzumg5JlVAS3LRXpv9oG8vebmxiG3cnXq6z3WqtSfQmfJlg891iBtnbyEt+LV+vPvqq3Cr6j0GjkOShO+b48SaG7n1mLQWoP4L9juDU8xc7aa6aFhwykvLsiod67VN1gndbbCiXWQ/A/zANFRQ5IJajNVHat2x2KRgqu++u40cDRh+t+2GmuVP8XTTPvFY5nEyTEm3co3RMeNbiLTHVaqgsMv8XMn+JWjF5zUNKtWkhYkkH34= x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-Network-Message-Id: d8f44312-d586-4eff-7677-08d7802f2075 X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Dec 2019 00:46:58.9176 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: MHfGu0W2obFNGNiL9ph1oatxrBFIzbwTj+vntPpv1EHfSOveqD6AV3kBR8LM8aAy7j9VCfvnFOqYjYy3mvU82A== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR21MB1382 X-Groupsio-MsgNum: 52220 Content-Language: en-US Content-Type: multipart/related; boundary="_005_CH2PR21MB14007D64FAABF243388D5270EF570CH2PR21MB1400namp_"; type="multipart/alternative" --_005_CH2PR21MB14007D64FAABF243388D5270EF570CH2PR21MB1400namp_ Content-Type: multipart/alternative; boundary="_000_CH2PR21MB14007D64FAABF243388D5270EF570CH2PR21MB1400namp_" --_000_CH2PR21MB14007D64FAABF243388D5270EF570CH2PR21MB1400namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable Mike, I think I=92ve gotten all the feedback here and all the action items from = our call. The current branch should be good to go, minus the couple of thin= gs 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. 2. 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=92t see how we could make them private= . 3. In the MdePkg, I see a new top level directory called =91HostLibrary= = =92. Since these lib instances are only used from a host based test, can t= hey be moved into the =91Test=92 directory? * Can move. 4. MdePkg/MdePkgTest.dsc. * Can this DSC file be moved into the =91Test=92 directory? i. Yes * I see this DSC file only supports VS today. How much work is it = to add GCC support? i. Don=92t know. T= his 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() =96 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 w= orked so we kept them. We=92re just as interested in the answers to these q= uestions as you are. 2. MdePkg/HostLibrary/DebugLibHost * What is =91#ifndef TEST_WITH_KLEE=92 * What is the =91PatchFormat()=92 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=92re just as interested in the answers to these q= uestions as you are. 3. MdePkg/HostLibrary/BaseMemoryLibHost * Why can=92t 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 w= orked so we kept them. We=92re just as interested in the answers to these q= uestions as you are. 4. 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=92re just as interested in the answers to these q= uestions 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. W= ould it make more sense to use =91Host=92 as a prefix instead of a postfix = in the lib instance names? * I don=92t know if there=92s a 1:1 relationship with these. For so= me library classes (that you might have host versions of), the class itself= is the PeiSomethingLib, and the host version would be the PeiSomethingLibH= ost. No? 6. 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). 7. 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=92s not alwa= ys in pairs. You would only have a single framework init, but could have mu= ltiple suites. * I see a pattern where the CreateUnitTestSuite() =91Package=92 par= ameter is used as a prefix to every call to AddTestCase() in the =91ClassNa= me=92 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 t= he unit test case name? i. Right now, our = tests just coincidentally share similar names with the packages, but we don= = =92t feel this is axiomatic and don=92t want to force this naming on other= s, who may be trying to bolt into other reporting structures. 1. I see the use of the =91Fw=92 variable as a shorthand for =91Framewo= rk=92. Can we use something other than =91Fw=92. It may be confused with = = =91Firmware=92. * No real arguments. Wouldn=92t fight a find-replace, but it=92ll j= ust 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 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 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 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. Fingerprints ar= e unique IDs to make sure that serialize/unserialized state matches the tes= ts upon re-entry. I=92m not married to MD5, but it needs to be predictably = unique, and carried with the framework. I would not want to make any requir= ements on CryptoLib, since these aren=92t crypto-strong. * Update all the strings to be ASCII? See Unicode vs ASCII above. i. Ideally not, un= less there=92s a strong use case. * UNIT_TEST_SAVE_TEST =96 The last field is a variable sized array,= so it can be a formal field. i. Not opposed, bu= t don=92t really want to make the change myself. Is there a style guide tha= t this is breaking? * UNIT_TEST_SAVE_CONTEXT - =96 The last field is a variable sized a= rray, so it can be a formal field. * UNIT_TEST_SAVE_HEADER =96 Can the last 3 fields be changed to poi= nters 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=92s an int= eresting point. Could you draw up your suggestion and submit a PR? 1. UnitTestPkg =96 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=92ll 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 o= f tests the community wants support for =93out of the box=94. If we want to= easily enable tests around =96 for example =96 ExitBootServices, we will l= ikely want to tweak this going forward to its simplest form. The version we= have here was sufficient to enable the test cases that we=92ve currently w= ritten. 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 discus= s another implementation, but this is not crypto-strong. It=92s just for un= iqueness. 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 obvious= ly wouldn=92t reset, but could conceivably quit. Or maybe that doesn=92t ma= ke any sense for a host. - Bret From: devel@edk2.groups.io on behalf of Bret Barkel= ew via Groups.Io Sent: Wednesday, December 4, 2019 10:24:21 AM To: Andrew Fish ; devel@edk2.groups.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=92ve got a lot more there. Let me take a look and update the draft. I= =92ll 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 =91HostLibrary= = =92. Since these lib instances are only used from a host based test, can t= hey be moved into the =91Test=92 directory? 3. MdePkg/MdePkgTest.dsc. * Can this DSC file be moved into the =91Test=92 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() =96 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 =91#ifndef TEST_WITH_KLEE=92 * What is the =91PatchFormat()=92 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=92t 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 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 =91Host=92 as a prefix instead of a postfix = in the 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() =91Package=92 par= ameter is used as a prefix to every call to AddTestCase() in the =91ClassNa= me=92 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 t= he unit test case name? 1. I see the use of the =91Fw=92 variable as a shorthand for =91Framewo= rk=92. Can we use something other than =91Fw=92. It may be confused with = = =91Firmware=92. 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 =96 The last field is a variable sized array,= so it can be a formal field. * UNIT_TEST_SAVE_CONTEXT - =96 The last field is a variable sized a= rray, so it can be a formal field. * UNIT_TEST_SAVE_HEADER =96 Can the last 3 fields be changed to poi= nters 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 =96 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=92s an RFC doc here: https://github.com/corthon/edk2-staging/blob/ed= k2-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_CH2PR21MB14007D64FAABF243388D5270EF570CH2PR21MB1400namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable

Mike,

 

I think I=92ve gotten all the feedback here and all= the action items from our call. The current branch should be good to go, m= inus 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.
  2. 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=92t see how we could = make them private.
  3. In the MdePkg, I see a new top level directory called =91HostLibrary=92. S= ince these lib instances are only used from a host based test, can they be = moved into  the =91Test=92 directory?
    1. Can move.
  4. MdePkg/MdePkgTest.dsc. 
    1. Can this DSC file be moved into the =91Test=92 directory?
      2. =

       = ;            &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=92t kn= ow. This is an item on our list, but lower priority and not a blocker.

  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() =96 I suspect this will not work in a host based env= ironment because it is self modifying code.  Should it be ASSERT (FALS= E)?
    5. Libraries were taken directly from the Intel wor= k in HBFA. They worked so we kept them. We=92re just as interested in the a= nswers to these questions as you are.
  2. MdePkg/HostLibrary/DebugLibHost
    1. What is =91#ifndef TEST_WITH_KLEE=92
    2. What is the =91PatchFormat()=92 function?  It is always disabled 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=92re just as interested in the a= nswers to these questions as you are.
  3. MdePkg/HostLibrary/BaseMemoryLibHost
    1. Why can=92t we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf instead = 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=92re just as interested in the a= nswers to these questions as you are.
  4. 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=92re just as interested in the a= nswers to these questions as you are.
  5. 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 =91Host=92 as a prefix instead of a postfix in t= he lib instance names?
    2. I don=92t know if there=92s a 1:1 relationship w= ith 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
    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).
  7. 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=92s not= always in pairs. You would only have a single framework init, but could ha= ve multiple suites.

    1. I see a pattern where the CreateUnitTestSuite() =91Package=92 parameter is= used as a prefix to every call to AddTestCase() in the =91ClassName=92 par= ameter.  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=92t feel this is axiomatic and don=92t want to force this naming on others, who may be trying to bolt into other reporting structures.

    1. I see the use of the =91Fw=92 variable as a shorthand for =91Framework=92.=   Can we use something other than =91Fw=92.  It may be confused w= ith =91Firmware=92. 
      1. No real arguments. Wouldn=92t fight a find-repla= ce, but it=92ll just be a bunch of touches as we commit.<= /li>
    2. 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=92m not married to MD5, but it needs to be predict= ably unique, and carried with the framework. I would not want to make any requ= irements on CryptoLib, since these aren=92t crypto-strong.

      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=92s a strong use case.

      1. UNIT_TEST_SAVE_TEST =96 The last field is a variable sized array, so it ca= n be a formal field.

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

      1. UNIT_TEST_SAVE_CONTEXT - =96 The last field is a variable sized array, so = it can be a formal field.
      2. UNIT_TEST_SAVE_HEADER =96 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=92s a= n interesting point. Could you draw up your suggestion and submit a PR?

    1. UnitTestPkg =96 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=92ll imp= rove 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 =93out of the box=94. If w= e want to easily enable tests around =96 for example =96 ExitBootServices, = we will likely want to tweak this going forward to its simplest form. The v= ersion we have here was sufficient to enable the test cases that we=92ve 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=92s just f= or 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=92t reset, but could conceivably quit. Or maybe that doesn= =92t make any sense for a host.

     

     

     

    - Bret

     

    From: devel@edk2.groups.io <devel@edk2.groups.io> = on behalf of 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 <d= evel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@intel.com&g= t;
    Cc: rfc@edk2.groups.io <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=92ve got a lot more there. Let me take a look = and update the draft. I=92ll 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 =91HostLibrary=92. Since these lib instances are only use= d from a host based test, can they be moved into  the =91Test=92 directory?
    3. MdePkg/MdePkgTest.d= sc. 
      1. Can this DSC file be moved into the =91Test=92 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() =96 I suspect this will not work in a host based env= ironment because it is <= span class=3D"xspelle">self modifying code. &nbs= p;Should it be ASSERT (FALSE)?
    1. MdePkg/HostLibrary<= /span>/DebugLibHost
      1. What is =91#ifndef TEST_WITH_KLEE=92
      2. What is the =91PatchFormat()=92 function?&n= bsp; It is always disabl= ed with if (0).
      3. DebugPrintEnabled(), DebugPrintLevelEnabled(= ), DebugCodeEnabled())
    1. MdePkg/HostLibrary<= /span>/BaseMemoryLibHost
      1. Why can=92t we use MdePkg/Library/BaseMemor= yLib/BaseMemoryLib/inf instead and re= duce 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 =91Host=92 as a prefix instead of a pos= tfix 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() =91Package=92 param= eter is used as a prefix to every call to AddTestCase() in the =91ClassName=92 parameter.  Can we simplify AddTestCa= se() 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 =91Fw=92 variable as a= shorthand for =91Framework=92.  Can we use something other than =91Fw= =92.  It may be confused with =91Firmware=92.  
      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 =96 The last field is a variable sized array, so it ca= n be a formal field.
        5. UNIT_TEST_SAVE_CONTEXT - =96 The last field is a variable sized array, so = it can be a formal field.
        6. UNIT_TEST_SAVE_HEADER =96 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 =96 = 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=92s an RFC do= c here: https://github.com/corthon/edk2-staging/blob/edk= 2-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_CH2PR21MB14007D64FAABF243388D5270EF570CH2PR21MB1400namp_-- --_005_CH2PR21MB14007D64FAABF243388D5270EF570CH2PR21MB1400namp_ Content-Type: image/png; name="F682B0CBBA1B476BA5A57F08CA726D52.png" Content-Description: F682B0CBBA1B476BA5A57F08CA726D52.png Content-Disposition: inline; filename="F682B0CBBA1B476BA5A57F08CA726D52.png"; size=159; creation-date="Sat, 14 Dec 2019 00:46:58 GMT"; modification-date="Sat, 14 Dec 2019 00:46:58 GMT" Content-ID: Content-Transfer-Encoding: base64 iVBORw0KGgoAAAANSUhEUgAAArYAAAADCAYAAABmm0wDAAAAAXNSR0IArs4c6QAAAARnQU1BAACx jwv8YQUAAAAJcEhZcwAADsMAAA7DAcdvqGQAAAA0SURBVGhD7dZBCQAwDATB+DdVBVUQMSn0GQcH szAets7tAQCAdH9sS5IkSUpvny4AAOTpeVQ/cX0X8qc8AAAAAElFTkSuQmCC --_005_CH2PR21MB14007D64FAABF243388D5270EF570CH2PR21MB1400namp_ Content-Type: image/png; name="75CF15E5BB864774AD23F7E65474DED4.png" Content-Description: 75CF15E5BB864774AD23F7E65474DED4.png Content-Disposition: inline; filename="75CF15E5BB864774AD23F7E65474DED4.png"; size=140; creation-date="Sat, 14 Dec 2019 00:46:58 GMT"; modification-date="Sat, 14 Dec 2019 00:46:58 GMT" Content-ID: Content-Transfer-Encoding: base64 iVBORw0KGgoAAAANSUhEUgAAAsQAAAABCAYAAADZ77itAAAAAXNSR0IArs4c6QAAAARnQU1BAACx jwv8YQUAAAAJcEhZcwAADsMAAA7DAcdvqGQAAAAhSURBVEhL7cMBDQAACAMg+5cygQkeRoMIG9WT VVXVn7MHYi5moJeByLMAAAAASUVORK5CYII= --_005_CH2PR21MB14007D64FAABF243388D5270EF570CH2PR21MB1400namp_--