Patina Dxe Core: Add a default environment based test logger

[vineelko]

Description

Since Patina relies on the log::* API for logging, not configuring a default
logger can cause log::* calls to appear completely uncovered in llvm-cov
reports, slightly reducing overall coverage. Adding a simple environment based
logger avoids this issue. Although this change enables coverage for log::*,
there are still cases that cannot be covered. For example, in the function
below, there is no way to get coverage for lines 6 and 7.

1   1   fn print() {
2   1       let mut i = 0;
3   11      for _ in 0..10 {
4   10          log::info!(
5   10              "This will be covered {} {}",
6                   i,
7                   "This and the above line will not be covered."
8               );
9   10          i = i + 1;
10          }
11  1   }

The environment logger can be enabled as shown below:
To enable logging, set the RUST_LOG environment variable to the desired
log level (e.g., debug, info, warn, error) before running the tests.

$ RUST_LOG=debug cargo test -p patina_dxe_core allocator::usage_tests::uefi_memory_map -- --nocapture
or
PS:> $env:RUST_LOG="debug"; cargo test -p patina_dxe_core allocator::usage_tests::uefi_memory_map -- --nocapture

For cargo make coverage a corresponding Makefile.toml change will be added
from patina-devops

[tasks.test]
description = "Run tests and collect coverage data without generating reports."
env = { "RUST_LOG" = "info" }  <---
...

How This Was Tested

cargo make all

Integration Instructions

NA

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[os-d]

By default this won't log anything, right? Only if you pass --no-capture will the log have it? I'm worried about cluttering the log and slowing down tests if we do actual logging.

[Javagedes]

Are you sure we actually need a real logger here? I think we just need to make sure the level conditions are correct so that the if statement in the expanded macro does not bail out. I kind of think that just involves setting the max_level() correctly maybe? This is what the expanded log macros come out to:

{
    let lvl = ::log::Level::Error;
    if lvl <= ::log::STATIC_MAX_LEVEL
        && lvl <= ::log::max_level()
    {
        ::log::__private_api::log(
            { ::log::__private_api::GlobalLogger },
            format_args!(
                "Error freeing previous MAT pointer: {0:#X?}",
                err,
            ),
            lvl,
            &(
                "patina_dxe_core::config_tables::memory_attributes_table",
                "patina_dxe_core::config_tables::memory_attributes_table",
                ::log::__private_api::loc(),
            ),
            (),
        );
    }
}

Overall, I'm not against having a real logger, but realistically it should be unnecessary with the debugger on a host based system.

[vineelko]

By default this won't log anything, right? Only if you pass --no-capture will the log have it? I'm worried about cluttering the log and slowing down tests if we do actual logging.

That's correct. No logs without --nocapture

[vineelko]

Are you sure we actually need a real logger here? I think we just need to make sure the level conditions are correct so that the if statement in the expanded macro does not bail out. I kind of think that just involves setting the max_level() correctly maybe? This is what the expanded log macros come out to:

{
    let lvl = ::log::Level::Error;
    if lvl <= ::log::STATIC_MAX_LEVEL
        && lvl <= ::log::max_level()
    {
        ::log::__private_api::log(
            { ::log::__private_api::GlobalLogger },
            format_args!(
                "Error freeing previous MAT pointer: {0:#X?}",
                err,
            ),
            lvl,
            &(
                "patina_dxe_core::config_tables::memory_attributes_table",
                "patina_dxe_core::config_tables::memory_attributes_table",
                ::log::__private_api::loc(),
            ),
            (),
        );
    }
}

Overall, I'm not against having a real logger, but realistically it should be unnecessary with the debugger on a host based system.

I still think there is value in have a real logger because it helps diagnose individual tests using --nocapture and especially since during normal test execution this does not make any different wrt to the output. For more granular control on the log level may be we can switch the real logger to be based on env_logger. That way we can set the var appropriately along with --nocapture