Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
259 changes: 259 additions & 0 deletions tests/test_hooks.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,259 @@
from __future__ import annotations

import pathlib
import typing
from importlib.metadata import EntryPoint
from unittest.mock import MagicMock, Mock, patch

import pytest
from packaging.requirements import Requirement

from fromager import hooks


@pytest.fixture(autouse=True)
def _clear_hook_cache() -> typing.Generator[None, None, None]:
hooks._mgrs.clear()
yield
hooks._mgrs.clear()


def _make_fake_ext(plugin: typing.Callable[..., typing.Any]) -> Mock:
ext = Mock()
ext.plugin = plugin
return ext


def test_die_on_plugin_load_failure_raises() -> None:
ep = EntryPoint(name="bad_plugin", value="some.module:func", group="fromager.hooks")
original_err = ImportError("no such module")

with pytest.raises(RuntimeError, match="bad_plugin") as exc_info:
hooks._die_on_plugin_load_failure(
mgr=Mock(),
ep=ep,
err=original_err,
)

assert exc_info.value.__cause__ is original_err


@patch("fromager.hooks.hook.HookManager")
def test_get_hooks_creates_manager(mock_hm_cls: Mock) -> None:
fake_mgr = MagicMock()
fake_mgr.names.return_value = ["my_hook"]
mock_hm_cls.return_value = fake_mgr

result = hooks._get_hooks("post_build")

mock_hm_cls.assert_called_once_with(
namespace="fromager.hooks",
name="post_build",
invoke_on_load=False,
on_load_failure_callback=hooks._die_on_plugin_load_failure,
)
assert result is fake_mgr


@patch("fromager.hooks.hook.HookManager")
def test_get_hooks_returns_cached(mock_hm_cls: Mock) -> None:
fake_mgr = MagicMock()
fake_mgr.names.return_value = ["my_hook"]
mock_hm_cls.return_value = fake_mgr

first = hooks._get_hooks("post_build")
second = hooks._get_hooks("post_build")

mock_hm_cls.assert_called_once()
assert first is second


@patch("fromager.hooks._get_hooks")
def test_run_post_build_hooks_exception_propagates(mock_get: Mock) -> None:
def bad_plugin(**kwargs: typing.Any) -> None:
raise ValueError("hook failed")

fake_mgr = MagicMock()
fake_mgr.names.return_value = ["bad"]
fake_mgr.__iter__ = lambda self: iter([_make_fake_ext(bad_plugin)])
mock_get.return_value = fake_mgr
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MagicMock() / .names.return_value / .__iter__ code pattern is copy-pasted six times lines 76–79, 99–101, 133–135, 156–158, 187–189, 208–210) in this file. If the hook iteration mechanism changes, all six blocks need updating. Extract a helper like _make_fake_mgr(plugins) that returns a ready-to-use mock manager


with pytest.raises(ValueError, match="hook failed"):
hooks.run_post_build_hooks(
ctx=Mock(),
req=Requirement("pkg"),
dist_name="pkg",
dist_version="1.0",
sdist_filename=pathlib.Path("/tmp/a.tar.gz"),
wheel_filename=pathlib.Path("/tmp/a.whl"),
)


@patch("fromager.hooks._get_hooks")
def test_run_post_build_hooks_calls_plugin(mock_get: Mock) -> None:
called_with: dict[str, typing.Any] = {}

def fake_plugin(**kwargs: typing.Any) -> None:
called_with.update(kwargs)

fake_mgr = MagicMock()
fake_mgr.names.return_value = ["my_hook"]
fake_mgr.__iter__ = lambda self: iter([_make_fake_ext(fake_plugin)])
mock_get.return_value = fake_mgr

ctx = Mock()
req = Requirement("numpy>=1.0")
sdist = pathlib.Path("/tmp/numpy-1.0.tar.gz")
wheel = pathlib.Path("/tmp/numpy-1.0-cp312-linux_x86_64.whl")

hooks.run_post_build_hooks(
ctx=ctx,
req=req,
dist_name="numpy",
dist_version="1.0",
sdist_filename=sdist,
wheel_filename=wheel,
)

mock_get.assert_called_once_with("post_build")
assert called_with["ctx"] is ctx
assert called_with["req"] is req
assert called_with["dist_name"] == "numpy"
assert called_with["dist_version"] == "1.0"
assert called_with["sdist_filename"] is sdist
assert called_with["wheel_filename"] is wheel


@patch("fromager.hooks._get_hooks")
def test_run_post_bootstrap_hooks_exception_propagates(mock_get: Mock) -> None:
def bad_plugin(**kwargs: typing.Any) -> None:
raise ValueError("hook failed")

fake_mgr = MagicMock()
fake_mgr.names.return_value = ["bad"]
fake_mgr.__iter__ = lambda self: iter([_make_fake_ext(bad_plugin)])
mock_get.return_value = fake_mgr

