🧪 Add dependency injection and tests for MustGetExe#103
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
This PR improves the testability of the shared MustGetExe helper in Lib/v2/ by allowing dependency injection of MsgBox/ExitApp, and adds tests to validate both the success and failure paths without terminating the test process.
Changes:
- Extend
MustGetExeto accept optional injectedMsgBoxandExitAppcallables for testing. - Add
MustGetExesuccess-path and failure-path coverage intests/FindExe_Test.ahk.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
Lib/v2/AHK_Common.ahk |
Adds optional mock hooks to avoid real MsgBox/ExitApp during tests. |
tests/FindExe_Test.ahk |
Adds coverage for MustGetExe returning the resolved exe and invoking failure handlers. |
| mockMsgBox(msg) { | ||
| mockState["msgBoxCalled"] := true | ||
| } | ||
|
|
||
| mockExitApp(code) { |
- Adds `mockMsgBox` and `mockExitApp` optional parameters to `MustGetExe` in `AHK_Common.ahk` to enable testability of the exit path without killing the test runner. - Adds comprehensive tests for `MustGetExe` to `FindExe_Test.ahk` covering both the successful lookup and the error/exit path. - Updates CI workflow to include retry logic for Chocolatey package installations. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
![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 updates the MustGetExe function in Lib/v2/AHK_Common.ahk to support dependency injection for MsgBox and ExitApp, enabling more robust unit testing. New tests were added to tests/FindExe_Test.ahk to cover success and failure scenarios. However, the test implementation contains AutoHotkey v2 syntax errors, specifically regarding the placement of function definitions and the structure of the try block, which need to be addressed using anonymous functions as suggested.
| mockMsgBox := (msg) => (mockState["msgBoxCalled"] := true) | ||
| mockExitApp := (code) => (mockState["exitAppCalled"] := true, mockState["exitCode"] := code) | ||
|
|
||
| ; Attempt to find a non-existent file with no fallbacks | ||
| try { | ||
| MustGetExe("definitely_nonexistent.exe", [], mockMsgBox, mockExitApp) | ||
| } | ||
|
|
||
| AssertEqual(1, mockState["msgBoxCalled"], "MustGetExe should call MsgBox on failure") | ||
| AssertEqual(1, mockState["exitAppCalled"], "MustGetExe should call ExitApp on failure") | ||
| AssertEqual(1, mockState["exitCode"], "MustGetExe should exit with code 1 on failure") | ||
|
|
||
| ; Final Results |
There was a problem hiding this comment.
This block contains two syntax errors for AutoHotkey v2:
- Function definitions inside control flow:
mockMsgBoxandmockExitAppare defined using thefunction name() { ... }syntax inside atryblock. In AHK v2, function definitions cannot be placed inside control flow statements. - Invalid
tryblock: Thetryblock at line 80 lacks a correspondingcatchorfinallyblock, which is a requirement in AHK v2. Furthermore, sinceMustGetExedoes not throw exceptions, thetryblock is unnecessary.
To fix this while keeping the logic within the test's execution flow, you can use anonymous functions (fat arrows) assigned to variables, which are allowed inside try blocks.
mockMsgBox := (msg) => (mockState["msgBoxCalled"] := true)
mockExitApp := (code) => (mockState["exitAppCalled"] := true, mockState["exitCode"] := code)
; Attempt to find a non-existent file with no fallbacks
MustGetExe("definitely_nonexistent.exe", [], mockMsgBox, mockExitApp)
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by kimi-k2.6-20260420 · 235,058 tokens |
Ven0m0
deleted the
test/mustgetexe-dependency-injection-15835316681249555530
branch
🎯 What: Adds testability and tests for the error handling path of
MustGetExewhich callsExitAppandMsgBox.📊 Coverage: Added test cases to
tests/FindExe_Test.ahkthat cover the successful path (returns executable string) and the failure path (calls mockedMsgBoxandExitAppwith correct parameters).✨ Result: Improved test coverage and reliability for core process utilities without needing a specialized runner to handle hard process exits.
PR created automatically by Jules for task 15835316681249555530 started by @Ven0m0