test: add visitor tests for 20 untested source files

[retran]

Summary

• Add 21 new visitor test files covering all previously untested visitor source files
• 69 new test functions bring visitor test count from 128 to 197
• Covers security (24 tests including grant/revoke for all access types), OData, agent editor, REST, settings, module, association, connection, business events, enumeration, JSON structure, data transformer, widgets, image collection, navigation, lint, import/export mapping, move, misc queries, and database connection visitors

Additional changes

This PR also includes changes from earlier PRs in the stack that this branch builds on:

• Workflow mutator fix (from PR fix: handle missing TaskPage key in workflow activity mutation #243): Fix SetActivityProperty(..., "PAGE", ...) to handle missing TaskPage BSON key by appending the field and replacing the activity in the workflow tree. Includes replaceActivityRecursive helper for nested sub-flow traversal.
• Test enablers (from PR Add test enablers: MockPageMutator, MockWorkflowMutator, registry completeness #244): Add MockPageMutator and MockWorkflowMutator configurable mock implementations. Expand executor registry completeness test with handler count snapshot.

These are included because this is a stacked PR branch (PR 0 → A → B). Once the earlier PRs merge and this branch is rebased, only the visitor test changes will remain in the diff.

Motivation

Part of the MDL subsystem test coverage initiative. The visitor package had 22.8% coverage (18/79 exported functions tested). This PR covers the remaining untested visitor functions to provide a safety net for future refactoring.

Test approach

Follows existing visitor test pattern: inline MDL strings → Build(input) → type-assert AST nodes → assert key fields. Flat imperative style matching visitor_workflow_test.go conventions.

[Copilot]

Pull request overview

Adds broad test coverage for previously untested MDL visitor statements and tightens a workflow mutator edge case around setting a user task page when the BSON key is missing.

Changes:

• Add many new mdl/visitor/*_test.go files that parse MDL snippets via Build(...) and assert resulting AST node fields.
• Fix mprWorkflowMutator.SetActivityProperty(..., "PAGE", ...) to handle missing TaskPage keys by appending the field and replacing the activity in the workflow BSON tree.
• Expand executor registry completeness coverage (new statement types + handler count snapshot) and apply minor formatting/cleanup in existing tests and backend code.

Reviewed changes

Copilot reviewed 28 out of 31 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
mdl/visitor/visitor_widgets_test.go	New visitor tests for UPDATE WIDGETS statements.
mdl/visitor/visitor_settings_test.go	New visitor tests for settings/configuration statements.
mdl/visitor/visitor_security_test.go	New visitor tests for roles, grants/revokes, and project security statements.
mdl/visitor/visitor_rest_test.go	New visitor tests for REST client and published REST service statements.
mdl/visitor/visitor_odata_test.go	New visitor tests for OData client/service and external entity statements.
mdl/visitor/visitor_navigation_test.go	New visitor tests for navigation/profile/menu parsing.
mdl/visitor/visitor_move_test.go	New visitor tests for MOVE statements.
mdl/visitor/visitor_module_test.go	New visitor tests for CREATE MODULE.
mdl/visitor/visitor_misc_test.go	New visitor tests for misc statements (SEARCH/EXECUTE/help/UPDATE).
mdl/visitor/visitor_lint_test.go	New visitor tests for LINT and SHOW LINT RULES parsing.
mdl/visitor/visitor_jsonstructure_test.go	New visitor tests for CREATE JSON STRUCTURE.
mdl/visitor/visitor_import_mapping_test.go	New visitor tests for CREATE IMPORT MAPPING.
mdl/visitor/visitor_imagecollection_test.go	New visitor tests for CREATE IMAGE COLLECTION.
mdl/visitor/visitor_export_mapping_test.go	New visitor tests for CREATE EXPORT MAPPING.
mdl/visitor/visitor_enumeration_test.go	New visitor tests for enumeration ops and CREATE CONSTANT variants.
mdl/visitor/visitor_dbconnection_test.go	New visitor tests for CREATE DATABASE CONNECTION.
mdl/visitor/visitor_datatransformer_test.go	New visitor tests for CREATE DATA TRANSFORMER.
mdl/visitor/visitor_connection_test.go	New visitor tests for CONNECT/DISCONNECT.
mdl/visitor/visitor_businessevents_test.go	New visitor tests for business event service parsing.
mdl/visitor/visitor_association_test.go	New visitor tests for association create/alter parsing.
mdl/visitor/visitor_agenteditor_test.go	New visitor tests for agent editor-related statements (model/MCP/KB/agent).
mdl/types/id_test.go	Minor formatting alignment in ID validation tests.
mdl/types/edmx_test.go	Minor formatting alignment in EDMX tests.
mdl/executor/widget_registry.go	Minor formatting alignment in struct fields.
mdl/executor/registry_test.go	Add new known statement types + handler count snapshot test.
mdl/backend/mpr/workflow_mutator_test.go	Update/extend tests to cover PAGE mutation when TaskPage key is missing (incl. nested sub-flow).
mdl/backend/mpr/workflow_mutator.go	Fix PAGE mutation to handle missing TaskPage by appending + replacing activity in BSON tree.
mdl/backend/mpr/convert_roundtrip_test.go	Remove trailing whitespace line.
mdl/backend/mpr/backend.go	Formatting-only changes for method layout.
mdl/backend/mock/mock_workflow_mutator.go	Add a configurable mock implementation of backend.WorkflowMutator.
mdl/backend/mock/mock_page_mutator.go	Add a configurable mock implementation of backend.PageMutator.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 28 out of 31 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• Formatting changes in backend.go: The changes to mdl/backend/mpr/backend.go (adding braces for single-line functions) are unnecessary since the project uses go fmt. These should be removed to avoid noisy diffs, though they are harmless.
• Stacked PR scope: While understandable given the PR stack, the inclusion of workflow mutator fixes and test enablers dilutes the primary focus on visitor tests. These should ideally be in separate PRs, but the author's explanation about rebasing mitigates this concern.

What Looks Good

• Visitor tests: Excellent addition of 21 test files covering previously untested visitor functions (69 new tests). Follows established patterns: inline MDL strings → Build() → type assertions → field validation. Matches visitor_workflow_test.go conventions.
• Workflow mutator fix: Correctly handles missing TaskPage BSON key by appending and replacing activity in tree. Includes comprehensive tests for flat and nested scenarios. The replaceActivityRecursive helper is well-implemented.
• Test enablers: MockPageMutator and MockWorkflowMutator are clean, configurable mocks that follow existing patterns. The registry test addition (HandlerCountSnapshot) is a smart safeguard for future completeness.
• Test quality: All new tests are flat, imperative, and avoid time.sleep. Security-focused tests (grant/revoke) are thorough and table-driven where appropriate.
• No MDL syntax changes: Since no new MDL features are added, full-stack consistency checks are not applicable—appropriate for this test-focused PR.

Recommendation

Approve. The PR significantly improves test coverage for the visitor package (from 22.8% to ~49% based on context) with high-quality tests. The minor formatting issues are trivial and the stacked PR context is justified. Once rebased, only the valuable visitor test changes will remain. Prerequisite changes (workflow fix, test enablers) are correct and ready for merging.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[Copilot]

Pull request overview

Copilot reviewed 28 out of 31 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

AI Code Review

What Looks Good

• The PR adds comprehensive test coverage for 21 previously untested visitor source files, increasing test count from 128 to 197
• Tests follow the established pattern: inline MDL strings → Build(input) → type-assert AST nodes → assert key fields
• Covers important areas including security (grant/revoke for all access types), OData, agent editor, REST, settings, and more
• Includes valuable fixes from earlier PRs in the stack:
  • Workflow mutator fix for handling missing TaskPage BSON key with proper recursive replacement
  • Test enablers (MockPageMutator, MockWorkflowMutator) and executor registry completeness test
• All test files use flat imperative style matching existing conventions
• No evidence of duplicating existing implementations or re-documenting shipped features

Minor Issues

• In visitor_datatransformer_test.go: The test uses SNIPPET '{"id": 1, "name": "test"}' which is correct MDL syntax, but the backticks in the expected value comment might be confusing (though technically accurate)
• In mock implementations: Some methods return nil values when function fields are unset (e.g., WidgetScope() returns nil). While acceptable for mocks that should be configured in tests, this could cause nil pointer dereferences if misused in tests
• Comment in workflow mutator test: "TaskPage key present with nil value — should be replaced with a new PageReference" is slightly misleading since the test sets Value: nil and expects replacement, but the behavior is correct

Recommendation

Approve. The PR substantially improves test coverage for the visitor package, fixes a real BSON handling issue, and adds useful testing utilities. The minor issues noted are acceptable in test code and don't affect correctness. The changes are well-scoped and follow project conventions.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[Copilot]

Pull request overview

Copilot reviewed 28 out of 31 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.

[ako]

Review: PR #245 — test: add visitor tests for 20 untested source files

Verdict: Approve with notes — excellent coverage investment. 69 new tests across 21 files is a meaningful step up from 22.8% visitor coverage. A few specific items worth addressing.

---

Overview

Pure test coverage PR. All new production changes (workflow mutator fix, mock mutators, registry completeness) are carry-overs from #243 and #244 in the stack — not re-reviewed here. The 69 new test functions follow the established visitor test pattern: inline MDL → Build(input) → type-assert → assert key fields.

---

Specific Issues

1. ToolChoice: auto (lowercase) in agent test

visitor_agenteditor_test.go line 779:

input := `... ToolChoice: auto, ...`
...
if stmt.ToolChoice != "auto" {

The BSON schema (per the proposal, and confirmed by Tina's corrections in PR #214) stores "Auto" (capitalized), matching GenAICommons.ENUM_ToolChoice which is distinct from the lowercase runtime enum. If the visitor stores the raw token "auto", the executor must capitalize it when writing BSON — or the visitor should normalize it. As-written, the test documents whatever the visitor currently does, but if the visitor passes "auto" through unchanged, Studio Pro will reject it. Worth confirming the visitor normalizes auto → "Auto" and updating the test to use the correct PascalCase form.

2. TestCreateConsumedMCPService missing Endpoint/AuthenticationMicroflow

The test only covers ProtocolVersion, Version, ConnectionTimeoutSeconds, and Documentation. PR #214 (merged today) confirmed that endpoint and authenticationMicroflow are first-class fields in the BSON schema, not runtime-only. If the visitor already parses them, they should be tested here; if not, this is a gap to track.

3. Constants test placed in enumeration file

visitor_enumeration_test.go contains TestCreateConstant and TestCreateConstant_ExposedToClient. These belong in visitor_constant_test.go (or similar) — constants aren't enumerations. Low-priority but it'll cause confusion when someone searches for constant tests.

---

Test Quality

• Pattern is consistent with visitor_workflow_test.go — flat imperative, no test helpers needed at this scale. ✓
• Security tests (24 functions) cover grant/revoke for all access types: entity, microflow, page, workflow, OData, published REST. Comprehensive. ✓
• Each test file maps 1:1 to a visitor source file, which makes coverage gaps easy to spot in future. ✓
• Tests parse valid inputs only — no invalid/error-path coverage, which is consistent with the existing suite and acceptable for a coverage initiative PR.

---

Minor

• TestConnect_Local and TestDisconnect in visitor_connection_test.go are trivially thin (no field assertions on DisconnectStmt) but there's nothing meaningful to assert there — fine.
• TestUpdate in visitor_misc_test.go — worth confirming this covers UPDATE WIDGETS or whatever the UpdateStmt maps to; the name is ambiguous.