feat: add comprehensive mock-based handler tests (85 tests)#232
feat: add comprehensive mock-based handler tests (85 tests)#232retran wants to merge 1 commit intomendixlabs:mainfrom
Conversation
AI Code ReviewLet me review this pull request based on the provided information and the review checklist. Summary of the PRThis PR adds 85 mock-based unit tests across 30 files for all testable handler functions in Review against the checklistOverlap & duplication
MDL syntax design
Full-stack consistency
Test coverage
Security & robustness
Scope & atomicity
Code quality
Bugs & correctness
What Looks Good
RecommendationThis PR appears to be high-quality test code that follows best practices. It adds comprehensive unit tests for the executor layer without modifying any production code. The shared test infrastructure is well-designed and will make future test writing easier. I recommend approving this PR. The PR depends on #229 (feature/context-isolation) which needs to be merged first, but that's a dependency issue rather than a problem with this PR itself. There are no critical issues, moderate issues, or minor issues to report based on the review checklist. The PR is focused, well-executed, and adds value through improved test coverage. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
retran
force-pushed
the
feature/mock-handler-tests
branch
from
e29f3d5 to
70ec21e
Compare
AI Code ReviewCritical IssuesNone found. Moderate IssuesNone found. Minor IssuesNone found. What Looks Good
RecommendationApprove. This PR significantly improves test coverage for the executor layer with high-quality, well-isolated test code that follows Go best practices. The test infrastructure is reusable and well-designed. Once dependency #229 is merged and this PR is rebased, it's ready for integration. No production code risks as this is test-only. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Adds broad mock-based unit test coverage for mdl/executor handlers by introducing shared mock/test helpers and many new handler-focused tests that call handlers directly via ExecContext + MockBackend (no production code changes).
Changes:
- Introduce shared test infrastructure (
mock_test_helpers_test.go) for buildingExecContext, container hierarchies, model factories, and assertion helpers. - Add mock-based unit tests for a wide range of SHOW/DESCRIBE and CREATE/DROP handler functions across executor commands.
- Refactor/expand enumeration handler tests to use the shared helpers and add describe/not-found cases.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| mdl/executor/mock_test_helpers_test.go | Shared helpers for building mock contexts, hierarchies, factories, and assertions used by the new tests. |
| mdl/executor/cmd_write_handlers_mock_test.go | Mock tests for write-oriented handlers (create/drop module/enumeration/entity/etc.). |
| mdl/executor/cmd_workflows_mock_test.go | Mock tests for workflow show/describe handlers. |
| mdl/executor/cmd_settings_mock_test.go | Mock tests for settings show/describe handlers. |
| mdl/executor/cmd_security_mock_test.go | Mock tests for security show/describe handlers (project/module/user/demo roles). |
| mdl/executor/cmd_rest_clients_mock_test.go | Mock tests for consumed REST client show/describe handlers. |
| mdl/executor/cmd_published_rest_mock_test.go | Mock tests for published REST service show/describe handlers. |
| mdl/executor/cmd_pages_mock_test.go | Mock tests for pages/snippets/layouts show handlers (incl. module filtering). |
| mdl/executor/cmd_odata_mock_test.go | Mock tests for OData client/service show + describe handlers. |
| mdl/executor/cmd_navigation_mock_test.go | Mock tests for navigation show/describe handlers. |
| mdl/executor/cmd_modules_mock_test.go | Mock test for modules listing (including document counting inputs). |
| mdl/executor/cmd_misc_mock_test.go | Mock test for version display handler. |
| mdl/executor/cmd_microflows_mock_test.go | Mock tests for microflow/nanoflow show + describe not-found cases. |
| mdl/executor/cmd_mermaid_mock_test.go | Mock test for mermaid domain model describe output. |
| mdl/executor/cmd_jsonstructures_mock_test.go | Mock test for JSON structures show handler. |
| mdl/executor/cmd_javascript_actions_mock_test.go | Mock tests for JS actions show/describe handlers. |
| mdl/executor/cmd_javaactions_mock_test.go | Mock tests for Java actions show/describe handlers. |
| mdl/executor/cmd_import_mappings_mock_test.go | Mock test for import mappings show handler. |
| mdl/executor/cmd_imagecollections_mock_test.go | Mock tests for image collections show/describe handlers. |
| mdl/executor/cmd_fragments_mock_test.go | Mock tests for fragments show handler (empty + non-empty). |
| mdl/executor/cmd_export_mappings_mock_test.go | Mock test for export mappings show handler. |
| mdl/executor/cmd_enumerations_mock_test.go | Refactor + expand enumeration tests (show filter, describe, not-found) using shared helpers. |
| mdl/executor/cmd_entities_mock_test.go | Mock tests for entities show handler (incl. module filtering). |
| mdl/executor/cmd_dbconnection_mock_test.go | Mock tests for DB connections show/describe handlers. |
| mdl/executor/cmd_datatransformer_mock_test.go | Mock tests for data transformer list/describe handlers. |
| mdl/executor/cmd_contract_mock_test.go | Documents why contract commands are not mock-tested (integration-test preferred). |
| mdl/executor/cmd_constants_mock_test.go | Mock tests for constants show/describe handlers (incl. empty + not-found). |
| mdl/executor/cmd_businessevents_mock_test.go | Mock tests for business event services show/describe + “not implemented” client output. |
| mdl/executor/cmd_associations_mock_test.go | Mock tests for associations show handler (incl. module filtering). |
| mdl/executor/cmd_agenteditor_mock_test.go | Mock tests for agent editor models/agents/KBs/MCP services show + describe handlers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AI Code ReviewLet me analyze this pull request review request carefully. The PR is titled "feat: add comprehensive mock-based handler tests (85 tests)" and it's adding unit tests for handler functions in
Now I need to review this against the checklist provided in CLAUDE.md. Let me go through the checklist sections: Overlap & duplication
This PR is adding tests, not new functionality. It's adding mock-based unit tests for existing handler functions. Since it's test-only code, there shouldn't be overlap with production functionality. The PR explicitly states "No production code changes — test-only files". Looking at the diff, I see:
The modifications to existing files appear to be related to the agent editor functionality (adding mock functions for Create/Delete operations), which seems necessary to support the new tests. Since this is adding tests for existing functionality, and the PR states it's test-only, there shouldn't be overlap with existing implementations in terms of functionality. However, I should check if the test infrastructure duplicates existing test helpers. Looking at the new From the PR description: "Introduce shared test infrastructure ( This seems to be new infrastructure specifically for these mock-based tests. Since it's in the test files and not production code, and the PR says it's introducing this infrastructure, it's likely not duplicating existing test helpers (though I should verify). Let me check if there are existing test helpers that might be duplicated. Actually, looking more carefully at the diff, I see that The PR states: "All tests use stdlib only (no testify dependency)" which is good practice. MDL syntax design (for new or modified MDL syntax)This section checks if the PR adds or modifies MDL syntax. Looking through the diff, I don't see any changes to MDL grammar files (
There are no changes to MDL syntax itself - no new keywords, no new statement types, etc. The PR is purely about adding tests. So this section doesn't apply. Full-stack consistency (for MDL features)This section checks if new MDL features are wired through the full pipeline. Since this PR is not adding any new MDL features (it's only adding tests), this section doesn't apply. Test coverage
This is the most relevant section for this PR. The PR is adding test files, so we need to check:
From the PR description:
Looking at some of the test files (like
I don't see any The tests appear to be testing the handler functions directly with mocked backends, which is a reasonable unit test approach. They're not integration tests (which would require a real .mpr file), but the PR explicitly states they're mock-based unit tests. The PR says: "Cover show/list/describe/create/drop commands for: enumerations, constants, entities, associations, modules, microflows, pages, security, navigation, settings, OData, REST, business events, workflows, java actions, JS actions, fragments, data transformers, DB connections, image collections, JSON structures, import/export mappings, agent editor, misc, mermaid, and write handlers" Looking at the diff, I see test files for many of these areas:
This seems to cover the areas mentioned in the PR. One thing to note: the PR mentions "write handlers" but I don't see specific test files for write operations (CREATE, ALTER, DROP). Let me check if those are covered. Looking at some test files:
Actually, in
So the Describe tests are implicitly testing the CREATE handlers because DESCRIBE outputs the CREATE statement that would create the element. This is a common pattern in MDL testing - DESCRIBE should output a valid CREATE statement that can recreate the element. Let me check if there are explicit tests for CREATE, ALTER, DROP operations. Looking through the test files, I see:
However, the DESCRIBE tests are effectively testing the CREATE path because they verify that the DESCRIBE output contains the expected CREATE statement. For ALTER and DROP, I don't see explicit tests in the visible portions, but let me check if they're implied. Actually, looking at the PR description again: "Cover show/list/describe/create/drop commands" It specifically mentions create/drop commands, so I would expect to see tests for those. Let me look for CREATE and DROP tests in the diff. In In
No explicit CREATE or DROP tests. However, the Similarly, in This suggests that the DESCRIBE tests are covering the CREATE path by verifying the DESCRIBE output. For DROP operations, I don't see explicit tests in the visible portions. Let me check if there are any DROP tests. Actually, looking at the agent editor test file ( However, in the mock backend (
And in This suggests that the tests might be exercising the CREATE and DROP paths through the mock function calls, even if they don't explicitly test SHOW DROP or similar. But to properly test DROP operations, we would need tests that:
Let me check if such tests exist. Looking through the visible parts of the diff, I don't see explicit tests for DROP operations. For example, in However, the PR states: "Cover show/list/describe/create/drop commands" This suggests that create/drop should be covered. Let me look more carefully. Actually, in the test files, the DESCRIBE tests are testing the CREATE path (by verifying DESCRIBE outputs CREATE statements), but I don't see explicit tests for the DROP path. Let me check one more thing: the PR says "All tests use stdlib only (no testify dependency)" and "Tests bypass This means the tests are calling the handler functions (like So for CREATE operations, they would be calling the execCreate* functions directly. Let me check if there are tests that do this. In
But I don't see tests that call However, To test the actual CREATE handler (the execCreate* function), we would need tests that:
Let me check if such tests exist. Looking at Is there a test that calls this directly? In the visible portion of Let me check if there are any tests that call exec* functions directly. Actually, looking at the imports in the test files, they import the executor package, so they could call the exec* functions. For example, in package executorAnd then functions like These appear to be functions in the executor package. Let me check what these functions are. In the executor package, there are likely files like:
And these functions would call the exec* functions. For example, To test the execCreate* function directly, we would need a test that:
Let me check if such tests exist in the diff. I don't see explicit tests for the exec* functions in the visible portions. However, looking at the PR description again: "Cover show/list/describe/create/drop commands for: ..." And the PR title: "add comprehensive mock-based handler tests" The "handler functions" likely refer to the exec* functions in Let me check the executor directory structure. From the project architecture in CLAUDE.md: So the handler functions are in The PR says: "Tests bypass This suggests that the tests are calling the handler functions (the exec* functions) directly, not going through the Executor struct's Execute method. Let me verify this with one of the test files. In func TestShowAssociations_Mock(t *testing.T) {
// ... setup ...
mb := &mock.MockBackend{
IsConnectedFunc: func() bool { return true },
ListModulesFunc: func() ([]*model.Module, error) { return []*model.Module{mod}, nil },
ListDomainModelsFunc: func() ([]*domainmodel.DomainModel, error) { return []*domainmodel.DomainModel{dm}, nil },
}
ctx, buf := newMockCtx(t, withBackend(mb))
assertNoError(t, showAssociations(ctx, ""))
// ...
}Here, Where is Let me check if there's a corresponding execShowAssociations function. Actually, looking at the pattern in other parts of the codebase, it's likely that:
For CREATE/ALTER/DROP commands, there would be exec* functions. Let me look for tests of CREATE operations. In In assertNoError(t, describeAgentEditorModel(ctx, ast.QualifiedName{Module: "M", Name: "GPT4"}))And then it checks that the output contains "CREATE MODEL". This suggests that It's not testing the execCreateModel function; it's testing the describe function. To test the execCreateModel function, we would need a test that:
Let me check if such tests exist. I don't see them in the visible portions of the diff for the agent editor tests. Let me check another area, like constants. In
But I don't see a test that calls execCreateConstant or execModifyConstant directly. However, looking at the imports in package executorAnd the functions Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
AI Code ReviewLet me analyze this PR review request. The PR is titled "feat: add comprehensive mock-based handler tests (85 tests)" and adds 85 mock-based unit tests across 30 files for all testable handler functions in Key points from the PR:
I need to review this against the checklist provided in CLAUDE.md. Let me go through the checklist items: Overlap & duplication
This PR is adding tests, not new functionality. It's adding mock-based unit tests for handler functions. There's no mention of proposals in the PR body, and the diff shows only test files and mock infrastructure changes. No production code changes, so no overlap with existing implementations in terms of functionality. MDL syntax design (for new or modified MDL syntax)If the diff adds or modifies MDL syntax (grammar rules, keywords, statement types), check:
This PR doesn't add or modify any MDL syntax. It's purely adding tests. The diff shows only test files and mock infrastructure changes. No changes to grammar, AST, visitor, or executor production code. Full-stack consistency (for MDL features)New MDL commands or language features must be wired through the full pipeline. If the diff adds any MDL feature, check:
Again, this PR doesn't add any MDL features. It's adding tests for existing executor handler functions. No changes to the full pipeline. Test coverage
This PR is specifically about improving test coverage. It adds 85 mock-based unit tests across 30 files for handler functions in Looking at the diff, I see test files like:
These appear to be testing the handler functions directly. The PR says "Tests bypass The PR also mentions "Introduce shared test infrastructure ( I don't see any obvious use of Security & robustness
This is test code, so security concerns are less relevant. The test code appears to be using mock backends and not dealing with real files, sockets, etc. in a way that would introduce security vulnerabilities. Scope & atomicity
The PR is scoped to adding comprehensive mock-based handler tests. The description focuses on this single concern: adding tests for handler functions in Code quality
This is test code, so code quality standards still apply but may be slightly different. Looking at the diff, the test code appears to be well-structured with the shared infrastructure. I don't see obvious code quality issues in the visible portions. Bugs & correctness
Again, this is test code, but we want the tests to be correct. The tests appear to be setting up mock backends and verifying expected behavior. They use helper functions like Now, let me look for any specific issues in the visible parts of the diff:
One thing I notice is that the PR mentions it depends on #229 ( Let me check if there are any obvious issues: In the test files, I see patterns like: mb := &mock.MockBackend{
IsConnectedFunc: func() bool { return true },
ListAgentEditorModelsFunc: func() ([]*agenteditor.Model, error) { return []*agenteditor.Model{m1}, nil },
}This looks correct - they're setting up the mock backend to return specific test data. The helper functions like The PR says: "Introduce shared test infrastructure ( I don't see Let me check if there are any potential issues:
However, looking more closely, the PR body says: "All tests use stdlib only (no testify dependency)" and "Tests bypass But the diff shows changes to:
The changes to non-test files are:
The changes to mdl/ast/ast_agenteditor.go and internal/marketplace/types.go seem questionable as part of a "test-only files" PR. They might be accidental inclusions or might be necessary for some reason. Let me think: why would ast_agenteditor.go need to change for test purposes? The AST definitions shouldn't need to change just to add tests for the executor, unless the tests are exposing a need for refactoring the AST. Actually, looking at the changes in ast_agenteditor.go, they're just changing the spacing of field declarations:
This is purely a formatting change with no functional impact. It's possible this was done by an auto-formatter or as part of cleaning up the file, but it's not related to adding tests. Similarly, the change in internal/marketplace/types.go is just adjusting comment spacing. These changes are harmless but might be considered noise in a PR that's supposed to be about adding tests. However, the changes to the mock backend are necessary to support the new tests for agent editor functionality. Now, let me check if there are any issues with the test approach: The PR says tests "bypass Looking at one of the test files, e.g., cmd_agenteditor_mock_test.go: func TestShowAgentEditorModels_Mock(t *testing.T) {
mod := mkModule("M")
m1 := &agenteditor.Model{
BaseElement: model.BaseElement{ID: nextID("aem")},
ContainerID: mod.ID,
Name: "GPT4",
Provider: "MxCloudGenAI",
DisplayName: "GPT-4 Turbo",
Key: &agenteditor.ConstantRef{QualifiedName: "M.APIKey"},
}
h := mkHierarchy(mod)
withContainer(h, m1.ID, mod.ID)
mb := &mock.MockBackend{
IsConnectedFunc: func() bool { return true },
ListAgentEditorModelsFunc: func() ([]*agenteditor.Model, error) { return []*agenteditor.Model{m1}, nil },
}
ctx, buf := newMockCtx(t, withBackend(mb), withHierarchy(h))
assertNoError(t, showAgentEditorModels(ctx, ""))
out := buf.String()
assertContainsStr(t, out, "Qualified Name")
assertContainsStr(t, out, "Module")
assertContainsStr(t, out, "Provider")
assertContainsStr(t, out, "Key Constant")
assertContainsStr(t, out, "Display Name")
assertContainsStr(t, out, "M.GPT4")
assertContainsStr(t, out, "MxCloudGenAI")
assertContainsStr(t, out, "M.APIKey")
assertContainsStr(t, out, "GPT-4 Turbo")
}This looks like a good unit test:
The use of helper functions like mkModule, nextID, mkHierarchy, withContainer, newMockCtx, withBackend, withHierarchy, assertNoError, assertContainsStr suggests a well-designed test infrastructure. One potential concern: the PR mentions "thread-safe ID generation via In the test files, I see Let me see if I can spot the ID generation in the visible diff... I don't see the implementation of nextID in the visible portions, but the PR says it's in the shared test infrastructure. Overall, this PR appears to be adding valuable test coverage for the executor handler functions. The approach of testing the handlers directly with mocked dependencies is sound. The minor issues I noticed:
These are harmless but might be considered noise in a PR that's supposed to be about adding tests-only files. However, it's possible these changes were made as part of preparing the codebase for better testability (e.g., if the AST changes were needed to make the agent editor structures more testable, though I don't see how spacing changes would help with that). Given that the PR says "No production code changes — test-only files", but we see changes to non-test files, this is a bit inconsistent. However, the changes to non-test files are:
So the only non-trivial production code changes are to the mock backend, which is test-related infrastructure. The changes to the AST file and marketplace types file are truly trivial and likely accidental or done by an auto-formatter. Let me check if there are any other issues: In the test files, I notice some tests have descriptions like "not yet implemented": func TestShowBusinessEventClients_Mock(t *testing.T) {
ctx, buf := newMockCtx(t)
assertNoError(t, showBusinessEventClients(ctx, ""))
assertContainsStr(t, buf.String(), "not yet implemented")
}This is testing that a not-yet-implemented feature returns an appropriate message, which is reasonable. Also, I see tests for error cases: func TestDescribeConstant_Mock_NotFound(t *testing.T) {
// ... setup ...
err := describeConstant(ctx, ast.QualifiedName{Module: "MyModule", Name: "Missing"})
assertError(t, err)
}This is good - testing error paths. Now, let me think about the checklist items again: Overlap & duplication
MDL syntax design
Full-stack consistency
Test coverage
Security & robustness
Scope & atomicity
Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 36 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
retran
force-pushed
the
feature/mock-handler-tests
branch
from
16956b5 to
3866c81
Compare
AI Code ReviewCritical Issues
Moderate Issues
Minor Issues
What Looks Good
RecommendationApprove - The PR significantly improves test coverage for the executor layer with well-designed tests and minimal, safe production changes solely to enable testability. All checklist items are satisfied for this type of change (test addition). No syntax changes were introduced, so MDL syntax and full-stack consistency checks are not applicable. The test infrastructure is robust and follows best practices. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 36 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Phase 1-8: 85 read/write handler tests with mock backend infrastructure Phase 9: 104 additional tests covering error paths, not-connected guards, and JSON format output Test categories: - Read-path handlers (SHOW/DESCRIBE) across 20+ domains - Write-path handlers (CREATE/DROP) for 10 operations - Backend error propagation for all handler groups - Not-connected guard verification for 29 guarded handlers - JSON format output validation for 26 show/list handlers
retran
force-pushed
the
feature/mock-handler-tests
branch
from
3866c81 to
fc7adcd
Compare
AI Code ReviewWhat Looks Good
Minor Issues
RecommendationApprove. The PR adds valuable test coverage for executor handlers with well-designed test infrastructure. The minor whitespace issues are negligible and don't warrant blocking the merge. The changes to make MockBackend more testable are justified and follow standard patterns. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
Summary
Add comprehensive mock-based handler tests for the MDL executor, covering 189 test cases across 33 test files.
Depends on: #229 (merged)
What changed
Test infrastructure (
mock_test_helpers_test.go)newMockCtxfactory with functional options (withBackend,withFormat,withHierarchy, etc.)mkModule,mkEntity,mkMicroflow,mkPage, etc.)assertNoError,assertError,assertContainsStr,assertValidJSON)disconnectedBackend()helper for not-connected guard testsPhase 1-8: Handler tests (85 tests)
Phase 9: Error paths, not-connected guards, JSON format (104 tests)
Connected()/ConnectedForWrite()guards verifiedshow/listhandlers that usewriteResultverified withFormatJSONProduction code changes
Funcfields toMockBackendfor 8 agent-editor write methods (was hardcodedreturn nil)ContainerIDparameter semantics inwithContainerhelper and all call sitesast_agenteditor.goTest count