Repository navigation
test: add initial system monitor tests - #241
Conversation
Signed-off-by: Lakshya <lakshya08kumar@gmail.com>
ChristianHandy
left a comment
ChristianHandy
left a comment
There was a problem hiding this comment.
The three CI failures all have the same root cause. Each run fails on test_system_monitor.py::test_start_and_stop_polling_runs_a_thread with AssertionError at test_system_monitor.py:180 because event.wait(timeout=1) returns False. That means the thread/event expected by the polling lifecycle never fires within the timeout. The test is effectively telling us that the start/stop polling path is not completing correctly, so this is a real code issue in the system monitor thread logic rather than a CI/environment problem. All three jobs pass 823 tests and fail on this single same assertion, which indicates the issue is deterministic and reproducible.
Using AI as a coding assistant is fine by me, and you can keep using it that way. I just want to get the code away from AI-only code as much as I can.
If you have any questions, feel free to ask.
…ed (#245) ## Summary Adds a bot comment to every pull request, like Home Assistant's review comments, but without AI: it says whether the pull request can be merged and what it changes. It is created once and updated whenever *Pull request check*, *Tests* or *CodeQL* finishes. The comment shows: - a verdict: **Ready to merge** / **Waiting for checks** / **Not ready to merge** (failed required checks, merge conflicts, draft) - the changed files grouped by area (app code, web pages, tests, CI, dependencies, Android, docs, ...) - each required check with a link to its log - other failed checks - a short "how to fix" per failed check (e.g. the `/allow-ai <sha>` line for AI assistance) Separately, I turned on **automatic Copilot code review** with a repository ruleset ("Copilot code review", main, on every push, not on drafts). That's a setting, not part of this diff. ## Changes - `scripts/pr_summary.py`: builds and posts or updates the comment (GitHub API only, never the PR's code) - `.github/workflows/pr-summary.yml`: `workflow_run` trigger, runs from main with `pull-requests: write`, checks out main only; the head SHA goes through an env variable - `tests/test_pr_summary.py`: 8 tests (areas, re-runs, verdicts, rendering, create vs. update of the comment, outdated commits) - `CONTRIBUTING.md`, `CHANGELOG.md` `workflow_run` workflows only run from the default branch, so the comment won't appear on this pull request; it starts on the next ones after merging. ## Verification - `pytest tests/test_pr_summary.py tests/test_license_headers.py`: 8 passed; `pr_summary.py` 88 % covered - `ruff check`: passed - `zizmor --persona pedantic`: no findings (1 documented ignore: `dangerous-triggers`) - Rendered against the open PR #241 read-only: correct verdict, checks and hints ## AI assistance Prepared with Claude Code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: ChristianHandy <christianhandy180@gmail.com> Co-authored-by: ChristianHandy <christianhandy180@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
🟢 Ready to merge
Commit What this pull request changes3 files, +199 / -5 lines.
Changed files
Required checks
Other checks: 12 passed, 0 failed, 0 running. |
Signed-off-by: Lakshya <lakshya08kumar@gmail.com>
…FleetPilot into test/system-monitor
ChristianHandy
left a comment
ChristianHandy
left a comment
There was a problem hiding this comment.
Thanks @lucky08-exe, the Event-based stop in system_monitor.py looks good! Two things are needed before it can be merged:
-
Please move test_system_monitor.py into the tests/ folder (git mv test_system_monitor.py tests/). Since #208, pytest only collects tests from tests/, so right now CI doesn't run your tests at all. That's why "pytest (Python 3.13)" reports only 6 % of the changed lines as tested.
-
Please add a line to CHANGELOG.md under Unreleased, e.g. "System monitor: stopping the background poller takes effect immediately instead of after up to 60 seconds (#163)."
Then please runpython -m pytest -q tests/test_system_monitor.pyandruff check .and update the "How it was tested" section. Using AI to learn is fine; I'll handle the AI check once everything else is green.
Signed-off-by: Lakshya <lakshya08kumar@gmail.com>
Signed-off-by: Lakshya <lakshya08kumar@gmail.com>
|
/allow-ai c0fde48 AI used as a learning aid (declared); tests moved to tests/, changelog added, reviewed |
ChristianHandy
left a comment
ChristianHandy
left a comment
There was a problem hiding this comment.
Thanks @lucky08-exe, nicely done! Tests in tests/, changelog entry, and the Event-based stop all look good. Merging.
What changes for users
Nothing visible. Adds automated tests for
system_monitor.pyto verify database initialization and handling of an unavailablepsutildependency.Closes #163
How it was tested
python -m pytest -q tests/test_system_monitor.py --basetemp .pytest-tmp-system-monitor— 8 passed.ruff check .— all checks passed.psutil, missing frequency/temperature readings, query APIs, retention pruning, and polling start/stop behavior.Checklist
python -m pytest -qandruff check .pass; coverage has not dropped.git commit -s, using the commit author's email address (DCO).CHANGELOG.mdupdated under Unreleased if users notice the change.docs/user-guide.md, anddocs/de/benutzerhandbuch.mdif possible).AI assistance
I'm just getting started with open source and still learning how to write proper tests. I used AI as a learning assistant to understand the existing code, break down the requirements, and approach testing step by step. I worked on this contribution with AI guidance, writing tests while learning the reasoning behind them. I did my best with what I know so far, so if I've missed anything or there are things that could be done better, I'd really appreciate your feedback and guidance. I'm happy to make any changes needed!