test: add visitor tests for 20 untested source files by retran · Pull Request #245 · mendixlabs/mxcli

retran · 2026-04-21T11:25:14Z

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.

When setting PAGE on a workflow activity where the TaskPage BSON key is absent (not just nil), dSet silently failed because it only updates existing keys. Added replaceActivity helper that appends the key to the activity document and replaces it in the BSON tree by match. Three cases now handled correctly: - TaskPage exists with value: update Page field in place - TaskPage exists with nil value: replace via dSet - TaskPage absent: append key, replace activity in tree

… duplication

…ss test Add mock implementations of PageMutator (16 methods) and WorkflowMutator (15 methods) using the same Func-field delegation pattern as MockBackend. Both include compile-time interface checks. Fix allKnownStatements() to include 8 missing agent editor statement types (CreateAgent, DropAgent, CreateModel, DropModel, CreateConsumedMCPService, DropConsumedMCPService, CreateKnowledgeBase, DropKnowledgeBase). Add handler count snapshot test.

Cover security (24 tests), odata (4), agent editor (4), REST (2), settings (4), module (1), association (3), connection (2), business events (1), enumeration (5), JSON structure (1), data transformer (1), widgets (2), image collection (1), navigation (1), lint (4), import/export mapping (2), move (2), misc queries (4), and database connection (1). Visitor test function count: 128 -> 197 (+69).

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.

…comment - visitor_lint_test.go: assert Target.Name and Target.Module separately to prevent false positives - visitor_widgets_test.go: use guarded type assertion with len check instead of direct assertion that would panic - mock_page_mutator.go: clarify doc comment that ContainerType defaults to ContainerPage, not zero value

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 · 2026-04-21T12:02:16Z

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.

Replace 13 unguarded prog.Statements[0].(*ast.XxxStmt) direct assertions with guarded two-value form + t.Fatalf across visitor_settings_test.go, visitor_security_test.go, and visitor_lint_test.go to prevent panics on unexpected types.

github-actions · 2026-04-21T13:07:48Z

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.

retran added 8 commits April 21, 2026 11:53

test: add nested sub-flow test for replaceActivity recursive path

7047e58

test: use distinct IDs in nested sub-flow test to ensure recursive path

767b9f8

refactor: use dGetArrayElements in replaceActivityRecursive to reduce…

cddd422

… duplication

style: invert dSet condition to eliminate empty branch

34d7a58

Clarify handler count snapshot test comment per review

4d970a6

Copilot AI review requested due to automatic review settings April 21, 2026 11:25

Copilot started reviewing on behalf of retran April 21, 2026 11:25 View session

Copilot AI reviewed Apr 21, 2026

View reviewed changes

Comment thread mdl/visitor/visitor_lint_test.go Outdated

Comment thread mdl/visitor/visitor_widgets_test.go Outdated

Comment thread mdl/backend/mock/mock_page_mutator.go

retran marked this pull request as draft April 21, 2026 11:33

retran marked this pull request as ready for review April 21, 2026 11:49

retran requested a review from Copilot April 21, 2026 11:53

retran marked this pull request as draft April 21, 2026 11:53

Copilot started reviewing on behalf of retran April 21, 2026 11:53 View session

Copilot AI reviewed Apr 21, 2026

View reviewed changes

Comment thread mdl/visitor/visitor_security_test.go

Comment thread mdl/backend/mpr/workflow_mutator.go

Remove unused grantRevokeTest struct per review

94891e8

retran requested a review from Copilot April 21, 2026 12:40

Copilot started reviewing on behalf of retran April 21, 2026 12:41 View session

Copilot AI reviewed Apr 21, 2026

View reviewed changes

Comment thread mdl/visitor/visitor_settings_test.go Outdated

Comment thread mdl/visitor/visitor_security_test.go Outdated

Comment thread mdl/visitor/visitor_lint_test.go Outdated

retran requested a review from Copilot April 21, 2026 13:16

Copilot started reviewing on behalf of retran April 21, 2026 13:16 View session

Copilot AI reviewed Apr 21, 2026

View reviewed changes

retran marked this pull request as ready for review April 21, 2026 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

test: add visitor tests for 20 untested source files#245
retran wants to merge 11 commits intomendixlabs:mainfrom
retran:test/visitor-coverage

retran commented Apr 21, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

retran commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Additional changes

Motivation

Test approach

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 21, 2026

AI Code Review

Critical Issues

Moderate Issues

Minor Issues

What Looks Good

Recommendation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Apr 21, 2026

AI Code Review

What Looks Good

Minor Issues

Recommendation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

retran commented Apr 21, 2026 •

edited

Loading