From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web12.9998.1575328353812593281 for ; Mon, 02 Dec 2019 15:12:34 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: michael.d.kinney@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Dec 2019 15:12:33 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,270,1571727600"; d="scan'208,217";a="385076356" Received: from orsmsx107.amr.corp.intel.com ([10.22.240.5]) by orsmga005.jf.intel.com with ESMTP; 02 Dec 2019 15:12:32 -0800 Received: from orsmsx160.amr.corp.intel.com (10.22.226.43) by ORSMSX107.amr.corp.intel.com (10.22.240.5) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 2 Dec 2019 15:12:32 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.200]) by ORSMSX160.amr.corp.intel.com ([169.254.13.204]) with mapi id 14.03.0439.000; Mon, 2 Dec 2019 15:12:32 -0800 From: "Michael D Kinney" To: "devel@edk2.groups.io" , "bret.barkelew@microsoft.com" , "Kinney, Michael D" , "rfc@edk2.groups.io" Subject: Re: EDK2 Host-Based Unit Test RFC (Now with docs!) Thread-Topic: EDK2 Host-Based Unit Test RFC (Now with docs!) Thread-Index: AQHVoQS12ZW6E6MMrkasWCUh3oP5e6enceDw Date: Mon, 2 Dec 2019 23:12:31 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Return-Path: michael.d.kinney@intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_E92EE9817A31E24EB0585FDF735412F5B9E25DDAORSMSX113amrcor_" --_000_E92EE9817A31E24EB0585FDF735412F5B9E25DDAORSMSX113amrcor_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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? 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? 3. In the MdePkg, I see a new top level directory called 'HostLibrary'.= Since these lib instances are only used from a host based test, can they b= e moved into the 'Test' directory? 4. 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? 5. 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)? 6. 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()) 7. MdePkg/HostLibrary/BaseMemoryLibHost * Why can't we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf i= nstead and reduce the number of host specific lib instances? 8. 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. 9. 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? 10. 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. 11. 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? 12. 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'. 13. 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? 14. 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? 15. 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. 16. UnitTestPkg/Library/UnitTestTerminationLibTbd * Do we really need this lib instance now? Thanks, Mike From: devel@edk2.groups.io On Behalf Of Bret Barkel= ew 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_E92EE9817A31E24EB0585FDF735412F5B9E25DDAORSMSX113amrcor_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

H= i Bret,

<= o:p> 

T= hanks for posting this content.  Host based unit testing is a very valuable addition to the CI check= s.

<= o:p> 

I= have the following comments:

