Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 4683 unit test unexpected exception may be seen as PASS condition #5345

Conversation

mdkinney
Copy link
Member

@mdkinney mdkinney commented Feb 4, 2024

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4683

The main bug is that EDK II CI can miss a unit test failure when
a unit test generates an exception. Addressing this one issue
required the following chain of changes to the UnitTestFrameworkPkg
that are included in this patch series.

New in v4

  • Moved GoogleTestLib.inf /EHsc --> EHs from Patch 2 ->Patch 3
    so the /EHsc change is not mixed with /MT changes in Patch 2.
  • Fixed typo in Patch 1 commit message

New in V3

  • Add EXPECT_THROW_MESSAGE() and ASSERT_THROW_MESSAGE() to
    GoogleTestLib.h to simplify unit test case statements testing for
    an ASSERT() for a specific trigger expressiom. Uncrustify format
    of EXPECT_THAT() with {stmt} make code harder to read. Adding
    macros makes the source content in unit test cases match the
    source style of EXPECT_THROW() statements.
  • Update GoogTest samples to use EXPECT_THROW_MESSAGE() instead of
    EXPECT_THAT().

New in V2

  • Include Throws() APIs in GoogleTestLib.h
  • Update GoogleTest samples to demonstrate use of EXPECT_THROW()
    and EXPECT_THAT() to test for more specific ASSERT() conditions
    including the ability to verify the specific ASSERT() expression
    that was triggered.

V1

  • For host-base VX20xx builds, use release builds by default to
    make sure unexpected exceptions are caught as a test failure.
    Using debug libraries may generate a popup window to prompt
    running a debugger, which is value for debug, but that mode
    does not generate an error in CI.
  • Update UNIT_TEST_DEBUG to enable use of VS20xx debug libraries
    to enable popup windows to run debugger.
  • Remove DebugLib.h internal macro name collision with windows.h
    that was introduced when release libraries are used by default.
  • Update MSFT and GCC flags to consistently enable structured
    exception handling so Google Test host-based unit tests that
    check for expected ASSERT() conditions can be properly detected.
    The additional benefit of this bug fix is that catching thrown
    exceptions is much faster than EXPECT_DEATH() tests. The gtest
    sample unit tests are updated to use ASSERT_ANY_THROW() instead
    of EXPECT_DEATH().
  • In order to catch C++ exceptions for google test, the
    UnitTestDebugAssertLib has to be split into a target version
    and a host version and had to be able to detect the difference
    between cmock host-based tests and gtest host-based tests. This
    required minor changes to the UnitTestLib to be able to use the
    UnitTestLib GetFrameworkHandle() API to make that determination.
    It is NULL for gtest and non-NULL for cmocka.
  • In order to test the unit test failing conditions, unit tests
    the fail on purpose and unit tests that generate exceptions are
    added. These are not added to CI because that would cause CI to
    always fail. Instead, they can be run to make sure that reports
    generated provide the right level of detail for a developer to
    quickly identify the source of the unit test failure or the
    source of the exception.

Cc: Michael Kubacki mikuback@linux.microsoft.com
Cc: Sean Brogan sean.brogan@microsoft.com
Cc: Liming Gao gaoliming@byosoft.com.cn
Cc: Zhiguang Liu zhiguang.liu@intel.com
Cc: Andrew Fish afish@apple.com
Cc: Leif Lindholm quic_llindhol@quicinc.com
Signed-off-by: Michael D Kinney michael.d.kinney@intel.com

@mdkinney mdkinney force-pushed the Bug_xxx_UnitTestFrameworkPkg_GoogleTest_UnexpectedAssert branch 7 times, most recently from c4e3cc8 to c7adb83 Compare February 9, 2024 20:26
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4683

When VS20xx host-based unit tests are built with debug
libraries a name collision occurs with the DebugLib.h
internal macro _DEBUG(). Rename this internal macro
to _DEBUGLIB_DEBUG() to address the name collision.

Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4683

Add /MT to MSFT CC_FLAGS to always use release libraries
when building host-based unit tests so any exceptions
generated during host-based test execution generate an
error message in stderr instead of a popup window.

Use /MTd when -D UNIT_TESTING_DEBUG is to use debug
libraries when building host-based unit tests so any
exceptions generated during host-based test execution
generate a popup window with option to attach a debugger.

Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4683

Update MSFT CC_FLAGS for host-based unit tests to use /EHs
instead of /EHsc to support building C functions with SEH
(Structured Exception Handling) enabled.  This is required to
build UnitTestDebugAssertLibHost.inf.

