15 split lrllamalua into focused testable modules#19
Merged
LostOne1000 merged 10 commits intoJul 18, 2026
Conversation
- Extract OllamaClient.lua for API communication and server validation - Extract ThumbnailService.lua for thumbnail export and Base64 encoding - Extract PromptBuilder.lua for user/system prompt assembly - Extract ResponseValidator.lua for API response parsing and validation - Extract MetadataService.lua for keyword CSV parsing - Refactor Common.lua to delegation layer that re-exports original names - Add Busted test suite with 62 tests across 5 spec files - Makefile delegates to run_tests.sh as authoritative runner - Test paths resolved dynamically via debug.getinfo for portability
The inline string.gmatch keyword loop in LrLlama.lua duplicated the logic already tested in MetadataService.parseKeywordCsv. Replace it with the Common delegation layer so production and tests share the same code path. - Export parseKeywordCsv from Common.lua as a MetadataService delegation - Replace 4-line inline parsing loop in LrLlama.lua with Common.parseKeywordCsv - Add parseKeywordCsv to Common.lua export checklist and delegation behavior test - Add production wiring assertion verifying LrLlama.lua calls the tested parser - Add Lua tests GitHub Actions workflow file 65 tests pass.
…coverage - Wrap OllamaClient module in createClient(deps) constructor for testability while maintaining backward compatibility (existing callers see no changes) - Expose defaultClient.new so tests can inject fake dependencies - Add 27 tests for getBaseUrl, fetchModels, makeModelItems, saveServerAndRefresh - Add 13 tests for generate() pipeline: success path, retry logic, error propagation, temp-file cleanup, prompt construction, validation delegation - All 104 tests pass; Lua syntax valid across plugin and test files Co-Authored-By: Claude <noreply@anthropic.com>
Extract SinglePhotoController.lua with pure workflow logic (state transitions for generate, save-server, save-metadata) that accepts all dependencies via injection. Create LlamaDialog.lua owning all LrView/LrBinding UI construction. Rewrite LrLlama.lua as a thin entry point that builds the client adapter and delegates to LlamaDialog. - Add 173 Busted unit tests across 9 spec files (0 failures, 0 errors) - Client adapter pattern maps Common delegation layer to stable OllamaClient interface expected by SinglePhotoController - Fix loadfile invocation bugs in LrLlama.lua and LlamaDialog.lua Co-Authored-By: Claude <noreply@anthropic.com>
…tibility layer Refactor Common.lua to use explicit wrapper functions instead of direct reference assignments, enabling test spies to observe delegation behavior. Add createCommon(deps) factory exposed as common.new() for dependency injection in tests. - Every compatibility export now has a behavioral delegation test with fake focused modules (27 tests, up from 12 shape-only assertions) - sendDataToApi forwards all 7 argument positions including nil intermediates - fetchAvailableModels passes exact prefs object identity to fetchModels - Error propagation verified for both wrapper functions - Mutation checks confirm tests catch dropped args, wrong prefs, and misrouted delegation Co-Authored-By: Claude <noreply@anthropic.com>
Add 33 behavioral tests for addKeywordsWithParent, getLlmKeywordsFromPhoto, and removeLlmKeywords using plain-Lua fake catalog, photo, and keyword objects. No real Lightroom SDK required. Coverage includes: - Invalid input guards (nil, non-table types) - Parent creation with exact argument verification - Child creation under exact llm parent object (identity assertions) - Empty/nil entry handling and ipairs truncation behavior - Partial child creation failure resilience - Parent creation failure error propagation - Keyword filtering with exact parent-name matching - pcall all-or-nothing error protection characterization - removeLlmKeywords removes only llm children by identity - Error semantics asymmetry documented (get uses pcall, remove does not) Mutation verification confirmed detection of 4 representative defects.
Lightroom's embedded Lua VM doesn't expose xpcall or debug.traceback. Replace the exception-handling wrapper in runWithCleanup() with pcall, which is available in the sandbox and still surfaces error messages with line numbers.
…dbox-safe patterns - Add event recording to fakes for ordering assertions - Fix string.find typos (string.findtostring → string.find(tostring(...))) - Fix logger fake calling convention (colon notation matches production :error, :warn) - Expand test coverage: retry timing, error preservation, cleanup ordering
Lightroom's LrTasks.startAsyncTask wraps task callbacks in protected calls. Nesting our own pcall inside that context prevented SDK functions (http.post, encodeBase64, etc.) from yielding to the Lightroom event loop, causing 'Yielding is not allowed within a C or metamethod call' errors. SDK functions return nil on error rather than throwing Lua exceptions, so the pcall was only catching theoretical user-code exceptions at the cost of breaking normal operation. Exceptions now propagate naturally to Lightroom's own async task error handler. - Simplified runWithCleanup to sequential operation + cleanup (no pcall) - Removed unused pack/unpack helpers - Removed activeLogger parameter (no longer needed without dual-failure logging) - Updated tests to expect exception propagation via pcall at test level
…steps Wrapping SDK calls (encodeBase64, http.post) in pcall breaks when those functions yield internally — Lightroom's non-blocking I/O crosses the protected boundary and pcall returns early, silently swallowing real results. Tests passed because mocks don't yield. Instead, inline explicit cleanup at every exit point: - SDK calls run unwrapped; nil-return paths trigger best-effort cleanup via tryCleanup() helper (pcall-wrapped LrFileUtils.delete, which is local disk I/O and safe to protect) - Pure-Lua steps (prompt builder, JSON encode) remain pcall-safe since they cannot yield - Error preservation: operation error always primary on failure paths; cleanup error becomes primary only on the success path Trade-off: if an SDK function genuinely throws (not yields or returns nil), cleanup is skipped. This is a true SDK crash — far better than breaking the happy path for every generate call. Changes: - Remove pack/unpack/runWithCleanup (~50 lines) - Add tryCleanup helper for error-path cleanup - Inline executePost into generate() with explicit cleanup at each step - Update tests 3 and 8 to expect SDK exception propagation - Replace encode-throw/http-post-throw in meta-test with prompt-throw/ json-throw (pure-Lua exceptions that DO trigger cleanup)
8 tasks
LostOne1000
commented
Jul 18, 2026
LostOne1000
deleted the
15-split-lrllamalua-into-focused-testable-modules
branch
July 18, 2026 11:55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolve changes from #15 .