<= o:p> 

  1. I see that MdePkg adds a dependency on UnitTestPkg.  This makes UnitTestPkg the root packa= ge when building and running host based unit tests.  This makes sense, but good to highlight that all packages that use = host based tests will introduce a new package dependency on UnitTestPkg.
    1. Since the dependen= cy only applies to unit tests, can we update the DependencyCheck plugin to support listing de= pendencies for FW components separate from dependencies for unit tests?
  2. I see UnitTestPkg declares 6 new lib classes.  Are all 6 classes intended to be used directly from a unit test cas= e?  If some of these are only intended to be used from the framework in= side the UnitTestPkg can we make them private?
  3. In = the MdePkg, I see a new top level directory call= ed ‘HostLibrary’. Since these lib= instances are only used from a host based test, can they be moved into  the ‘Test’ direc= tory?
  4. MdePkg/MdePkgTest.dsc. 
    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?
  5. MdePkg/HostLibrary/Base= LibHost
    1. Why are there 2 INFs. = One with ASM and one without ASM?&= nbsp; 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().  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.
    4. Why are DRx and CRx re= gisters emulated?  I would think and code that depends on read/writing these registers= would not be compatible with host based testing.  Can we change to ASSERT (FALSE)?
    5. PatchInstructionX86() – I= suspect this will not work in a host based environment because it is self modifying co= de.  Should it be ASSERT (FALSE)?
  6. MdePkg/HostLibrary/Debu= gLibHost
    1. What is ‘#ifndef TEST_WITH_KLEE’<= /li>
    2. What is the = 216;PatchFormat()’ function?  It is always disabled with if (0).
    3. Are the PCDs to set the deb= ug message levels disabled on purpose? (DebugPrintEn= abled(), DebugPrintLevelEnabled(), DebugCodeEnabled())
  7. MdePkg/HostLibrary/Base= MemoryLibHost
    1. Why can’t we= use MdePkg/Library/BaseMe= moryLib/BaseMemoryLib/inf instead and = reduce the number of host specific lib instances?
    2. MdePkg/HostLibrary/Memo= ryAllocationLibHost
      1. Why is are memcpy(), assert(), m= emset() used in this lib?  I would expect this lib instance to match the UefiMemoryAllocationLib with the only the use of malloc() and free= () to replace the UEFI Boot Services calls.
    3. Host library = instance naming conventions.
      1. The current naming= convention uses the environment as a prefix(e.g. Pei, Smm, Uefi) and the services the lib instance uses as a post fix.  Would it make more sense to use ‘Host’ as a prefix inst= ead of a postfix in the lib instance names?
    4. Unicode vs AS= CII strings
      1. I see InitUnitTestFramework(), CreateUnitTestSuite(), and AddTestCase() all take Unicode strings for t= he labels.  I also see extra code to convert gEfiCallerB= aseName from ASCII to Unicode to use it as a short name of a test fr= amework.  I think it would be simpler if the parameters to these APIs were AS= CII and the framework can convert to Unicode if needed.
    5. 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 suites.  Perhaps we can have a single CreateUnitTestS= uite() API where Framework is an IN/OUT and if it is passed in as NU= LL, the Framework handle is created.
      2. I see a pattern where the CreateUnitTestSuite() ‘Package’ = parameter is used as a prefix to every call to AddTestCase() in the ‘ClassName’ parameter.&= nbsp; Can we simplify AddTestCase() so it o= nly need to pass in the name of the unit test case, and the framework can a= ppend Package  and the unit test case name?
    6. I see the use= of the ‘Fw’ variable as a shorthand for ‘Framework’.&n= bsp; Can we use something other than ‘F= w’.  It may be confused with ‘Firmware’. 
    7. UnitTestPkg/Include/UnitTestTypes.h=
      1. I see several hard= coded string lengths.  Since this runs in a host environment and strings can always be all= ocated, can the hard coded lengths be removed?  Update the structs to use pointers to strings instead of string arr= ays, and the framework can manage alloc() and free()?
        2. How are Fingerprints = used?  The idea of using as hash as a unique identifier is a good idea.  How is the hash calculated?  = What unit test code artifacts are used so developers know what param= eters must be unique?  Can we just decide to use a specific hash algorithm/size and the st= ructure can be a pointer to an allocated buffer instead of a fixed size arr= ay to make it easy to change the algorithm/size in the future?
      2. Update al= l the strings to be ASCII?  See Unicode vs ASCII above.
      3. UNIT_TEST_SAVE_TEST – The las= t field is a variable sized array, so it can be a formal field.
      4. UNIT_TEST_SAVE_CONTEXT - – The last fi= eld is a variable sized array, so it can be a formal field.
      5. UNIT_TEST_SAVE_HEADER – Can the las= t 3 fields be changed to pointers and be formal fields?
      6. Do the structures really need to be packed?  Specially with the changes suggested above?  Is the intent to potentially share data stored on disk between diff= erent host execution environments that may have different width architectur= es?
    8. UnitTestPkg – UnitTestLib.h
      1. Can you provide mo= re context for the APIs SaveFrameworkState(), SaveFrameworkStateAndQuit(), SaveFrameworkStateAndReboot(), SetFrameworkBootNextDevice().  I do not see any Load() functions, so how would a set of tests be r= esumed?  If these do not apply to host based tests, should these be split ou= t to a different lib class?
    9. UnitTestPkg/Library/UnitTestLib
      1. I see an implement= ation of MD5.  We should not do our own.  We should use an approved implementation such as OpenSSL.
    10. UnitTestPkg/Library/UnitTestTerminationLibTbd<= o:p>
      1. Do we really need = this lib instance now?

    <= o:p> 

    T= hanks,

    <= o:p> 

    M= ike

    <= o:p> 

    From: devel@edk2.groups.io <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 land= ed in edk2/master, we'd like to get the basic framework for unittesting sta= ged as well. Target intercept date would be immediately after the 2019/11 s= tabilization, 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/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 no= t explicitly firmware code, so it seems unnecessary). Maybe in a new edk2-t= est2 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 he= re: 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_E92EE9817A31E24EB0585FDF735412F5B9E25DDAORSMSX113amrcor_--