🧪 Add test for workspace setup report generation#178
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
Adds isolated unit-test coverage for src.setup.run_setup, ensuring SetupReport aggregates workspace setup, prefetch results, and deferred-init output correctly without relying on external state.
Changes:
- Introduces
tests/test_setup.pywith a happy-path unit test forrun_setup. - Mocks prefetch side effects (
start_mdm_raw_read,start_keychain_prefetch,start_project_scan) plusbuild_workspace_setupandrun_deferred_init. - Asserts the resulting
SetupReportfields and verifies mocked calls/arguments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from src.setup import run_setup, WorkspaceSetup | ||
| from src.prefetch import PrefetchResult | ||
| from src.deferred_init import DeferredInitResult | ||
|
|
| mock_workspace = WorkspaceSetup( | ||
| python_version="3.9.0", | ||
| implementation="CPython", | ||
| platform_name="Linux" |
| plugin_init=False, | ||
| skill_init=False, | ||
| mcp_prefetch=False, | ||
| session_hooks=False |
There was a problem hiding this comment.
Code Review
This pull request introduces a new test file, tests/test_setup.py, which utilizes mocks to verify that the run_setup function correctly aggregates data into a report. The feedback suggests expanding the test suite to cover default parameter values for trusted and cwd, as well as adding a test for the SetupReport.as_markdown method to ensure proper output formatting.
| @mock.patch('src.setup.start_mdm_raw_read') | ||
| @mock.patch('src.setup.start_keychain_prefetch') | ||
| @mock.patch('src.setup.start_project_scan') | ||
| def test_run_setup_generates_report( |
There was a problem hiding this comment.
The test currently only covers the case where trusted is explicitly set to False and a cwd is provided. To ensure comprehensive coverage as stated in the PR description, consider adding test cases for the default values of these parameters (i.e., trusted=True and cwd=None). This would verify that the logic for calculating the default project root and the default trust state is correct. Additionally, consider adding a test for the SetupReport.as_markdown method to ensure the report is formatted correctly.
There was a problem hiding this comment.
@gemini-code-assist, what are the steps and line numbers to make this fix?
There was a problem hiding this comment.
To address the feedback, you should add two new test methods to the SetupTests class in tests/test_setup.py. These tests will verify the default parameter behavior and the markdown generation logic.
Steps:
- Add
test_run_setup_defaults: This test verifies thatrun_setup()correctly defaults totrusted=Trueandcwd=Nonewhen no arguments are provided. - Add
test_setup_report_as_markdown: This test ensures that theas_markdownmethod produces the expected string output for aSetupReportinstance.
These should be added after the existing test_run_setup_generates_report method (around line 76).
| def test_run_setup_generates_report( | |
| def test_run_setup_defaults(self, mock_start_project_scan, *args) -> None: | |
| run_setup() | |
| mock_start_project_scan.assert_called_once_with(None) | |
| def test_setup_report_as_markdown(self) -> None: | |
| from src.setup import SetupReport | |
| report = SetupReport(cwd=Path('/test'), trusted=True, setup=mock.MagicMock(), deferred_init=mock.MagicMock(), prefetches=()) | |
| self.assertIsInstance(report.as_markdown(), str) | |
| self.assertIn('###', report.as_markdown()) |
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
@claude code review |
|
@copilot code review |
Agent-Logs-Url: https://github.com/badMade/claw-code/sessions/85e5b7f4-43a4-42ee-a3a6-f0c46ab1c5fe Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
Addressed in 6382507: applied the requested formatting cleanups in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.github/workflows/auto-merge.yml:42
- The
auto-mergeworkflow no longer performs any merge/enable-automerge action and ends right after a check-run polling loop. As written, this job will succeed without merging anything, effectively disabling auto-merge (and the PR description doesn’t mention changing CI behavior). Restore the missing steps (and consider also validating check conclusions, not juststatus === 'completed').
- name: Wait for checks to complete
uses: actions/github-script@v7
id: wait-checks
with:
script: |
const pr = context.payload.pull_request;
const owner = context.repo.owner;
const repo = context.repo.repo;
// Wait up to 5 minutes for checks to complete
for (let i = 0; i < 10; i++) {
await new Promise(r => setTimeout(r, 30000));
const { data: checkRuns } = await github.rest.checks.listForRef({
owner, repo, ref: pr.head.sha
});
if (checkRuns.check_runs.every(run => run.status === 'completed')) {
break;
}
}
| from src.setup import run_setup, WorkspaceSetup | ||
| from src.prefetch import PrefetchResult | ||
| from src.deferred_init import DeferredInitResult | ||
|
|
||
|
|
||
| class SetupTests(unittest.TestCase): |
🎯 What: Added a comprehensive unit test for
run_setupinsrc/setup.py.📊 Coverage: The new test covers the happy path for the generation of
SetupReport. It mocks the internal dependencies including the prefetch side effects (start_mdm_raw_read,start_keychain_prefetch,start_project_scan),build_workspace_setup, andrun_deferred_initto verify the outputs are aggregated properly and the arguments are passed correctly without requiring any external state.✨ Result: Enhanced the reliability of the workspace setup logic by adding isolated unit test coverage, ensuring future refactoring won't regress the
SetupReportinitialization data layout.PR created automatically by Jules for task 4846647906736791127 started by @badMade