[wic,v3,09/10] tests/unit/test_bb_utils: test mkdirhier() and fix its missing errno import

mkdirhier() wraps os.makedirs() and, on OSError, re-checks the error to
decide whether to swallow a benign "already exists" race or re-raise:

    except OSError as e:
        if e.errno != errno.EEXIST or not os.path.isdir(directory):
            raise e

but the module never imports errno. The reference to errno.EEXIST is
only evaluated when os.makedirs() actually raises, so the defect is
invisible on the happy path and on the unexpanded-${} guard. The moment
a real OSError occurs -- a parent component that is a file, a permission
error, anything -- the handler itself raises NameError: name 'errno' is
not defined, masking the original error with a misleading one.

The fix is a one-line import. With errno available the handler behaves
as intended: an EEXIST on an existing directory is swallowed, while any
other OSError (and an EEXIST whose path is not a directory) propagates
unchanged.

This commit adds tests/unit/test_bb_utils.py, covering mkdirhier() end
to end:

  - the happy path (nested, single-level, idempotent, existing dir);
  - the unexpanded-${} guard (rejected, nothing created, and that a
    bare brace without a dollar is allowed);
  - the OSError handler: a real mkdir under a file component surfaces an
    OSError, an arbitrary OSError propagates with its errno intact, an
    EEXIST on an existing directory is swallowed, and an EEXIST on a
    regular file propagates.

The error-path tests fail with NameError without the import and pass
with it, so the test and the fix belong together in this one change.

AI-Generated: codex/claude-opus 4.8 (xhigh)
Signed-off-by: Trevor Woerner <twoerner@gmail.com>
---
changes in v3:
- switch the tests from tempfile.mkdtemp() to pytest's tmp_path
  fixture so each test's scratch directory is cleaned up instead of
  leaking under /tmp; no change to what is tested.
changes in v2:
- v1 recorded this bug with an xfail marker in one large commit;
  v2 drops the xfail, asserts the correct behaviour directly, and
  lands the one-line errno-import fix in this same commit so the
  test passes green.
---
 src/wic/bb/utils.py         |   1 +
 tests/unit/test_bb_utils.py | 116 ++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)
 create mode 100644 tests/unit/test_bb_utils.py

On Wed, 2026-07-01 at 03:40 -0400, Trevor Woerner via
lists.yoctoproject.org wrote:
> mkdirhier() wraps os.makedirs() and, on OSError, re-checks the error to
> decide whether to swallow a benign "already exists" race or re-raise:
> 
>     except OSError as e:
>         if e.errno != errno.EEXIST or not os.path.isdir(directory):
>             raise e
> 
> but the module never imports errno. The reference to errno.EEXIST is
> only evaluated when os.makedirs() actually raises, so the defect is
> invisible on the happy path and on the unexpanded-${} guard. The moment
> a real OSError occurs -- a parent component that is a file, a permission
> error, anything -- the handler itself raises NameError: name 'errno' is
> not defined, masking the original error with a misleading one.
> 
> The fix is a one-line import. With errno available the handler behaves
> as intended: an EEXIST on an existing directory is swallowed, while any
> other OSError (and an EEXIST whose path is not a directory) propagates
> unchanged.
> 
> This commit adds tests/unit/test_bb_utils.py, covering mkdirhier() end
> to end:
> 
>   - the happy path (nested, single-level, idempotent, existing dir);
>   - the unexpanded-${} guard (rejected, nothing created, and that a
>     bare brace without a dollar is allowed);
>   - the OSError handler: a real mkdir under a file component surfaces an
>     OSError, an arbitrary OSError propagates with its errno intact, an
>     EEXIST on an existing directory is swallowed, and an EEXIST on a
>     regular file propagates.
> 
> The error-path tests fail with NameError without the import and pass
> with it, so the test and the fix belong together in this one change.
> 
> AI-Generated: codex/claude-opus 4.8 (xhigh)
> Signed-off-by: Trevor Woerner <twoerner@gmail.com>
> ---
> changes in v3:
> - switch the tests from tempfile.mkdtemp() to pytest's tmp_path
>   fixture so each test's scratch directory is cleaned up instead of
>   leaking under /tmp; no change to what is tested.
> changes in v2:
> - v1 recorded this bug with an xfail marker in one large commit;
>   v2 drops the xfail, asserts the correct behaviour directly, and
>   lands the one-line errno-import fix in this same commit so the
>   test passes green.
> ---
>  src/wic/bb/utils.py         |   1 +
>  tests/unit/test_bb_utils.py | 116 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+)
>  create mode 100644 tests/unit/test_bb_utils.py
> 
> diff --git a/src/wic/bb/utils.py b/src/wic/bb/utils.py
> index 3750056ba563..0b40dd0c1d59 100644
> --- a/src/wic/bb/utils.py
> +++ b/src/wic/bb/utils.py
> @@ -1,6 +1,7 @@
>  """
>  Minimal subset of BitBake's bb.utils used by standalone wic.
>  """
> +import errno
>  import os
>  
>  # from bitbake/lib/bb/utils.py

Nitpick: It's better to fix the bug in a separate commit before the test
is added.

> diff --git a/tests/unit/test_bb_utils.py b/tests/unit/test_bb_utils.py
> new file mode 100644
> index 000000000000..aeca054109ff
> --- /dev/null
> +++ b/tests/unit/test_bb_utils.py
> @@ -0,0 +1,116 @@
> +r"""
> +Unit tests for wic.bb.utils -- a small standalone subset of BitBake's
> +bb.utils. mkdirhier() is a mkdir -p wrapper with two guards: an
> +unexpanded-${} check and an OSError handler that re-checks EEXIST.
> +"""

