Skip to content

Address ao update test review comments#2068

Closed
suraj-markup wants to merge 2 commits into
AgentWrapper:mainfrom
suraj-markup:codex/address-update-test-review
Closed

Address ao update test review comments#2068
suraj-markup wants to merge 2 commits into
AgentWrapper:mainfrom
suraj-markup:codex/address-update-test-review

Conversation

@suraj-markup
Copy link
Copy Markdown
Contributor

Summary

  • replaces inline Windows skip guards in update-script tests with the shared isWindows() helper
  • asserts the root-detection smoke test status before reading the command log so failures show useful stderr

Tests

  • pnpm --filter @aoagents/ao-cli test -- update-script.test.ts update-ps1.test.ts update.test.ts
  • pnpm build
  • pnpm --filter @aoagents/ao-cli typecheck
  • pnpm lint (warnings only)

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 26, 2026

Greptile Summary

This PR tidies up the update-script test file in two ways: it replaces every inline process.platform === "win32" skip guard with the canonical isWindows() helper from @aoagents/ao-core, and it wraps the root-detection smoke test body in a try/finally block so the temp directory is always deleted even when the expect(result.status…).toBe(0) assertion throws.

  • Platform check cleanup: five it.skipIf(process.platform === "win32") calls are replaced with it.skipIf(isWindows()), satisfying the CROSS_PLATFORM guide's golden rule and making the checks centrally testable.
  • Cleanup safety: the root-detect test moves the status assertion before readFileSync, then wraps both assertions inside try/finally { rmSync(…) }, ensuring the temp dir is removed regardless of pass/fail.

Confidence Score: 5/5

Safe to merge — changes are confined to a single test file, replacing inline platform checks with the shared helper and adding a finally-guarded cleanup.

All five guard replacements correctly use isWindows() as required by the cross-platform guide. The try/finally in the root-detect test properly guarantees temp-dir cleanup on both assertion success and failure. No production code is touched.

No files require special attention.

Important Files Changed

Filename Overview
packages/cli/tests/scripts/update-script.test.ts Replaces five inline process.platform === "win32" guards with isWindows() per CROSS_PLATFORM.md, and wraps the root-detect test in try/finally so temp-dir cleanup is guaranteed even when the status assertion fails.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["it.skipIf(isWindows())"] --> B{Windows?}
    B -- Yes --> C[Test Skipped]
    B -- No --> D[spawnSync bash script]
    D --> E[try block]
    E --> F["expect(result.status).toBe(0)\n(fails fast with stderr/stdout msg)"]
    F -- fails --> G[throw assertion error]
    F -- passes --> H[readFileSync commandLog]
    H --> I["expect(commands).toContain(...)"]
    G --> J
    I --> J["finally: rmSync(tempRoot)"]
    J --> K[Temp dir cleaned up]
Loading

Reviews (2): Last reviewed commit: "Ensure update script test cleans temp di..." | Re-trigger Greptile

Comment thread packages/cli/__tests__/scripts/update-script.test.ts Outdated
@suraj-markup
Copy link
Copy Markdown
Contributor Author

Closing this accidental follow-up. The relevant review fixes were applied directly to #2058 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant