Add browser smoke harness#2
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new browser smoke harness in scripts/browser_smoke.py to validate the homepage, dashboard, and representative plot pages using standard library components. It also updates documentation and repository validation scripts to reflect this addition. Review feedback highlights a potential crash when using relative paths with as_uri(), suggests dynamically generating smoke targets from the manifest to improve resilience, and recommends decoupling HTML report tests from the repository's build state through mocking.
| args = parser.parse_args(argv) | ||
|
|
||
| results = run_smoke_checks(args.root) |
There was a problem hiding this comment.
The --root argument should be resolved to an absolute path before use. The Path.as_uri() method (called via run_smoke_checks -> evaluate_target -> _relative_url) raises a ValueError if the path is not absolute. If a user provides a relative path via the CLI (e.g., --root .), the script will crash.
| args = parser.parse_args(argv) | |
| results = run_smoke_checks(args.root) | |
| args = parser.parse_args(argv) | |
| args.root = args.root.resolve() | |
| results = run_smoke_checks(args.root) |
| SMOKE_TARGETS = [ | ||
| { | ||
| "name": "homepage", | ||
| "title": "Plots homepage", | ||
| "path": "index.html", | ||
| "checks": [ | ||
| 'data-page="home"', | ||
| "plots_manifest.json", | ||
| "published entries", | ||
| ], | ||
| }, | ||
| { | ||
| "name": "dashboard", | ||
| "title": "Unified Dashboard", | ||
| "path": "dashboard/index.html", | ||
| "checks": [ | ||
| "dashboard.css", | ||
| "dashboard.js", | ||
| "dashboard-container", | ||
| ], | ||
| }, | ||
| { | ||
| "name": "representative-plot", | ||
| "title": "AI Compute Timeline", | ||
| "path": "ai-compute-timeline/index.html", | ||
| "checks": [ | ||
| "output/ai_compute_timeline_interactive.html", | ||
| "output/ai_compute_timeline_highres.png", | ||
| "output/ai_compute_timeline.svg", | ||
| ], | ||
| }, | ||
| ] |
There was a problem hiding this comment.
The smoke targets and their associated content checks are currently hardcoded. Since the repository is manifest-driven, consider dynamically generating the targets (especially the representative-plot) by querying the manifest via manifest_utils.plot_entries. This would make the harness more resilient to directory renames or plot deletions in the future.
| def test_render_html_report_mentions_targets(self, repo_root: Path): | ||
| browser_smoke = _load_browser_smoke() | ||
|
|
||
| results = browser_smoke.run_smoke_checks(repo_root) | ||
| html = browser_smoke.render_html_report(results, repo_root) | ||
|
|
||
| assert "Browser smoke receipt" in html | ||
| assert "homepage" in html | ||
| assert "dashboard/index.html" in html | ||
| assert "ai-compute-timeline/index.html" in html | ||
| assert "PASS" in html |
There was a problem hiding this comment.
This test is an integration test that depends on the repository being in a 'built' state. If the build hasn't been run, the test will fail on the assert "PASS" in html line. To make the test suite more robust, consider splitting this into a unit test that uses mock result data to verify the HTML rendering logic, while keeping the integration check separate.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7edb551618
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| def _relative_url(root: Path, rel_path: str) -> str: | ||
| return (root / rel_path).as_uri() |
There was a problem hiding this comment.
Using Path.as_uri() on (root / rel_path) crashes when --root is passed as a relative path (for example --root .), because relative paths cannot be expressed as file URIs. In that scenario the script raises ValueError before any smoke checks run, so the CLI option documented as a repository-root override is unusable unless callers manually pass an absolute path.
Useful? React with 👍 / 👎.
1 participant
Goal:
What changed:
Verification:
Risk / notes:
Next loop command:
/qa and test feature in the web browser. take screen shots to make sure everything looks right. Update docs. Commit, push, and merge.