This doesn't need to be a raw string.

> +import errno
> +import os
> +import sys
> +import unittest.mock as mock
> +from pathlib import Path
> +
> +import pytest
> +
> +_SRC = Path(__file__).resolve().parent.parent.parent / "src"
> +if str(_SRC) not in sys.path:
> +    sys.path.insert(0, str(_SRC))
> +
> +from wic.bb.utils import mkdirhier
> +
> +
> +# ---------------------------------------------------------------------------
> +# Happy path
> +# ---------------------------------------------------------------------------
> +
> +class TestMkdirhierHappyPath:
> +    def test_creates_nested_directory(self, tmp_path):
> +        target = os.path.join(str(tmp_path), "a", "b", "c")
> +        mkdirhier(target)
> +        assert os.path.isdir(target)
> +
> +    def test_idempotent_on_existing_directory(self, tmp_path):
> +        target = os.path.join(str(tmp_path), "x")
> +        mkdirhier(target)
> +        # Second call must not raise (exist_ok=True).
> +        mkdirhier(target)
> +        assert os.path.isdir(target)
> +
> +    def test_single_level(self, tmp_path):
> +        target = os.path.join(str(tmp_path), "single")
> +        mkdirhier(target)
> +        assert os.path.isdir(target)
> +
> +    def test_existing_root_directory(self, tmp_path):
> +        # An already-existing directory (the tmpdir itself) is a no-op.
> +        mkdirhier(str(tmp_path))
> +        assert os.path.isdir(str(tmp_path))

Are we testing mkdirhier() or testing Python's os.makedirs()
implementation?

There's no need to re-test Python's standard library. We only need to
test the functionality we add in mkdirhier(), and probably a single
basic "can it create a directory" test.

> +
> +
> +# ---------------------------------------------------------------------------
> +# Unexpanded bitbake-variable guard
> +# ---------------------------------------------------------------------------
> +
> +class TestMkdirhierUnexpandedVar:
> +    def test_unexpanded_var_rejected(self):
> +        with pytest.raises(Exception) as ei:
> +            mkdirhier("/tmp/${WORKDIR}/x")
> +        assert "unexpanded bitbake variable" in str(ei.value)
> +
> +    def test_unexpanded_var_not_created(self, tmp_path):
> +        bad = os.path.join(str(tmp_path), "${VAR}", "sub")
> +        with pytest.raises(Exception):
> +            mkdirhier(bad)
> +        # Nothing should have been created.
> +        assert not os.path.exists(os.path.join(str(tmp_path), "${VAR}"))

This assert could be merged into the previous test.

> +
> +    def test_brace_without_dollar_is_allowed(self, tmp_path):
> +        # Only the literal '${' marker triggers the guard; a bare '{' is a
> +        # legal (if unusual) directory name.
> +        target = os.path.join(str(tmp_path), "plain{brace")
> +        mkdirhier(target)
> +        assert os.path.isdir(target)
> +
> +
> +# ---------------------------------------------------------------------------
> +# OSError handler
> +# ---------------------------------------------------------------------------
> +
> +class TestMkdirhierErrorPath:
> +    def test_oserror_under_a_file_component(self, tmp_path):
> +        # Create a regular file, then try to mkdir a subdir *under* it. On
> +        # Linux os.makedirs raises NotADirectoryError (an OSError subclass);
> +        # the handler must let it surface rather than swallow it.
> +        f = os.path.join(str(tmp_path), "afile")
> +        Path(f).write_text("x")
> +        target = os.path.join(f, "sub")
> +        with pytest.raises(OSError):
> +            mkdirhier(target)
> +
> +    def test_generic_oserror_propagates(self):
> +        # An arbitrary OSError (EACCES != EEXIST) must propagate unchanged.
> +        err = OSError()
> +        err.errno = errno.EACCES
> +        with mock.patch("wic.bb.utils.os.makedirs", side_effect=err):
> +            with pytest.raises(OSError) as ei:
> +                mkdirhier("/whatever/path")
> +        assert ei.value.errno == errno.EACCES
> +
> +    def test_eexist_on_existing_dir_is_swallowed(self, tmp_path):
> +        # An EEXIST OSError on a path that is already a directory is the
> +        # benign race the handler exists to absorb; it must not propagate.
> +        err = OSError()
> +        err.errno = errno.EEXIST
> +        with mock.patch("wic.bb.utils.os.makedirs", side_effect=err):
> +            mkdirhier(str(tmp_path))  # exists and is a dir -> swallowed, no raise
> +
> +    def test_eexist_on_existing_file_propagates(self, tmp_path):
> +        # EEXIST but the path is a regular file, not a directory: the
> +        # handler's isdir() re-check fails, so the error must propagate.
> +        f = os.path.join(str(tmp_path), "afile")
> +        Path(f).write_text("x")
> +        err = OSError()
> +        err.errno = errno.EEXIST
> +        with mock.patch("wic.bb.utils.os.makedirs", side_effect=err):
> +            with pytest.raises(OSError) as ei:
> +                mkdirhier(f)
> +        assert ei.value.errno == errno.EEXIST

Claude loves to write test cases like this, that test specific little
details of the current implementation. This needs to be re-framed, test
the expected behaviour not the implementation details.

And to take a step back - if mkdirhier() is imported from bitbake, why
add test cases here anyway? If bitbake is missing test coverage then
it's better to add it there and plan to keep src/wic/bb in sync with
any future changes in bitbake.

Best regards,