fix: stabilize MDL roundtrip and integration test harness#267
fix: stabilize MDL roundtrip and integration test harness#267ako merged 5 commits intomendixlabs:mainfrom
Conversation
AI Code ReviewReview SummaryThis PR contains stabilization fixes for the MDL integration test harness and roundtrip functionality. It addresses four main areas: preferring locally installed mx, stabilizing the MDL test runner/docker flow, stabilizing MDL roundtrip/integration flows, and aligning role casing for integration tests. Overlap & Duplication
MDL Syntax Design
Full-Stack Consistency
Test Coverage
Security & Robustness
Scope & Atomicity
Code Quality
Bugs & Correctness
RecommendationAPPROVE - The PR successfully stabilizes the integration test harness and fixes critical roundtrip issues. The changes are well-tested, focused, and improve reliability without introducing regressions. Minor issues noted do not block merging. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
# Conflicts: # mdl/executor/cmd_microflows_create.go # mdl/executor/executor.go
|
Re: the AI reviewer comment about closing the mpr reader on all error paths in if reader, err := mpr.Open(opts.ProjectPath); err == nil {
projectVersion = reader.ProjectVersion().ProductVersion
reader.Close()
}The reader is only materialized inside the |
hjotha
force-pushed
the
submit/integration-test-stabilization
branch
from
912e0f9 to
48941bb
Compare
PR #267 Review — fix: stabilize MDL roundtrip and integration test harnessOverviewFour independent stabilization areas: version-aware mx/mxbuild resolution, test harness cleanup, three roundtrip fixes (FindOrCreate, default document roles, cross-module move), and case-insensitive role matching. Solid individual fixes, but the bundling is a concern. Blockers / Notable IssuesPR scope (checklist item: "Each PR is scoped to a single feature or concern")
import "github.com/mendixlabs/mxcli/sdk/mpr"
...
if mf, err := mpr.ParseMicroflowBSON(rawUnit.Contents, ...); err == nil {
return mf.ReturnType
}The executor must go through
Minor Issues
if latest, err := ctx.Backend.GetProjectSecurity(); err == nil {
ps = latest
} else if ps == nil {
return err
}If
What's Good
VerdictThe individual fixes are sound and well-tested. The two structural issues — backend abstraction violation in |
The roundtrip test helpers previously preferred a repo-local
`reference/mxbuild/modeler/mx` (a developer convenience that was never
checked into CI images), then fell back to cached downloads, then to
`PATH`. On machines where CI installs `mx` via the system package
(current setup) the harness still walked the fallback chain and either
ran against a stale cached binary or a missing repo-local path, which
silently changed the Mendix version exercised by the tests.
Reorder `findMxBinary`:
MX_BINARY env var -> PATH -> repo-local reference -> cached downloads
`PATH` now wins over repo-local so whatever version CI installs is what
the tests use. The cached-download fallback also stops returning the
lexicographically last match — `9.24.40.80973` sorted after `11.9.0`
and we ran integration tests on Mendix 9 by accident. Add
`newestVersionedPath` which parses each path's version segment into
`[major, minor, patch, build]` ints and picks the highest tuple.
Two `//go:build integration` tests cover the new helper:
- `TestNewestVersionedPath_PicksNewestNumericVersion` — version ordering
- `TestFindMxBinary_PrefersPathOverCachedDownloads` — PATH wins when
both PATH and cache have `mx` present
Several unrelated papercuts made the integration/test-runner flow unreliable in CI. This folds the fixes into one scope (MDL test runner + docker pathing) since they were discovered together while chasing the same green build. Test-runner and generator: - `varPattern` / `assignPattern` now match on any line in the test body (added `(?m)` flag and tightened anchoring). Without this, multi-line test cases saw `$var = CREATE ...` silently left un-rewritten, which then failed the compiled microflow with "variable is not declared". - Drop the `isSideEffectStatement` / auto-`ON ERROR CONTINUE` wrapping for DELETE/COMMIT/CHANGE. The blanket wrap hid actual test failures behind silent passes; if a test wants tolerant error handling it can write it explicitly. - `runner.Run` now ensures the `.docker/` stack exists before building or restarting (new `ensureDockerStack` helper that calls `docker.Init` on demand). Previously a fresh checkout with no `.docker/docker-compose.yml` failed with a raw `stat` error before any test ran. Docker layer: - `resolveMxBuild` / `ResolveMxForVersion` now accept a preferred project version and prefer exact-match cached binaries over the lexicographically-last glob entry (which sorted Mendix 9 after 11). `AnyCachedMxBuildPath` and the MxBuild downloader share the new `newestVersionedPath` helper that parses `[major,minor,patch,build]` numerically. - `findPADDir` / `isPADDir` recognise the already-extracted layout (`app/`, `bin/`, `etc/`, `lib/`) produced by newer PAD outputs where the Dockerfile is not emitted. - `Check` resolves the project's version from the MPR reader and passes it through, so check-on-build uses the same `mx` binary the project was last built against. Microflow builder type inference: - `flowBuilder.lookupMicroflowReturnType` walks the raw microflow unit to fetch the return type of a called microflow. When the builder knows the return type, `registerResultVariableType` records it in `varTypes` with qualified entity name, so subsequent CHANGE / attribute-access activities on the returned variable resolve correctly. This is what lets tests like `$Product = CALL MfTest.M012_CreateEntity(); CHANGE $Product SET Price = 10.0;` validate without forcing the test author to re-declare the variable type. Makefile / scripts: - `make test-mdl` now goes through `scripts/run-mdl-tests.sh`, which copies the project into a temp directory before running the bootstrap MDL + test spec. The old direct invocation mutated the repo's seed `app.mpr`, so re-running the target picked up stale state. Regression tests: - `TestFindPADDir_ExtractedLayoutInRoot` for the new PAD layout - generator tests cover the multiline assign pattern - builder-type test uses `MockBackend.ListMicroflowsFunc` to assert that a subsequent CHANGE resolves against the returned entity
Four independent stability fixes surfaced together while chasing a green
roundtrip suite. They touch different layers but are bundled here because
they all gate on the same test run — splitting would require duplicated
test fixtures across PRs.
sdk/mpr — FindOrCreate mapping roundtrip:
Import/export mappings store "FindOrCreate" in BSON as a *pair* of
fields: `ObjectHandling=Find` plus `ObjectHandlingBackup=Create`. The
parser previously copied `ObjectHandling` verbatim (yielding just
"Find"), and the export writer hardcoded `ObjectHandlingBackup=Create`
regardless of the primary mode.
- `parser_import_mapping.go` now recognises the `Find + Create` pair
and promotes it to the synthetic `FindOrCreate` value the domain
model uses elsewhere.
- `writer_export_mapping.go` echoes the actual `ObjectHandling`
instead of forcing "Create"; misuse produced CE6702 on describe +
re-execute cycles.
- `writer_import_mapping.go` inverts the split symmetrically: if the
model says `FindOrCreate`, serialise as `Find` + `Create` so Studio
Pro can load the mapping.
Added parser/writer tests cover both halves and a matching fixture
(`RestTest.PetRecord`) was added to `06-rest-client-examples.mdl`
so the mapping has a real entity to bind to; this lets
`roundtrip_doctype_test.go` drop CE6702 from the known-errors list.
mdl/executor — DROP + CREATE OR REPLACE preserves UnitID:
When a script ran `DROP MICROFLOW X; CREATE OR MODIFY MICROFLOW X ...`
in the same session, the executor deleted the Unit row and inserted
a new one with a fresh UUID. Studio Pro treats the new UnitID as an
unrelated document and rejects the file with ".mpr does not look like
a Mendix Studio Pro project".
- `executor.go` adds a `droppedMicroflows` cache on `executorCache`
keyed by qualified name, plus `rememberDroppedMicroflow` /
`consumeDroppedMicroflow` helpers.
- `cmd_microflows_drop.go` records the UnitID, ContainerID, and
AllowedModuleRoles before calling `DeleteMicroflow`.
- `cmd_microflows_create.go` consumes the remembered entry when the
qualified name matches, reusing the original UnitID (and
ContainerID when no new folder is given) so the rewrite looks like
an in-place update from the file's perspective.
- Mock regression test
`TestDropThenCreatePreservesMicroflowUnitID` asserts the reused
UnitID and preserved access roles.
mdl/executor — default document access roles:
Fresh `CREATE MICROFLOW` / `CREATE PAGE` on a module with no module
roles tripped CE0148 ("document has no access for any role"). Rather
than leaving every script author to define roles by hand, auto-
provision a minimal `User` role on modules that currently have none,
and stamp it onto new pages/microflows.
- `cmd_security_defaults.go` (new) adds `defaultDocumentAccessRoles`,
`remapDocumentAccessRoles` (cross-module MOVE), `documentRoleStrings`,
`cloneRoleIDs`, and `pruneInvalidUserRoles`. The auto-created role
carries a fixed description (`autoDocumentRoleDescription`) so
`CREATE MODULE ROLE User` on the same module is treated as a no-op
rather than CE0112 "already exists".
- `cmd_microflows_create.go` and `cmd_pages_create_v3.go` stamp the
default on new units and preserve existing roles across
`OR REPLACE` / `OR MODIFY`.
- `cmd_move.go` remaps access roles across cross-module moves using
the target module's role names (falling back to the default role
when the target has none).
- `cmd_modules.go` and `cmd_security_write.go` call
`pruneInvalidUserRoles` after DROP MODULE / DROP MODULE ROLE so
orphaned user roles don't trip CE0157.
- `cmd_microflows_builder.go` — `registerResultVariableType(nil)`
now clears stale entity typing and marks the variable as declared
with type `Unknown`, instead of leaving the variable un-tracked,
so subsequent CHANGE/attribute access still resolves. Covered by
`TestCallMicroflowUnknownResultTypeStillDeclaresVariable`.
Test and script hardening:
- `docker/check_test.go` and `docker/detect_test.go` set
`PATH=t.TempDir()` instead of `PATH=""` for the cache-preference
tests. Empty `PATH` confuses `exec.LookPath` on Linux (it falls
back to the current directory) and made the test flaky depending
on `os.Getwd()`. An empty temp directory is a clean "mx isn't on
PATH" signal.
- `scripts/run-mdl-tests.sh` now validates `$PROJECT_MPR`,
`$MXCLI_BIN`, `$TEST_SPEC`, `$BOOTSTRAP_MDL` with `[[ -f ... ]]`
guards. `${VAR:?...}` only fires on *unset* vars, so typo'd paths
previously produced an empty sandbox and confusing downstream
errors.
Addresses the integration-test failures that the fork-sync branch exposed: - sdk/mpr/writer_security.go: AddModuleRole now overwrites the existing role's Name/Description when a case-insensitive duplicate is detected. Mendix rejects case-insensitive duplicate role names (CE0123), so adopting the caller's casing matches runtime semantics and keeps subsequent GRANT ACCESS lookups resolvable. - mdl/executor/cmd_security_write.go: execCreateModuleRole matches the existing role case-insensitively and propagates a casing change to all units via UpdateQualifiedNameInAllUnits. Without this, AllowedModuleRoles references on microflows/pages/published REST services that were created before the user-declared role ran would be stale (CE1613). - mdl-examples/doctype-tests/06-rest-client-examples.mdl: capitalize Status = status in the PetRecord find-or-create block; attribute references are case-sensitive. - mdl-examples/doctype-tests/workflow-user-targeting.mdl: replace the obsolete System.UserGroup with System.WorkflowGroup to match the Mendix 11.9 workflow metamodel (CE5012). - mdl/executor/roundtrip_microflow_test.go: compare LOG INFO NODE expected output case-insensitively. DESCRIBE now emits lowercase keywords (commit 00b80f3). - mdl/executor/cmd_odata.go: pick up the gofmt-clean tab indentation after make lint-go; purely whitespace. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…n helper dedup) Addresses two structural issues from ako's review: 1. **sdk/mpr import in cmd_microflows_builder.go** — lookupMicroflowReturnType called mpr.ParseMicroflowBSON directly, violating the executor's backend abstraction contract. Added ParseMicroflowBSON(contents, unitID, containerID) to the MicroflowBackend interface, implemented on MprBackend (delegates to mpr.ParseMicroflowBSON) and Mock (Func-field with descriptive 'MockBackend.ParseMicroflowBSON not configured' default). lookupMicroflow ReturnType now goes through fb.backend.ParseMicroflowBSON. sdk/mpr import removed from the builder. 2. **newestVersionedPath duplication** — the four version-comparison helpers (newestVersionedPath, versionFromPath, parseVersionParts, compareVersion Parts) lived identically in cmd/mxcli/docker/download.go and mdl/executor/roundtrip_helpers_test.go. Exported the production helper as docker.NewestVersionedPath (with a full doc comment explaining the ranking); internal docker callers updated; the integration-test harness now imports docker and calls docker.NewestVersionedPath. All four copies removed from the test file.
hjotha
force-pushed
the
submit/integration-test-stabilization
branch
from
48941bb to
4a1ce9a
Compare
|
Thanks @ako — addressed the two structural items in 4a1ce9a:
On the scope concern — fair call. I'd prefer not to split this one retroactively since the four areas did surface from the same test run and the roundtrip fixes (FindOrCreate, default document roles, cross-module move) are small enough that reverting them piecewise would mean untangling them from the test harness changes that validate them. Open to doing it if you consider it a blocker, though — in that case I'd cut three separate PRs (version resolver + harness; roundtrip fixes; role casing) from this branch. The two minor issues (
|
AI Code ReviewWhat Looks Good
RecommendationApprove. The PR addresses real pain points in the test infrastructure with well-targeted fixes that improve reliability and performance. The changes are properly tested and maintain backward compatibility while resolving flaky integration tests. No MDL syntax or feature changes are present, so the full-stack consistency checklist doesn't apply. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
…n helper dedup) Addresses two structural issues from ako's review: 1. **sdk/mpr import in cmd_microflows_builder.go** — lookupMicroflowReturnType called mpr.ParseMicroflowBSON directly, violating the executor's backend abstraction contract. Added ParseMicroflowBSON(contents, unitID, containerID) to the MicroflowBackend interface, implemented on MprBackend (delegates to mpr.ParseMicroflowBSON) and Mock (Func-field with descriptive 'MockBackend.ParseMicroflowBSON not configured' default). lookupMicroflow ReturnType now goes through fb.backend.ParseMicroflowBSON. sdk/mpr import removed from the builder. 2. **newestVersionedPath duplication** — the four version-comparison helpers (newestVersionedPath, versionFromPath, parseVersionParts, compareVersion Parts) lived identically in cmd/mxcli/docker/download.go and mdl/executor/roundtrip_helpers_test.go. Exported the production helper as docker.NewestVersionedPath (with a full doc comment explaining the ranking); internal docker callers updated; the integration-test harness now imports docker and calls docker.NewestVersionedPath. All four copies removed from the test file.
hjotha
force-pushed
the
submit/integration-test-stabilization
branch
from
4a1ce9a to
ed97d97
Compare
AI Code ReviewLet me review this pull request based on the checklist provided in the CLAUDE.md file. Summary of ChangesThis PR contains several fixes to stabilize the MDL roundtrip and integration test harness:
Review Against ChecklistOverlap & duplication
MDL syntax design
Full-stack consistency (for MDL features)
Test coverage
Security & robustness
Scope & atomicity
Code quality
Bugs & correctness
What Looks Good
RecommendationThis PR appears to be a set of valuable bug fixes and stabilization improvements for the integration test harness and roundtrip functionality. It addresses real issues that were causing test flakiness and slow test runs. The changes are focused, well-tested, and follow the existing code patterns. There are no violations of the review checklist observed. Recommendation: Approve the PR. The PR successfully stabilizes the MDL roundtrip and integration test harness as intended, with appropriate test coverage and no violations of the project's contribution guidelines. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
3 participants
Summary
Four integration-harness and roundtrip stabilization fixes that surfaced from running the full test matrix end-to-end:
test: prefer installed mx in integration harness— the harness was re-downloading mxbuild on every run even when a workingmxwas already installed locally. Check for an installedmxfirst, fall back to download. Dramatically reduces test turnaround.stabilize MDL test runner and docker flow— timeouts, cleanup, and error propagation inscripts/run-mdl-tests.shand the docker check flow. Previously a single failing container could leave the harness stuck or swallow the underlying error.stabilize MDL roundtrip and integration flows— three independent roundtrip fixes bundled because they're all exercised by the same integration test fixtures:FindOrCreateimport-mapping roundtrip (split intoObjectHandling=Find+ObjectHandlingBackup=Create).autoDocumentRoleDescriptionsentinel +cmd_security_defaults.gohelpers).DROP+CREATE OR REPLACEmicroflow UnitID preservation honored acrossmoveas well.align role casing and unstick integration roundtrip tests— case-insensitive role lookup + fixture updates so the roundtrip tests unblock on projects that use mixed-case role names.Part of umbrella #257.
Test plan
bugfix_regression_test.go,roundtrip_doctype_test.go,parser_import_mapping_test.go,writer_export_mapping_test.go,writer_import_mapping_test.go.go build ./... && go test ./...pass on top of currentmain.