test: run all tests on a single fact instance - #1330
Conversation
Replace the per-test fact container lifecycle with a single session-scoped container that is kept alive across all tests. Between tests, the config file is rewritten and SIGHUP is sent to trigger hot-reload, avoiding the overhead of container create/start/stop/remove on every test. Key changes: - conftest.py: Make server, fact container, and config file session-scoped. Add per-test fact_config fixture that writes paths, drains stale events, and sends SIGHUP on setup, then restores baseline config on teardown with per-test log capture. - server.py: Add drain() method to clear events between tests. - reloader/mod.rs: Use mtime_nsec() instead of mtime() so that rapid config rewrites within the same second are detected. - test_rate_limit.py: Skip two tests that rely on cumulative metrics which are not isolatable with a session-scoped container. - Guard --output=all with pytest.exit() since session-scoped containers on network_mode=host cause port conflicts. Assisted-by: claude-opus-4-6 <noreply@opencode.ai>
Add prometheus-client dependency and a dedicated metrics.py module
that parses fact's OpenMetrics exposition format. MetricsSnapshot
supports fetching, diffing (before/after delta), and serialising
back to OpenMetrics text.
Key changes:
- metrics.py: MetricsSnapshot class with fetch/delta/to_text/get,
plus get_metric_value helper (moved from utils.py).
- conftest.py: fact_config fixture now captures a metrics snapshot
before each test and dumps the per-test delta to {logs_dir}/metrics.
- test_rate_limit.py: Remove skip decorators, use delta-based
assertions so cumulative counters from the session-scoped
container do not interfere.
- test_path_rmdir.py: Import get_metric_value from metrics instead
of utils.
- utils.py: Remove metrics code and unused requests import.
- requirements.txt: Add prometheus-client==0.26.0.
Assisted-by: claude-opus-4-6 <noreply@opencode.ai>
Move output-mode config (grpc/otlp) from the session-scoped base_config/fact_config_file/fact fixtures into the per-test fact_config fixture so the fact container no longer depends on a specific EventServer at startup. Re-enable --output=all by returning both modes instead of exiting. Use _fixtureinfo.argnames in pytest_generate_tests to only parametrize tests that directly request the server fixture. On teardown, clear paths but keep the same config instead of rebuilding from base_config with a new server reference. Assisted-by: claude-opus-4-6@default <noreply@opencode.ai>
📝 WalkthroughWalkthroughThe PR switches reload tracking to nanosecond timestamps, keeps one fact container alive across tests with SIGHUP-based configuration reloads, and introduces Prometheus metric snapshots for delta-based assertions. ChangesConfiguration reload integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pytest
participant fact_config
participant fact
participant MetricsSnapshot
Pytest->>fact_config: write test configuration
fact_config->>fact: send SIGHUP
fact-->>Pytest: reload configuration
Pytest->>MetricsSnapshot: fetch before metrics
Pytest->>fact: execute filesystem events
Pytest->>MetricsSnapshot: fetch after metrics
MetricsSnapshot-->>Pytest: return metric deltas
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## mauro/fix/cleanup-lpm #1330 +/- ##
=========================================================
+ Coverage 34.78% 35.16% +0.37%
=========================================================
Files 22 22
Lines 3300 3265 -35
Branches 3300 3265 -35
=========================================================
Hits 1148 1148
+ Misses 2147 2112 -35
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fact/src/config/reloader/mod.rs (1)
118-125: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a full timestamp consistently.
mtime_nsec()is only the nanosecond fraction, so storing it here drops the seconds component and can miss reloads on low-resolution filesystems; it also disagrees with themtime()value used when the cache is initialized. Keep one full timestamp representation in both places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fact/src/config/reloader/mod.rs` around lines 118 - 125, Update the metadata timestamp handling in the reloader path to use the same full timestamp representation as the cache initialization code, replacing the nanosecond-only `mtime_nsec()` value with the matching `mtime()` value. Preserve the existing error logging and continue behavior.
🧹 Nitpick comments (3)
tests/conftest.py (2)
268-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew
reload_facthelper isn't reused byrate_limited_configintests/test_rate_limit.py.
rate_limited_config(tests/test_rate_limit.py, lines 18-34) manually re-implements the write-config +fact.kill('SIGHUP')+sleep(0.1)sequence this helper now centralizes. Routing it throughreload_factwould avoid drift between the two implementations as reload semantics evolve.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 268 - 278, Update rate_limited_config in tests/test_rate_limit.py to use the existing reload_fact helper for writing the configuration, sending SIGHUP to the container, and applying the reload delay. Remove its duplicated reload sequence while preserving the current arguments and behavior.
95-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReliance on private pytest
_fixtureinfoattribute.
metafunc.definition._fixtureinfo.argnamesaccesses a pytest-internal, underscore-prefixed attribute rather than public API, so it's not guaranteed to remain stable across pytest releases. Consider deriving the direct test-function argnames viainspect.signature(metafunc.function).parametersinstead, which achieves the same "direct-only" distinction documented in theserverfixture's docstring without touching internals.Please verify whether
pytest8.4.1 exposes a stable public API for the direct (non-transitive) fixture argnames of a test function, or whether_fixtureinforemains the only option.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 95 - 98, Replace the private metafunc.definition._fixtureinfo.argnames access in pytest_generate_tests with direct parameter names obtained from inspect.signature(metafunc.function).parameters, preserving the check that only directly declared server arguments trigger indirect parametrization. Do not rely on pytest internals; use the public function signature approach unless a verified pytest 8.4.1 public API provides the same direct-only behavior.tests/metrics.py (1)
36-40: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd request timeouts to local
requests.getcalls. All three sites callrequests.get()against the fact container's HTTP endpoints without atimeout=, so an unresponsive container (e.g. mid-restart) would hang the call indefinitely instead of failing fast.
tests/metrics.py#L36-L40: add a timeout torequests.get(f'http://{endpoint}/metrics')inMetricsSnapshot.fetch.tests/conftest.py#L230-L237: add a timeout to the health-checkrequests.get(f'http://{ENDPOINT_ADDRESS}/health_check')call.tests/conftest.py#L246-L252: add a timeout to the session-teardownrequests.get(f'http://{ENDPOINT_ADDRESS}/metrics')call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/metrics.py` around lines 36 - 40, Add finite request timeouts to all three local HTTP calls: update MetricsSnapshot.fetch in tests/metrics.py, the health-check request in tests/conftest.py lines 230-237, and the session-teardown metrics request in tests/conftest.py lines 246-252. Use a consistent timeout value so each requests.get call fails promptly when the fact container is unresponsive.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/conftest.py`:
- Around line 300-320: Make the setup metrics collection around
MetricsSnapshot.fetch(ENDPOINT_ADDRESS) consistent with the teardown path by
handling transient fetch exceptions and representing an unavailable snapshot as
None. Update the corresponding delta()/get() usage to safely handle None while
preserving normal metrics comparisons when both snapshots are available.
- Around line 172-182: Flush the NamedTemporaryFile stream after
yaml.dump(config, f) and before yielding f.name in the fixture, ensuring the
container reads the complete configuration when it starts. Keep the existing
temporary-file lifecycle and cleanup unchanged.
---
Outside diff comments:
In `@fact/src/config/reloader/mod.rs`:
- Around line 118-125: Update the metadata timestamp handling in the reloader
path to use the same full timestamp representation as the cache initialization
code, replacing the nanosecond-only `mtime_nsec()` value with the matching
`mtime()` value. Preserve the existing error logging and continue behavior.
---
Nitpick comments:
In `@tests/conftest.py`:
- Around line 268-278: Update rate_limited_config in tests/test_rate_limit.py to
use the existing reload_fact helper for writing the configuration, sending
SIGHUP to the container, and applying the reload delay. Remove its duplicated
reload sequence while preserving the current arguments and behavior.
- Around line 95-98: Replace the private
metafunc.definition._fixtureinfo.argnames access in pytest_generate_tests with
direct parameter names obtained from
inspect.signature(metafunc.function).parameters, preserving the check that only
directly declared server arguments trigger indirect parametrization. Do not rely
on pytest internals; use the public function signature approach unless a
verified pytest 8.4.1 public API provides the same direct-only behavior.
In `@tests/metrics.py`:
- Around line 36-40: Add finite request timeouts to all three local HTTP calls:
update MetricsSnapshot.fetch in tests/metrics.py, the health-check request in
tests/conftest.py lines 230-237, and the session-teardown metrics request in
tests/conftest.py lines 246-252. Use a consistent timeout value so each
requests.get call fails promptly when the fact container is unresponsive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Enterprise
Run ID: e41ac2b4-39d9-459e-b504-d7e79933523e
📒 Files selected for processing (8)
fact/src/config/reloader/mod.rstests/conftest.pytests/metrics.pytests/requirements.txttests/server.pytests/test_path_rmdir.pytests/test_rate_limit.pytests/utils.py
💤 Files with no reviewable changes (1)
- tests/utils.py
| cwd = os.getcwd() | ||
| config = base_config() | ||
| f = NamedTemporaryFile( # noqa: SIM115 | ||
| prefix='fact-config-', | ||
| suffix='.yml', | ||
| dir=cwd, | ||
| mode='w', | ||
| ) | ||
| yaml.dump(config, config_file) | ||
|
|
||
| yield config, config_file.name | ||
| with ( | ||
| open(os.path.join(logs_dir, 'fact.yml'), 'w') as f, | ||
| open(config_file.name) as r, | ||
| ): | ||
| f.write(r.read()) | ||
| config_file.close() | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def test_container( | ||
| request: pytest.FixtureRequest, | ||
| docker_client: docker.DockerClient, | ||
| monitored_dir: str, | ||
| ignored_dir: str, | ||
| ): | ||
| """ | ||
| Run a container for triggering events in. | ||
| """ | ||
| container = docker_client.containers.run( | ||
| 'quay.io/fedora/fedora:43', | ||
| detach=True, | ||
| tty=True, | ||
| volumes={ | ||
| ignored_dir: { | ||
| 'bind': '/mounted', | ||
| 'mode': 'z', | ||
| }, | ||
| monitored_dir: { | ||
| 'bind': '/unmonitored', | ||
| 'mode': 'z', | ||
| }, | ||
| }, | ||
| name='fedora', | ||
| ) | ||
| container.exec_run('mkdir /container-dir') | ||
|
|
||
| yield container | ||
| yaml.dump(config, f) | ||
| yield f.name | ||
| f.close() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Flush the config file before it's consumed by the container.
yaml.dump(config, f) writes into NamedTemporaryFile's buffered stream, but f is never flushed before yield f.name — the very path bind-mounted read-only into the fact container at startup (lines 218-222). If the write hasn't hit the OS buffer yet when Docker reads the file, the container can start with an empty/truncated config, breaking session bootstrap intermittently.
🐛 Proposed fix
yaml.dump(config, f)
+ f.flush()
yield f.name📝 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.
| cwd = os.getcwd() | |
| config = base_config() | |
| f = NamedTemporaryFile( # noqa: SIM115 | |
| prefix='fact-config-', | |
| suffix='.yml', | |
| dir=cwd, | |
| mode='w', | |
| ) | |
| yaml.dump(config, config_file) | |
| yield config, config_file.name | |
| with ( | |
| open(os.path.join(logs_dir, 'fact.yml'), 'w') as f, | |
| open(config_file.name) as r, | |
| ): | |
| f.write(r.read()) | |
| config_file.close() | |
| @pytest.fixture | |
| def test_container( | |
| request: pytest.FixtureRequest, | |
| docker_client: docker.DockerClient, | |
| monitored_dir: str, | |
| ignored_dir: str, | |
| ): | |
| """ | |
| Run a container for triggering events in. | |
| """ | |
| container = docker_client.containers.run( | |
| 'quay.io/fedora/fedora:43', | |
| detach=True, | |
| tty=True, | |
| volumes={ | |
| ignored_dir: { | |
| 'bind': '/mounted', | |
| 'mode': 'z', | |
| }, | |
| monitored_dir: { | |
| 'bind': '/unmonitored', | |
| 'mode': 'z', | |
| }, | |
| }, | |
| name='fedora', | |
| ) | |
| container.exec_run('mkdir /container-dir') | |
| yield container | |
| yaml.dump(config, f) | |
| yield f.name | |
| f.close() | |
| cwd = os.getcwd() | |
| config = base_config() | |
| f = NamedTemporaryFile( # noqa: SIM115 | |
| prefix='fact-config-', | |
| suffix='.yml', | |
| dir=cwd, | |
| mode='w', | |
| ) | |
| yaml.dump(config, f) | |
| f.flush() | |
| yield f.name | |
| f.close() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/conftest.py` around lines 172 - 182, Flush the NamedTemporaryFile
stream after yaml.dump(config, f) and before yielding f.name in the fixture,
ensuring the container reads the complete configuration when it starts. Keep the
existing temporary-file lifecycle and cleanup unchanged.
| test_start = time.time() | ||
|
|
||
| config = base_config() | ||
| if server.output_mode == 'otlp': | ||
| config['otel'] = {'endpoint': 'http://127.0.0.1:4318/v1/logs'} | ||
| else: | ||
| config['grpc'] = {'url': 'http://127.0.0.1:9999'} | ||
| config['paths'] = [ | ||
| monitored_dir, | ||
| f'{monitored_dir}/**/*', | ||
| '/mounted/**/*', | ||
| '/container-dir/**/*', | ||
| ] | ||
|
|
||
| server.drain() | ||
| reload_fact(fact, config, fact_config_file) | ||
|
|
||
| metrics_before = MetricsSnapshot.fetch(ENDPOINT_ADDRESS) | ||
|
|
||
| yield config, fact_config_file | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded metrics_before fetch is inconsistent with the guarded teardown fetch.
metrics_before = MetricsSnapshot.fetch(ENDPOINT_ADDRESS) at line 317 has no exception handling, while the analogous metrics_after fetch at teardown (lines 327-333) is wrapped in try/except Exception: pass. A transient failure reaching /metrics right after the SIGHUP-triggered reload would error out the autouse fixture and fail every test, whereas the same failure at teardown is silently swallowed. Consider wrapping the setup fetch symmetrically (or making both paths consistently strict) with None handling in delta()/get().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/conftest.py` around lines 300 - 320, Make the setup metrics collection
around MetricsSnapshot.fetch(ENDPOINT_ADDRESS) consistent with the teardown path
by handling transient fetch exceptions and representing an unavailable snapshot
as None. Update the corresponding delta()/get() usage to safely handle None
while preserving normal metrics comparisons when both snapshots are available.
2 participants
Description
Change how tests are run from having a fact container being spin up and down for each test to spinning one container and running the full set of tests on that instead. This gives a substantial improvement to the time it takes to run the full set of tests, since the tests themselves are usually pretty fast and most of the time was being used in creating and removing the fact container.
The handling of logs is done in a way that we should still be able to see relevant information for each individual test, but if that happens to fail we also have a session wide log with the entire output of the fact container. For metrics, a new
MetricsSnapshottype is added so we can still get a per-test metrics dump which is useful to understand when a test fails because we missed some event.Assisted-by: claude-opus-4-6 noreply@opencode.ai
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
CI should be enough.
Summary by CodeRabbit
Bug Fixes
Tests