with pytest.raises(ValueError, match="hook failed"):
hooks.run_post_bootstrap_hooks(
ctx=Mock(),
req=Requirement("pkg"),
dist_name="pkg",
dist_version="1.0",
sdist_filename=None,
wheel_filename=None,
)


@patch("fromager.hooks._get_hooks")
def test_run_post_bootstrap_hooks_calls_plugin(mock_get: Mock) -> None:
called_with: dict[str, typing.Any] = {}

def fake_plugin(**kwargs: typing.Any) -> None:
called_with.update(kwargs)

fake_mgr = MagicMock()
fake_mgr.names.return_value = ["my_hook"]
fake_mgr.__iter__ = lambda self: iter([_make_fake_ext(fake_plugin)])
mock_get.return_value = fake_mgr

ctx = Mock()
req = Requirement("flask>=2.0")

hooks.run_post_bootstrap_hooks(
ctx=ctx,
req=req,
dist_name="flask",
dist_version="2.0",
sdist_filename=None,
wheel_filename=None,
)

mock_get.assert_called_once_with("post_bootstrap")
assert called_with["ctx"] is ctx
assert called_with["req"] is req
assert called_with["dist_name"] == "flask"
assert called_with["dist_version"] == "2.0"
assert called_with["sdist_filename"] is None
assert called_with["wheel_filename"] is None


@patch("fromager.hooks._get_hooks")
def test_run_prebuilt_wheel_hooks_exception_propagates(mock_get: Mock) -> None:
def bad_plugin(**kwargs: typing.Any) -> None:
raise ValueError("hook failed")

fake_mgr = MagicMock()
fake_mgr.names.return_value = ["bad"]
fake_mgr.__iter__ = lambda self: iter([_make_fake_ext(bad_plugin)])
mock_get.return_value = fake_mgr

with pytest.raises(ValueError, match="hook failed"):
hooks.run_prebuilt_wheel_hooks(
ctx=Mock(),
req=Requirement("pkg"),
dist_name="pkg",
dist_version="1.0",
wheel_filename=pathlib.Path("/tmp/a.whl"),
)


@patch("fromager.hooks._get_hooks")
def test_run_prebuilt_wheel_hooks_calls_plugin(mock_get: Mock) -> None:
called_with: dict[str, typing.Any] = {}

def fake_plugin(**kwargs: typing.Any) -> None:
called_with.update(kwargs)

fake_mgr = MagicMock()
fake_mgr.names.return_value = ["my_hook"]
fake_mgr.__iter__ = lambda self: iter([_make_fake_ext(fake_plugin)])
mock_get.return_value = fake_mgr

ctx = Mock()
req = Requirement("torch>=2.0")
wheel = pathlib.Path("/tmp/torch-2.0-cp312-linux_x86_64.whl")

hooks.run_prebuilt_wheel_hooks(
ctx=ctx,
req=req,
dist_name="torch",
dist_version="2.0",
wheel_filename=wheel,
)

mock_get.assert_called_once_with("prebuilt_wheel")
assert called_with["ctx"] is ctx
assert called_with["req"] is req
assert called_with["dist_name"] == "torch"
assert called_with["dist_version"] == "2.0"
assert called_with["wheel_filename"] is wheel
assert "sdist_filename" not in called_with


@patch("fromager.hooks.overrides._get_dist_info", return_value=("mypkg", "1.0.0"))
@patch("fromager.hooks.extension.ExtensionManager")
def test_log_hooks_logs_each_extension(
mock_em_cls: Mock,
mock_dist_info: Mock,
) -> None:
ext_a = Mock()
ext_a.name = "post_build"
ext_a.module_name = "my_plugins.hooks"

ext_b = Mock()
ext_b.name = "post_bootstrap"
ext_b.module_name = "other_plugins.hooks"

mock_em_cls.return_value = [ext_a, ext_b]

hooks.log_hooks()

mock_em_cls.assert_called_once_with(
namespace="fromager.hooks",
invoke_on_load=False,
on_load_failure_callback=hooks._die_on_plugin_load_failure,
)
assert mock_dist_info.call_count == 2
mock_dist_info.assert_any_call("my_plugins.hooks")
mock_dist_info.assert_any_call("other_plugins.hooks")
Comment on lines +234 to +259
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

test_log_hooks_logs_each_extension does not verify log output.

log_hooks()’s core behavior is emitting per-extension log lines; the current assertions only check collaborator calls.

Suggested assertion of emitted logs
@@
 def test_log_hooks_logs_each_extension(
     mock_em_cls: Mock,
     mock_dist_info: Mock,
+    caplog: pytest.LogCaptureFixture,
 ) -> None:
@@
-    hooks.log_hooks()
+    with caplog.at_level("DEBUG", logger=hooks.logger.name):
+        hooks.log_hooks()
@@
     assert mock_dist_info.call_count == 2
     mock_dist_info.assert_any_call("my_plugins.hooks")
     mock_dist_info.assert_any_call("other_plugins.hooks")
+    messages = [r.getMessage() for r in caplog.records if r.name == hooks.logger.name]
+    assert any("loaded hook 'post_build': from my_plugins.hooks" in m for m in messages)
+    assert any("loaded hook 'post_bootstrap': from other_plugins.hooks" in m for m in messages)
As per coding guidelines: `tests/**: Verify test actually tests the intended behavior.`
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@patch("fromager.hooks.overrides._get_dist_info", return_value=("mypkg", "1.0.0"))
@patch("fromager.hooks.extension.ExtensionManager")
def test_log_hooks_logs_each_extension(
mock_em_cls: Mock,
mock_dist_info: Mock,
) -> None:
ext_a = Mock()
ext_a.name = "post_build"
ext_a.module_name = "my_plugins.hooks"
ext_b = Mock()
ext_b.name = "post_bootstrap"
ext_b.module_name = "other_plugins.hooks"
mock_em_cls.return_value = [ext_a, ext_b]
hooks.log_hooks()
mock_em_cls.assert_called_once_with(
namespace="fromager.hooks",
invoke_on_load=False,
on_load_failure_callback=hooks._die_on_plugin_load_failure,
)
assert mock_dist_info.call_count == 2
mock_dist_info.assert_any_call("my_plugins.hooks")
mock_dist_info.assert_any_call("other_plugins.hooks")
`@patch`("fromager.hooks.overrides._get_dist_info", return_value=("mypkg", "1.0.0"))
`@patch`("fromager.hooks.extension.ExtensionManager")
def test_log_hooks_logs_each_extension(
mock_em_cls: Mock,
mock_dist_info: Mock,
caplog: pytest.LogCaptureFixture,
) -> None:
ext_a = Mock()
ext_a.name = "post_build"
ext_a.module_name = "my_plugins.hooks"
ext_b = Mock()
ext_b.name = "post_bootstrap"
ext_b.module_name = "other_plugins.hooks"
mock_em_cls.return_value = [ext_a, ext_b]
with caplog.at_level("DEBUG", logger=hooks.logger.name):
hooks.log_hooks()
mock_em_cls.assert_called_once_with(
namespace="fromager.hooks",
invoke_on_load=False,
on_load_failure_callback=hooks._die_on_plugin_load_failure,
)
assert mock_dist_info.call_count == 2
mock_dist_info.assert_any_call("my_plugins.hooks")
mock_dist_info.assert_any_call("other_plugins.hooks")
messages = [r.getMessage() for r in caplog.records if r.name == hooks.logger.name]
assert any("loaded hook 'post_build': from my_plugins.hooks" in m for m in messages)
assert any("loaded hook 'post_bootstrap': from other_plugins.hooks" in m for m in messages)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_hooks.py` around lines 231 - 256, Update the
test_log_hooks_logs_each_extension test to assert emitted log lines from
hooks.log_hooks(): use pytest's caplog (or patching the logger used in
fromager.hooks) to capture logs around the hooks.log_hooks() call and assert
that the captured records include messages referencing ext_a.module_name
("my_plugins.hooks") and ext_b.module_name ("other_plugins.hooks") (and
optionally the distribution info returned by _get_dist_info, i.e., "mypkg" and
"1.0.0"); keep the existing mocks (ExtensionManager and _get_dist_info) and
existing assertions about their calls, but add explicit assertions that the
logger produced one log entry per extension with the expected text.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not do this because I did not want to test brittle log output strings. The test instead just checks that _get_dist_info is called for each extension

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnadzam, fair point — log message strings are implementation details that can change independently of behavior, and asserting on them adds maintenance cost for little value. Verifying _get_dist_info is called per extension is a reasonable proxy that log_hooks() actually iterated and processed each one. I'll drop this suggestion.


✏️ Learnings added
Learnt from: mnadzam
Repo: python-wheel-build/fromager PR: 1034
File: tests/test_hooks.py:234-259
Timestamp: 2026-04-09T08:58:42.152Z
Learning: In fromager (python-wheel-build/fromager), the maintainer (mnadzam) prefers not to assert on log output strings in tests (e.g., tests/test_hooks.py) because they are considered brittle implementation details. Instead, tests should verify functional behavior such as checking that collaborator functions like `_get_dist_info` are called the expected number of times with the expected arguments.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: smoparth
Repo: python-wheel-build/fromager PR: 1030
File: tests/test_suggest_collection.py:5-5
Timestamp: 2026-04-08T22:13:20.914Z
Learning: In fromager CLI test files (e.g., tests/test_suggest_collection.py and other tests/test_*.py), it’s acceptable to extract JSON arrays from mixed stdout using the existing helper `_extract_json_from_output(output)` (implemented with `re.search`). This is an intentional project pattern driven by the shared `cli_runner` fixture using Click’s default `mix_stderr=True`, which can inline log/warning messages into `result.output`/`result.stdout`. Don’t flag this approach as fragile as long as the helper is used consistently and the extraction targets the expected JSON array format.

Loading