Update GCC CC_FLAGS for host-based unit tests to use -fexceptions
to support catching exceptions.

Update GoogleTestLib.h to include Throws() APIs that enable
unit tests to use EXPECT_THAT() to check for expected ASSERT()
conditions for a specific ASSERT() expression.

Update GCC CC_FLAGS to add --coverage for host-based builds
for all GCC tool chains.

Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4683

Update GetActiveFrameworkHandle() to remove ASSERT() and require
caller to check for NULL.

This allows GetActiveFrameworkHandle() to be used to determine if the
current host-based test environment is framework/cmocka or gtest. In
the framework/cmocka host-based environment GetActiveFrameworkHandle()
returns non-NULL. In the gtest host-based environment
GetActiveFrameworkHandle() returns NULL.

Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4683

Add an C++ implementation of UnitTestDebugAssert() API for
host-based environments. GoogleTest based environments throw
a C++ exception of type std::runtime_error when an ASSERT() is
triggered with a description that contains the filename, line
number, and the expression that triggered the ASSERT().

Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4683

Update GoogleTest samples to use EXPECT_ANY_THROW() instead
of ASSERT_DEATH(). ASSERT_DEATH() is a very slow method to
detect an expected ASSERT() condition. Throwing an exception
from ASSERT() and using EXPECT_ANY_THROW() is several orders
of magnitude faster.

Update GoogleTest sample with example of using EXPECT_THROW()
and EXPECT_THAT() to check for more specific ASSERT() conditions
that allow unit test cases to test functions that contain
more than one ASSERT() statement and verify that the expected
ASSERT() is the one that was actually triggered.

Update library mappings so target-based unit tests use
UnitTestDebugAssertLib.inf and host-based unit tests use
UnitTestDebugAssertLibHost.inf

Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4683

Add sample unit tests that always fail or generate unexpected
exceptions along with a new DSC file to build the unit tests
that always fail or generate unexpected exceptions. This can
be used to verify the log information on failures is accurate
and provides the correct information to determine the source
of the unit test failure.

Divide by zero is used to generate unexpected exceptions.  The
compiler warnings for divide by zero are disables for the unit
tests that generate divide by zero exceptions on purpose.

These tests are not added to CI because CI would always fail.

The UnitTestFrameworkPkg.ci.yaml file is updated to ignore the
INF files for host-based testing that always fail.

Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Sean Brogan <sean.brogan@microsoft.com>
Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Michael Kubacki <michael.kubacki@microsoft.com>
@mdkinney mdkinney force-pushed the Bug_xxx_UnitTestFrameworkPkg_GoogleTest_UnexpectedAssert branch from c7adb83 to c4242a4 Compare February 14, 2024 01:51
@mdkinney mdkinney changed the title Bug xxx unit test framework pkg google test unexpected assert Bug 4683 unit test unexpected exception may be seen as PASS condition Feb 14, 2024
@mdkinney mdkinney added the push Auto push patch series in PR if all checks pass label Feb 14, 2024
@mergify mergify bot merged commit a1c426e into tianocore:master Feb 14, 2024
125 checks passed
VivianNK added a commit to microsoft/mu_basecore that referenced this pull request Mar 1, 2024
## Description
Cherry pick from: tianocore/edk2#5345

- [ ] Impacts functionality?
- **Functionality** - Does the change ultimately impact how firmware
functions?
- Examples: Add a new library, publish a new PPI, update an algorithm,
...
- [ ] Impacts security?
- **Security** - Does the change have a direct security impact on an
application,
    flow, or firmware?
  - Examples: Crypto algorithm change, buffer overflow fix, parameter
    validation improvement, ...
- [ ] Breaking change?
- **Breaking change** - Will anyone consuming this change experience a
break
    in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo,
call
    a function in a new library class in a pre-existing module, ...
- [ ] Includes tests?
  - **Tests** - Does the change include any explicit test code?
  - Examples: Unit tests, integration tests, robot tests, ...
- [ ] Includes documentation?
- **Documentation** - Does the change contain explicit documentation
additions
    outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to
documentation
    on an a separate Web page, ...

## How This Was Tested

Local builds and CI tests ran.

## Integration Instructions

N/A

---------

Signed-off-by: Michael D Kinney <michael.d.kinney@intel.com>
Co-authored-by: Michael D Kinney <michael.d.kinney@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
push Auto push patch series in PR if all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant