From eff13c30ef8c4c79d4856cc7c11cacb2ed08ca7c Mon Sep 17 00:00:00 2001 From: Ako Date: Tue, 27 Oct 2026 15:54:11 +0000 Subject: [PATCH 1/2] fix: mxcli test stops at a parse error when a call has its own on error clause (ako/mxcli#1448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A call in a test body that already had an error-handling clause (`on error rollback`, `on error continue`, a custom handler) got the runner's `ON ERROR BEGIN … END ERROR` appended as a second one. The generated MDL did not parse ("mismatched input 'ON' expecting ';'", plus a misleading hint about `Error` being a keyword) and the run stopped at injection, before the app started. Upstream: mendixlabs/mxcli#1550. Cause: attachOnError (endpoint runner) and its two copies in the legacy after-startup runner wrapped every line startsErrorRaisingCall matched, unconditionally. Their line-based join also broke on a custom handler (the handler body's first `;` ended the "statement") and on a trailing comment. Fix: - One attachOnError for both runners, with and without @throws. It parses the body inside the generator's own wrapper and language version (visitor.FlowStatements, new) and reads each call's own onErrorClause from the tree — not text, so a clause on a later line counts and `on error` in a string, a comment or a nested handler does not. The set of wrapped statements is now a set of grammar rule names, pinned to the grammar. - A call with `on error continue` or a custom handler keeps it verbatim and gets no second handler. `on error rollback` is Mendix's default spelled out (the same ErrorHandlingType as no clause), so it is replaced by the runner's handler, which also rolls back: same model, same verdict. - Keeping custom handlers opened a false green that only the runtime showed: a handler that ends normally rejoins the flow past every statement reading the failed call's output — the @expect checks — and the test returned PASS with nothing evaluated. Each assertion now counts itself in both branches of its decision, and a guard after the last fails a short count ("not every assertion was evaluated"). Both runners. - Legacy log parser: a test with a RUN line and no verdict read as PASS (StatusPass is the zero value). A re-raising handler ends the legacy runner exactly there; it is now an ERROR that says so. Verification (Mendix 11.13.0, --local --db-type hsqldb, the issue's setup): the baseline binary stops with the reported parse error; the fixed one runs mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl: plain/rollback throwing FAIL "exception during execution: boom: a"; continue FAIL on its assertion; custom-handler-ends-normally FAIL "not every assertion was evaluated" (PASS before the guard); re-raise ERROR "boom: a"; @throws plain/rollback PASS, continue/custom FAIL "expected an exception but none was thrown", re-raise ERROR; the report's two statements PASS; an assertion on what the handler set PASS. --legacy-runner gives the same verdicts; its re-raise case was PASS and is now ERROR. Revert proofs (stubbing each guard): the always-attach stub reproduces "mismatched input 'ON'" in the unit tests; the rollback, counter and RUN-without-verdict stubs each fail their test. Co-Authored-By: Claude Opus 5.5 --- ...se-error-call-has-own-on-error-clause.json | 1 + .../skills/mendix/test-microflows/SKILL.md | 27 +- cmd/mxcli/testrunner/generator.go | 142 +++------ cmd/mxcli/testrunner/generator_endpoint.go | 160 ++++++++-- cmd/mxcli/testrunner/generator_test.go | 3 +- .../testrunner/own_error_handler_test.go | 290 ++++++++++++++++++ cmd/mxcli/testrunner/results.go | 11 +- docs-site/src/tools/test-annotations.md | 29 ++ ...est-runner-1448-own-error-handler.test.mdl | 166 ++++++++++ .../bug-tests/test-runner-1448-setup.mdl | 26 ++ mdl/visitor/flow_statements.go | 126 ++++++++ mdl/visitor/flow_statements_test.go | 73 +++++ 12 files changed, 928 insertions(+), 126 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-27-mxcli-test-stops-parse-error-call-has-own-on-error-clause.json create mode 100644 cmd/mxcli/testrunner/own_error_handler_test.go create mode 100644 mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl create mode 100644 mdl-examples/bug-tests/test-runner-1448-setup.mdl create mode 100644 mdl/visitor/flow_statements.go create mode 100644 mdl/visitor/flow_statements_test.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-27-mxcli-test-stops-parse-error-call-has-own-on-error-clause.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-27-mxcli-test-stops-parse-error-call-has-own-on-error-clause.json new file mode 100644 index 0000000000..c03158c332 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-27-mxcli-test-stops-parse-error-call-has-own-on-error-clause.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-27", "symptom": "`mxcli test` stops at injection with `Parse error: line 8:75 mismatched input 'ON' expecting ';'` (and a misleading `'Error' is a keyword` hint) when a test body's call has its own `on error rollback` / `on error continue` / custom handler — no test runs", "cause": "`attachOnError` appended the runner's `ON ERROR BEGIN … END ERROR` to every line `startsErrorRaisingCall` matched, unconditionally, so a call with its own clause got a second one. The line-based join also broke on a custom handler (its body's first `;` ended the 'statement') and on a trailing `-- comment`. Both runners had their own copy of the join", "file": "`cmd/mxcli/testrunner/generator_endpoint.go` (`attachOnError`, now shared by both runners), `mdl/visitor/flow_statements.go` (`FlowStatements`), `cmd/mxcli/testrunner/results.go` (`ParseLogResults`)", "insight": "Find statements by PARSING the body (in the generator's own wrapper and language version) and read each call's own onErrorClause from the tree: a regex cannot tell a clause on the next line from `on error` in a string, a comment or a nested handler. Decide per clause from what Mendix stores: `on error rollback` is the default spelled out (same ErrorHandlingType as no clause), so it gets the runner's handler — keeping it made the same exception an ERROR, failed a matching @throws, and aborted the legacy suite; continue/custom are kept verbatim. Keeping a custom handler opened a FALSE GREEN only a runtime run showed: a handler that ends normally rejoins PAST every statement reading the failed call's output (the flow builder's skip-var rule), i.e. past the @expect checks, and the test returned PASS with nothing evaluated. Fixed fail-closed: each assertion counts itself in both branches and a guard after the last fails a short count — an assertion on what the handler set still passes. The legacy log parser had the twin: a RUN line with no verdict read as PASS (StatusPass is the zero value); a re-raising handler did exactly that. Measured verdict table on 11.13.0 in mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl", "refs": ["ako/mxcli#1448", "mendixlabs/mxcli#1550"]} diff --git a/.claude/skills/mendix/test-microflows/SKILL.md b/.claude/skills/mendix/test-microflows/SKILL.md index ff1b9e6e75..e62bb58c8b 100644 --- a/.claude/skills/mendix/test-microflows/SKILL.md +++ b/.claude/skills/mendix/test-microflows/SKILL.md @@ -18,12 +18,12 @@ description: "Write and run MDL-based microflow tests with `mxcli test` — anno | 239-262 | Running Tests | | 263-450 | How It Works | | 451-489 | Check a test file before you run it | -| 490-596 | Writing Good Tests | -| 597-611 | Test File Organization | -| 612-624 | Interpreting Failures | -| 625-646 | CI Integration | -| 647-653 | Related Skills | -| 654-656 | Warm test loop (`mxcli test --local [--watch]`, `--attach`, `run --local --test-endpoint`) | +| 490-611 | Writing Good Tests | +| 612-626 | Test File Organization | +| 627-639 | Interpreting Failures | +| 640-661 | CI Integration | +| 662-668 | Related Skills | +| 669-671 | Warm test loop (`mxcli test --local [--watch]`, `--attach`, `run --local --test-endpoint`) | # Test Microflows Skill @@ -585,6 +585,21 @@ call microflow Sales.ValidateOrder(Total = -1); Anything else after the tag is an error, not a message: `@throws not found` is refused rather than read as an unquoted one. +A call that already has its own `on error` clause keeps it — the runner adds +its handler only to calls without one, so the test runs the statement exactly as +production does: + +- `on error rollback` is Mendix's default spelled out and behaves like no clause: + FAIL `exception during execution: …`, or PASS under a matching `@throws`. +- `on error continue` swallows the error; the assertions decide. `@throws` FAILs + (`expected an exception but none was thrown`). +- A custom handler that ends normally skips every statement reading the failed + call's result — the assertions — so the test FAILs with `not every assertion + was evaluated`. Assert on something the handler sets instead. +- A custom handler that re-raises lets the error leave the test: ERROR with the + message, also under `@throws`. Under Docker / `--legacy-runner` it ends the + whole run. + `@throws` and `@expect` cannot be combined. A `@throws` test compiles to a different shape — the verdict starts as a failure and only the error handler clears it — and the `@expect` checks are not emitted into it at all, so the diff --git a/cmd/mxcli/testrunner/generator.go b/cmd/mxcli/testrunner/generator.go index c28cb42c2e..674b81b3dc 100644 --- a/cmd/mxcli/testrunner/generator.go +++ b/cmd/mxcli/testrunner/generator.go @@ -161,9 +161,17 @@ func writeTestBlock(b *strings.Builder, tc TestCase, index int) { varNames := extractVariableNames(tc.MDL) renamedMDL := renameVariables(tc.MDL, varNames, suffix) + // Every assertion counts itself when evaluated; see assertionsEvaluatedVar. + // On this runner a skipped assertion logs no line at all, so without the + // guard the test would end with a RUN line and no verdict. + evaluated := assertionsEvaluatedVar + suffix + if len(tc.Expects) > 0 { + b.WriteString(fmt.Sprintf(" DECLARE %s Integer = 0;\n", evaluated)) + } + // Rewrite the MDL body: indent it and wrap CALL MICROFLOW with error handling lines := strings.Split(renamedMDL, "\n") - rewritten := rewriteWithErrorHandling(lines, tc.ID) + rewritten := rewriteWithErrorHandling(lines, tc.ID, tc.Version) writeBodyLines(b, rewritten, tc.Version) // Generate assertion checks for @expect (with renamed variables) @@ -175,8 +183,13 @@ func writeTestBlock(b *strings.Builder, tc TestCase, index int) { } writeExpectAggregates(b, " ", renamed) for _, exp := range renamed { - writeExpectAssertion(b, tc.ID, exp) + writeExpectAssertion(b, tc.ID, exp, evaluated) } + b.WriteString(fmt.Sprintf(" IF %s < %d THEN\n", evaluated, len(tc.Expects))) + b.WriteString(fmt.Sprintf(" LOG ERROR NODE 'MXTEST' '%s';\n", + escapeMDLString("MXTEST:FAIL:"+tc.ID+":"+assertionsSkippedReason))) + b.WriteString(" SET $AllPassed = false;\n") + b.WriteString(" END IF;\n") } else if !tc.expectsThrow() { // No expectations — just check it didn't throw b.WriteString(" IF $TestFailed = false THEN\n") @@ -214,7 +227,7 @@ func writeThrowsTestBlock(b *strings.Builder, tc TestCase, suffix string) { renamedMDL := renameVariables(tc.MDL, varNames, suffix) lines := strings.Split(renamedMDL, "\n") - rewritten := rewriteForThrowsTest(lines, didThrowVar, tc.Throws) + rewritten := rewriteForThrowsTest(lines, didThrowVar, tc.Throws, tc.Version) writeBodyLines(b, rewritten, tc.Version) // Check that it did throw @@ -238,13 +251,15 @@ func writeThrowsTestBlock(b *strings.Builder, tc TestCase, suffix string) { // Unlike the endpoint generator this path reports failures through the log // protocol, whose message is a single log line, so the observed value is // concatenated into that line rather than into a returned verdict. -func writeExpectAssertion(b *strings.Builder, testID string, exp Expect) { +func writeExpectAssertion(b *strings.Builder, testID string, exp Expect, evaluated string) { passCondition := fmt.Sprintf("$TestFailed = false and (%s)", exp.Condition) failMsg := "MXTEST:FAIL:" + testID + ":Expected " + exp.Raw b.WriteString(fmt.Sprintf(" IF %s THEN\n", passCondition)) + b.WriteString(fmt.Sprintf(" SET %s = %s + 1;\n", evaluated, evaluated)) b.WriteString(fmt.Sprintf(" LOG INFO NODE 'MXTEST' 'MXTEST:PASS:%s';\n", escapeMDLString(testID))) b.WriteString(" ELSE\n") + b.WriteString(fmt.Sprintf(" SET %s = %s + 1;\n", evaluated, evaluated)) if exp.Actual == "" { b.WriteString(fmt.Sprintf(" LOG ERROR NODE 'MXTEST' '%s';\n", escapeMDLString(failMsg))) } else { @@ -339,52 +354,16 @@ func renameExpect(exp Expect, names map[string]bool, suffix string) Expect { // rewriteWithErrorHandling adds ON ERROR clauses to the calls in the body. // The error handler ends with RETURN $AllPassed to properly terminate the flow path -// with a return value (required when the microflow returns a type). -func rewriteWithErrorHandling(lines []string, testID string) []string { - var result []string - - for i := 0; i < len(lines); i++ { - line := lines[i] - trimmed := strings.TrimSpace(line) - - if startsErrorRaisingCall(trimmed) && strings.HasSuffix(trimmed, ";") { - withoutSemicolon := strings.TrimSuffix(trimmed, ";") - result = append(result, withoutSemicolon+" ON ERROR BEGIN") - result = append(result, fmt.Sprintf( - " LOG ERROR NODE 'MXTEST' 'MXTEST:FAIL:%s:Exception during execution';", - escapeMDLString(testID))) - result = append(result, " SET $TestFailed = true;") - result = append(result, " SET $AllPassed = false;") - result = append(result, " RETURN $AllPassed;") - result = append(result, "END ERROR;") - } else if startsErrorRaisingCall(trimmed) && !strings.HasSuffix(trimmed, ";") { - var callLines []string - callLines = append(callLines, line) - for i+1 < len(lines) { - i++ - nextLine := lines[i] - callLines = append(callLines, nextLine) - if strings.HasSuffix(strings.TrimSpace(nextLine), ";") { - break - } - } - joined := strings.Join(callLines, "\n") - joined = strings.TrimSpace(joined) - joined = strings.TrimSuffix(joined, ";") - result = append(result, joined+" ON ERROR BEGIN") - result = append(result, fmt.Sprintf( - " LOG ERROR NODE 'MXTEST' 'MXTEST:FAIL:%s:Exception during execution';", - escapeMDLString(testID))) - result = append(result, " SET $TestFailed = true;") - result = append(result, " SET $AllPassed = false;") - result = append(result, " RETURN $AllPassed;") - result = append(result, "END ERROR;") - } else { - result = append(result, line) - } - } - - return result +// with a return value (required when the microflow returns a type). Which calls +// get one is attachOnError's decision, shared with the endpoint runner. +func rewriteWithErrorHandling(lines []string, testID string, v langver.Version) []string { + return attachOnError(lines, v, []string{ + fmt.Sprintf(" LOG ERROR NODE 'MXTEST' 'MXTEST:FAIL:%s:Exception during execution';", + escapeMDLString(testID)), + " SET $TestFailed = true;", + " SET $AllPassed = false;", + " RETURN $AllPassed;", + }) } // rewriteForThrowsTest wraps each call in the body with ON ERROR that sets the throw flag. @@ -393,41 +372,8 @@ func rewriteWithErrorHandling(lines []string, testID string) []string { // runner reports a wrong expectation the same way the endpoint runner does — as // "expected an exception but none was thrown", which is the only outcome its log // protocol has for a throws test that did not get what it asked for. -func rewriteForThrowsTest(lines []string, didThrowVar string, want string) []string { - var result []string - - for i := 0; i < len(lines); i++ { - line := lines[i] - trimmed := strings.TrimSpace(line) - - if startsErrorRaisingCall(trimmed) && strings.HasSuffix(trimmed, ";") { - withoutSemicolon := strings.TrimSuffix(trimmed, ";") - result = append(result, withoutSemicolon+" ON ERROR BEGIN") - result = append(result, throwsFlagHandler(didThrowVar, want)...) - result = append(result, "END ERROR;") - } else if startsErrorRaisingCall(trimmed) && !strings.HasSuffix(trimmed, ";") { - var callLines []string - callLines = append(callLines, line) - for i+1 < len(lines) { - i++ - nextLine := lines[i] - callLines = append(callLines, nextLine) - if strings.HasSuffix(strings.TrimSpace(nextLine), ";") { - break - } - } - joined := strings.Join(callLines, "\n") - joined = strings.TrimSpace(joined) - joined = strings.TrimSuffix(joined, ";") - result = append(result, joined+" ON ERROR BEGIN") - result = append(result, throwsFlagHandler(didThrowVar, want)...) - result = append(result, "END ERROR;") - } else { - result = append(result, line) - } - } - - return result +func rewriteForThrowsTest(lines []string, didThrowVar string, want string, v langver.Version) []string { + return attachOnError(lines, v, throwsFlagHandler(didThrowVar, want)) } // throwsFlagHandler sets the did-throw flag, gated on the expected message when @@ -444,22 +390,28 @@ func throwsFlagHandler(didThrowVar, want string) []string { } } -// startsErrorRaisingCall reports whether a line starts a call-shaped activity -// the runner must catch an error from: one that takes an ON ERROR clause in a -// microflow (onErrorClause on its rule in MDLMicroflow.g4). +// errorRaisingCallRules are the statements the runner must catch an error from: +// the call-shaped activities, named by their rule in MDLMicroflow.g4, each of +// which takes an onErrorClause there. TestErrorRaisingCallRulesAreGrammarRules +// fails if one is renamed, which would otherwise silently stop wrapping it. // // It used to match CALL MICROFLOW and CALL NANOFLOW only, so an error raised by // CALL JAVA ACTION escaped the test microflow — @throws could not catch it and // a test without one was reported as ERROR instead of FAIL (ako/mxcli#1164). -// Anchored at the statement start so a string that mentions a call is not one. -func startsErrorRaisingCall(s string) bool { - return errorRaisingCall.MatchString(s) +// It is a set of parsed statements rather than a pattern over a line, so a +// string or a comment that mentions a call is not one. +var errorRaisingCallRules = map[string]bool{ + "callMicroflowStatement": true, + "callNanoflowStatement": true, + "callJavaActionStatement": true, + "callExternalActionStatement": true, + "callWebServiceStatement": true, + "callWorkflowStatement": true, + "restCallStatement": true, + "sendRestRequestStatement": true, + "executeDatabaseQueryStatement": true, } -var errorRaisingCall = regexp.MustCompile(`(?i)^\s*(\$\w+\s*=\s*)?(` + - `call\s+(microflow|nanoflow|java\s+action|external\s+action|web\s+service|rest\s+service|workflow)` + - `|rest\s+call|send\s+rest\s+request|execute\s+database\s+query)\b`) - // escapeMDLString escapes single quotes for MDL string literals. func escapeMDLString(s string) string { return strings.ReplaceAll(s, "'", "''") diff --git a/cmd/mxcli/testrunner/generator_endpoint.go b/cmd/mxcli/testrunner/generator_endpoint.go index 2d0cfb53a2..c9ff710e28 100644 --- a/cmd/mxcli/testrunner/generator_endpoint.go +++ b/cmd/mxcli/testrunner/generator_endpoint.go @@ -6,7 +6,9 @@ import ( "fmt" "strings" + "github.com/mendixlabs/mxcli/mdl/ast" "github.com/mendixlabs/mxcli/mdl/langver" + "github.com/mendixlabs/mxcli/mdl/visitor" ) // Verdict protocol. A test microflow returns one string: either verdictPass, or @@ -95,12 +97,53 @@ func writeSetupCalls(b *strings.Builder, tc TestCase) { // writeExpectFlowBody writes the body of a normal test: run the MDL, then check // each @expect. An error during the body short-circuits to a FAIL verdict. +// +// Every assertion counts itself when it is evaluated, and the test fails if the +// count falls short (writeAssertionsEvaluatedGuard). See assertionsEvaluatedVar +// for the path that skips assertions without failing anything. func writeExpectFlowBody(b *strings.Builder, tc TestCase) { + if len(tc.Expects) > 0 { + fmt.Fprintf(b, " DECLARE %s Integer = 0;\n", assertionsEvaluatedVar) + } writeBodyLines(b, rewriteBodyForVerdict(strings.Split(tc.MDL, "\n"), tc), tc.Version) writeExpectAggregates(b, " ", tc.Expects) for _, exp := range tc.Expects { writeExpectCheck(b, exp) } + writeAssertionsEvaluatedGuard(b, len(tc.Expects)) +} + +// assertionsEvaluatedVar counts the assertions a test microflow actually +// evaluated. +// +// The count exists because a test can reach its RETURN without passing through +// its assertions, and nothing else would notice. A call the author gave a +// custom handler that ends normally (`on error begin log …; end error;`) +// rejoins the flow — and the microflow builder routes that rejoin past every +// statement reading the failed call's output, since the variable does not +// exist on the error path. The assertions read exactly that output. Measured on +// 11.13.0: a body whose call threw into such a handler reported PASS on +// `@expect $r = 'X'`, with $r never assigned (ako/mxcli#1448). +// +// The increments sit in both branches of each assertion's decision, so running +// one proves that the condition was evaluated; the guard after the last one +// cannot be skipped, because it reads nothing the body produced. +const assertionsEvaluatedVar = "$AssertionsEvaluated" + +// assertionsSkippedReason is the failure a short count reports. +const assertionsSkippedReason = "not every assertion was evaluated: an error in the body was caught by " + + "a call's own ON ERROR handler, and the flow resumed past the assertions that read that call's result" + +// writeAssertionsEvaluatedGuard fails a test that is still passing but did not +// evaluate all n of its assertions. A test that already failed keeps its own, +// more specific verdict. +func writeAssertionsEvaluatedGuard(b *strings.Builder, n int) { + if n == 0 { + return + } + fmt.Fprintf(b, " IF $Verdict = '%s' and %s < %d THEN\n", verdictPass, assertionsEvaluatedVar, n) + fmt.Fprintf(b, " SET $Verdict = '%s';\n", escapeMDLString(verdictFailPrefix+assertionsSkippedReason)) + b.WriteString(" END IF;\n") } // writeExpectAggregates emits the Aggregate list activities the assertions need, @@ -131,7 +174,7 @@ func writeExpectAggregates(b *strings.Builder, indent string, expects []Expect) func writeThrowsFlowBody(b *strings.Builder, tc TestCase) { fmt.Fprintf(b, " SET $Verdict = '%s';\n", escapeMDLString(verdictFailPrefix+"expected an exception but none was thrown")) - writeBodyLines(b, rewriteBodyForThrows(strings.Split(tc.MDL, "\n"), tc.Throws), tc.Version) + writeBodyLines(b, rewriteBodyForThrows(strings.Split(tc.MDL, "\n"), tc.Throws, tc.Version), tc.Version) } // writeExpectCheck writes one @expect assertion. @@ -145,7 +188,9 @@ func writeExpectCheck(b *strings.Builder, exp Expect) { // existing failure with a later assertion's result. fmt.Fprintf(b, " IF $Verdict = '%s' THEN\n", verdictPass) fmt.Fprintf(b, " IF %s THEN\n", exp.Condition) + fmt.Fprintf(b, " SET %s = %s + 1;\n", assertionsEvaluatedVar, assertionsEvaluatedVar) b.WriteString(" ELSE\n") + fmt.Fprintf(b, " SET %s = %s + 1;\n", assertionsEvaluatedVar, assertionsEvaluatedVar) fmt.Fprintf(b, " SET $Verdict = %s;\n", failVerdictExpr(exp)) b.WriteString(" END IF;\n") b.WriteString(" END IF;\n") @@ -179,7 +224,7 @@ func rewriteBodyForVerdict(lines []string, tc TestCase) []string { escapeMDLString(verdictFailPrefix+"exception during execution: "), latestErrorMessage), " RETURN $Verdict;", } - return attachOnError(lines, handler) + return attachOnError(lines, tc.Version, handler) } // rewriteBodyForThrows attaches an ON ERROR handler that clears the pre-set @@ -188,8 +233,8 @@ func rewriteBodyForVerdict(lines []string, tc TestCase) []string { // With a want, the handler clears it only for a matching message and otherwise // replaces it with one naming both sides, so a failing @throws says what was // raised instead of only what was expected. -func rewriteBodyForThrows(lines []string, want string) []string { - return attachOnError(lines, throwsHandler(want)) +func rewriteBodyForThrows(lines []string, want string, v langver.Version) []string { + return attachOnError(lines, v, throwsHandler(want)) } // throwsHandler builds the ON ERROR body of an @throws test. @@ -228,29 +273,100 @@ func quoteForMessage(s string) string { return "'" + s + "'" } -// attachOnError appends `ON ERROR BEGIN ... END ERROR` to each CALL statement in the body, -// joining a statement that spans several lines first. -func attachOnError(lines, handler []string) []string { - var out []string - for i := 0; i < len(lines); i++ { - trimmed := strings.TrimSpace(lines[i]) - if !startsErrorRaisingCall(trimmed) { - out = append(out, lines[i]) +// attachOnError gives each call in the body that has no error handling of its +// own the runner's `ON ERROR BEGIN … END ERROR`, so an error it raises becomes a +// verdict instead of escaping the test microflow. Every rewriting generator — +// both runners, with and without @throws — goes through here, so they cannot +// disagree about which calls are wrapped. +// +// A call the author already gave a clause keeps it, and gets no second one: two +// clauses on one activity is not MDL, and the generated script stopped the run +// at injection with a parse error on a line the author never wrote +// (ako/mxcli#1448). What the author wrote is what runs: +// +// - `on error continue` swallows the error, so the test goes on and its +// assertions decide — exactly what the production statement does. +// - A custom handler (`on error [without rollback] begin … end error`) runs +// as written. One that ends normally rejoins the flow; one that re-raises +// lets the error leave the test microflow, which the endpoint reports as an +// ERROR — never a pass. +// - `on error rollback` is the exception, because it is not a handler: it is +// Mendix's default spelled out, the ErrorHandlingType a call without a +// clause stores too. The two spellings are one model and get one verdict, +// so the clause is replaced by the runner's handler, which also rolls back. +// Keeping it would turn the same exception into an ERROR instead of a FAIL, +// fail an @throws test that should pass, and under the legacy runner abort +// the whole suite's after-startup flow. +// +// Statements are found by parsing the body, not by searching its text: the +// clause may be on a later line, a custom handler holds statements (and +// semicolons) of its own, and `on error` may be inside a string or a comment. +// A body that does not parse is returned unchanged — no handler can make it +// parse, and the error the author then sees is about their own text. +func attachOnError(lines []string, v langver.Version, handler []string) []string { + body := strings.Join(lines, "\n") + + // Parsed inside the same wrapper the generators put it in, under the same + // language version, so it reads exactly as the generated script will. + var wrap strings.Builder + writeScriptHeader(&wrap, v) + wrap.WriteString(createFlow(v) + " " + mxTestModule + ".Body ()\nBEGIN\n") + prefix := wrap.String() + stmts, ok := visitor.FlowStatements(prefix + body + "\nEND;\n") + if !ok { + return lines + } + + runes := []rune(body) + // The handler is laid out at the indentation of the line its call starts + // on, so a call nested in an IF or a loop reads as nested in --verbose. + block := func(callStart int) string { + lineStart := callStart + for lineStart > 0 && runes[lineStart-1] != '\n' { + lineStart-- + } + indent := lineStart + for indent < callStart && (runes[indent] == ' ' || runes[indent] == '\t') { + indent++ + } + pad := string(runes[lineStart:indent]) + var b strings.Builder + b.WriteString("ON ERROR BEGIN\n") + for _, l := range handler { + b.WriteString(pad + l + "\n") + } + b.WriteString(pad + "END ERROR") + return b.String() + } + type edit struct { + start, stop int + text string + } + var edits []edit + offset := len([]rune(prefix)) + for _, s := range stmts { + if !errorRaisingCallRules[s.Rule] { continue } - - stmt := lines[i] - for !strings.HasSuffix(strings.TrimSpace(stmt), ";") && i+1 < len(lines) { - i++ - stmt += "\n" + lines[i] + switch { + case s.OnError == nil: + edits = append(edits, edit{s.Stop - offset, s.Stop - offset, " " + block(s.Start-offset)}) + case s.OnError.Type == ast.ErrorHandlingRollback: + edits = append(edits, edit{s.OnError.Start - offset, s.OnError.Stop - offset, block(s.Start - offset)}) } - stmt = strings.TrimSuffix(strings.TrimSpace(stmt), ";") + } + if len(edits) == 0 { + return lines + } - out = append(out, stmt+" ON ERROR BEGIN") - out = append(out, handler...) - out = append(out, "END ERROR;") + // Back to front, so an edit never moves the offsets of one still to come. + // They cannot overlap: the only statements inside another are in a custom + // handler's body, and a call with a custom handler is never edited itself. + for i := len(edits) - 1; i >= 0; i-- { + e := edits[i] + runes = append(runes[:e.start:e.start], append([]rune(e.text), runes[e.stop:]...)...) } - return out + return strings.Split(string(runes), "\n") } // escapeMDLComment keeps a test name from closing the javadoc block it sits in. diff --git a/cmd/mxcli/testrunner/generator_test.go b/cmd/mxcli/testrunner/generator_test.go index 2b0a434ec9..cfc5ba11bc 100644 --- a/cmd/mxcli/testrunner/generator_test.go +++ b/cmd/mxcli/testrunner/generator_test.go @@ -6,6 +6,7 @@ import ( "strings" "testing" + "github.com/mendixlabs/mxcli/mdl/langver" "github.com/mendixlabs/mxcli/mdl/visitor" ) @@ -17,7 +18,7 @@ func TestRewriteWithErrorHandling_OnlyWrapsCalls(t *testing.T) { "DELETE $p1;", } - got := rewriteWithErrorHandling(lines, "test_1") + got := rewriteWithErrorHandling(lines, "test_1", langver.V0) joined := strings.Join(got, "\n") if strings.Contains(joined, "CHANGE $p1 (IsActive = false) ON ERROR CONTINUE;") { diff --git a/cmd/mxcli/testrunner/own_error_handler_test.go b/cmd/mxcli/testrunner/own_error_handler_test.go new file mode 100644 index 0000000000..f25e6bb98a --- /dev/null +++ b/cmd/mxcli/testrunner/own_error_handler_test.go @@ -0,0 +1,290 @@ +// SPDX-License-Identifier: Apache-2.0 + +package testrunner + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/grammar/parser" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// The runner attaches its own `ON ERROR BEGIN … END ERROR` to every call in a +// test body, so an exception becomes a verdict. A call the author already gave +// an error-handling clause used to get a SECOND one appended, and the generated +// MDL did not parse: the run stopped at injection with "mismatched input 'ON' +// expecting ';'" on a line of a script the author never saw +// (ako/mxcli#1448, mendixlabs/mxcli#1550). +// +// The runner always writes its handler in upper case and these bodies write +// theirs in lower case, so counting "ON ERROR BEGIN" counts the runner's +// handlers only. +const runnerHandler = "ON ERROR BEGIN" + +// generatedBy renders one test body through each of the four generators that +// rewrite a body: the endpoint runner and the legacy after-startup runner, each +// for a test with @expect and one with @throws. +func generatedBy(t *testing.T, body string) map[string]string { + t.Helper() + expect := parseOne(t, "/**\n * @test it runs\n * @expect $r != empty\n */\n"+body+"\n/\n") + throws := parseOne(t, "/**\n * @test it raises\n * @throws 'boom'\n */\n"+body+"\n/\n") + one := func(tc TestCase) *TestSuite { return &TestSuite{Name: "s", Tests: []TestCase{tc}} } + return map[string]string{ + "endpoint @expect": GenerateTestFlows(one(expect)), + "endpoint @throws": GenerateTestFlows(one(throws)), + "legacy @expect": GenerateTestRunner(one(expect)), + "legacy @throws": GenerateTestRunner(one(throws)), + } +} + +func mustParse(t *testing.T, runner, mdl string) { + t.Helper() + if _, errs := visitor.Build(mdl); len(errs) > 0 { + t.Errorf("%s: generated MDL does not parse: %v\n%s", runner, errs[0], mdl) + } +} + +// TestCallWithItsOwnHandlerKeepsIt pins the semantics: a call that continues +// on error, or has a custom handler, keeps exactly what the author wrote — the +// test exercises what the production statement does — and gets no second +// handler from the runner. +func TestCallWithItsOwnHandlerKeepsIt(t *testing.T) { + cases := []struct{ name, body, clause string }{ + {"on error continue", + "$r = call java action Shop.JA_Upper(Text = 'abc') on error continue;", + "on error continue"}, + {"custom handler", + "$r = call java action Shop.JA_Upper(Text = 'abc') on error begin\n log error node 'T' 'handled';\nend error;", + "on error begin"}, + {"custom handler without rollback", + "$r = call java action Shop.JA_Upper(Text = 'abc') on error without rollback begin\n log error node 'T' 'handled';\nend error;", + "on error without rollback begin"}, + {"custom handler that re-raises", + "$r = call java action Shop.JA_Upper(Text = 'abc') on error begin\n log error node 'T' 'handled';\n raise error;\nend error;", + "raise error;"}, + {"deprecated brace form", + "$r = call java action Shop.JA_Upper(Text = 'abc') on error { log error node 'T' 'handled'; };", + "on error {"}, + {"clause on the line after the call", + "$r = call microflow Shop.Order_Label(Text = '1')\n on error continue;", + "on error continue"}, + {"clause after a multi-line argument list", + "$r = call microflow Shop.Order_Label(\n Text = '1'\n) on error continue;", + "on error continue"}, + {"nanoflow call", "$r = call nanoflow Shop.NF(Text = '1') on error continue;", "on error continue"}, + {"rest call", + "$r = call rest service get 'https://example.com' returns String on error continue;", + "on error continue"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + for runner, mdl := range generatedBy(t, c.body) { + mustParse(t, runner, mdl) + if n := strings.Count(mdl, runnerHandler); n != 0 { + t.Errorf("%s: the runner added %d handler(s) to a call that has its own:\n%s", runner, n, mdl) + } + if !strings.Contains(mdl, c.clause) { + t.Errorf("%s: the author's clause %q is gone:\n%s", runner, c.clause, mdl) + } + } + }) + } +} + +// TestOnErrorRollbackIsTheDefaultSpelledOut pins the one clause that is NOT +// kept. `on error rollback` is Mendix's default, the same ErrorHandlingType an +// un-annotated call stores, so `call X()` and `call X() on error rollback` are +// one model and must get one verdict. Keeping the clause would let the error +// escape the test microflow: an ERROR instead of a FAIL, a failed @throws that +// should pass, and under the legacy runner an exception that aborts the whole +// suite's after-startup flow. +func TestOnErrorRollbackIsTheDefaultSpelledOut(t *testing.T) { + // The statement from the report, verbatim, and its microflow twin. + for _, body := range []string{ + "$r = call java action Shop.JA_Upper(Text = 'abc') on error rollback;", + "$r = call microflow Shop.Order_Label(Text = '1') on error rollback;", + "$r = call microflow Shop.Order_Label(Text = '1')\n ON ERROR ROLLBACK;", + } { + for runner, mdl := range generatedBy(t, body) { + mustParse(t, runner, mdl) + if n := strings.Count(mdl, runnerHandler); n != 1 { + t.Errorf("%s: want the runner's handler once in place of `on error rollback`, got %d:\n%s", runner, n, mdl) + } + if strings.Contains(strings.ToLower(mdl), "on error rollback") { + t.Errorf("%s: `on error rollback` was kept beside the runner's handler:\n%s", runner, mdl) + } + } + } +} + +// TestUnhandledCallStillFails is the control for both tests above: a call with +// no clause still gets the handler that turns its exception into a FAIL, and an +// @throws test still gets the one that clears its pre-set failure. +func TestUnhandledCallStillFails(t *testing.T) { + got := generatedBy(t, "$r = call java action Shop.JA_Upper(Text = 'abc');") + for runner, mdl := range got { + mustParse(t, runner, mdl) + if n := strings.Count(mdl, runnerHandler); n != 1 { + t.Errorf("%s: want one runner handler on an unhandled call, got %d:\n%s", runner, n, mdl) + } + } + if !strings.Contains(got["endpoint @expect"], verdictFailPrefix+"exception during execution: ") { + t.Errorf("the endpoint handler does not FAIL the test:\n%s", got["endpoint @expect"]) + } + if !strings.Contains(got["legacy @expect"], "MXTEST:FAIL:") { + t.Errorf("the legacy handler does not FAIL the test:\n%s", got["legacy @expect"]) + } +} + +// TestOnErrorTextThatIsNotAClause pins that the clause is found by parsing, not +// by searching text: `on error` in a string literal or a comment is not a clause, +// so the call is unhandled and gets the runner's handler. +func TestOnErrorTextThatIsNotAClause(t *testing.T) { + for name, body := range map[string]string{ + "in a string literal": "$r = call microflow Shop.Order_Label(Text = 'on error continue');", + "in a line comment": "$r = call microflow Shop.Order_Label(Text = '1'); -- on error continue", + "in a block comment": "$r = call microflow Shop.Order_Label(Text = '1') /* on error continue */;", + "in a log after the call": "$r = call microflow Shop.Order_Label(Text = '1');\n" + + "log info node 'T' 'call microflow X() on error rollback';", + } { + t.Run(name, func(t *testing.T) { + for runner, mdl := range generatedBy(t, body) { + mustParse(t, runner, mdl) + if n := strings.Count(mdl, runnerHandler); n != 1 { + t.Errorf("%s: want exactly one runner handler, got %d:\n%s", runner, n, mdl) + } + } + }) + } +} + +// TestHandlerDecisionIsPerCall pins that the decision is made for each call on +// its own: a handled call beside an unhandled one, on one line or nested in a +// block, leaves the unhandled one wrapped. A call inside the author's own +// handler body has no clause of its own, so it is wrapped like any other. +func TestHandlerDecisionIsPerCall(t *testing.T) { + for name, body := range map[string]string{ + "two calls on one line": "$a = call microflow Shop.Order_Label(Text = '1'); " + + "$r = call microflow Shop.Order_Label(Text = '2') on error continue;", + "handled call inside an if": "if true then\n" + + " $r = call microflow Shop.Order_Label(Text = '1') on error continue;\n" + + " $a = call microflow Shop.Order_Label(Text = '2');\n" + + "end if;", + "call inside the author's handler": "$r = call microflow Shop.Order_Label(Text = '1') on error begin\n" + + " $a = call microflow Shop.Order_Label(Text = '2');\n" + + "end error;", + } { + t.Run(name, func(t *testing.T) { + for runner, mdl := range generatedBy(t, body) { + mustParse(t, runner, mdl) + if n := strings.Count(mdl, runnerHandler); n != 1 { + t.Errorf("%s: want the runner's handler on the one unhandled call, got %d:\n%s", runner, n, mdl) + } + } + }) + } +} + +// TestErrorRaisingCallRulesAreGrammarRules pins errorRaisingCallRules to the +// grammar. The set is matched against rule names from the parse, so a rule +// renamed in MDLMicroflow.g4 would silently stop being wrapped — every error it +// raised escaping the test microflow again (ako/mxcli#1164). +func TestErrorRaisingCallRulesAreGrammarRules(t *testing.T) { + parser.MDLParserInit() // the rule names are filled in lazily + known := map[string]bool{} + for _, name := range parser.MDLParserParserStaticData.RuleNames { + known[name] = true + } + if !known["callMicroflowStatement"] || len(errorRaisingCallRules) == 0 { + t.Fatalf("no rule names to check against; the guard would pass vacuously") + } + for rule := range errorRaisingCallRules { + if !known[rule] { + t.Errorf("errorRaisingCallRules names %q, which is not a rule of the MDL grammar", rule) + } + } +} + +// TestEveryAssertionIsCounted pins the guard against the false green that +// keeping the author's handler opens up. A custom handler that ends normally +// rejoins the flow past every statement reading the failed call's output — the +// assertions — so without a count the test returned PASS having evaluated +// nothing (measured on 11.13.0, ako/mxcli#1448). Each assertion counts itself in +// both branches of its decision, and a guard after the last one fails the test +// when the count is short. +func TestEveryAssertionIsCounted(t *testing.T) { + src := "/**\n * @test handled\n * @expect $r = 'X'\n * @expect $r != empty\n */\n" + + "$r = call java action Shop.JA_Fail(Text = 'a') on error begin\n log error node 'T' 'handled';\nend error;\n/\n" + tc := parseOne(t, src) + suite := &TestSuite{Name: "s", Tests: []TestCase{tc}} + + for runner, c := range map[string]struct { + mdl, counter, guard string + }{ + "endpoint": {GenerateTestFlows(suite), assertionsEvaluatedVar, + "IF $Verdict = '" + verdictPass + "' and " + assertionsEvaluatedVar + " < 2 THEN"}, + "legacy": {GenerateTestRunner(suite), assertionsEvaluatedVar + "_1", + "IF " + assertionsEvaluatedVar + "_1 < 2 THEN"}, + } { + mustParse(t, runner, c.mdl) + inc := "SET " + c.counter + " = " + c.counter + " + 1;" + if n := strings.Count(c.mdl, inc); n != 4 { + t.Errorf("%s: want the counter raised in both branches of both assertions (4), got %d:\n%s", runner, n, c.mdl) + } + g := strings.Index(c.mdl, c.guard) + if g < 0 { + t.Fatalf("%s: no guard %q:\n%s", runner, c.guard, c.mdl) + } + if g < strings.LastIndex(c.mdl, inc) { + t.Errorf("%s: the guard comes before the last assertion:\n%s", runner, c.mdl) + } + if !strings.Contains(c.mdl[g:], escapeMDLString(assertionsSkippedReason)) { + t.Errorf("%s: the guard does not fail the test with the reason:\n%s", runner, c.mdl[g:]) + } + } + + // Control: a test that asserts nothing has nothing to count. + bare := parseOne(t, "/**\n * @test bare\n */\n$r = call java action Shop.JA_Upper(Text = 'a');\n/\n") + for runner, mdl := range map[string]string{ + "endpoint": GenerateTestFlows(&TestSuite{Name: "s", Tests: []TestCase{bare}}), + "legacy": GenerateTestRunner(&TestSuite{Name: "s", Tests: []TestCase{bare}}), + } { + if strings.Contains(mdl, assertionsEvaluatedVar) { + t.Errorf("%s: a test without @expect got an assertion counter:\n%s", runner, mdl) + } + } +} + +// TestLegacyRunWithoutVerdictIsNotAPass pins the log runner's other false green. +// A test whose error leaves the monolithic runner — a call whose own handler +// re-raises, kept as written since ako/mxcli#1448 — aborts the flow after its +// RUN line and before any verdict. The parser created the result on RUN with +// the zero status, which is PASS: measured on 11.13.0 under --legacy-runner, the +// re-raising test reported PASS and the next one "not executed". +func TestLegacyRunWithoutVerdictIsNotAPass(t *testing.T) { + suite := &TestSuite{Name: "s", Tests: []TestCase{ + {ID: "test_1", Name: "passes", Expects: []Expect{expectOf("$a = 1")}}, + {ID: "test_2", Name: "re-raises", Expects: []Expect{expectOf("$b = 1")}}, + {ID: "test_3", Name: "never reached", Expects: []Expect{expectOf("$c = 1")}}, + }} + const log = "MXTEST: MXTEST:START:\n" + + "MXTEST: MXTEST:RUN:test_1:passes\n" + + "MXTEST: MXTEST:PASS:test_1\n" + + "MXTEST: MXTEST:RUN:test_2:re-raises\n" + got := ParseLogResults(strings.NewReader(log), suite, false) + if s := got.Tests[0].Status; s != StatusPass { + t.Errorf("control: test_1 logged PASS, status = %v", s) + } + if s := got.Tests[1].Status; s == StatusPass { + t.Errorf("test_2 logged RUN and no verdict, and reads as PASS") + } else if !strings.Contains(got.Tests[1].Message, "no verdict") { + t.Errorf("test_2: message %q does not say it reported no verdict", got.Tests[1].Message) + } + if s := got.Tests[2].Status; s != StatusError { + t.Errorf("control: test_3 never ran, status = %v", s) + } + if got.AllPassed() { + t.Error("a suite whose runner aborted mid-test passed") + } +} diff --git a/cmd/mxcli/testrunner/results.go b/cmd/mxcli/testrunner/results.go index 228f636f51..905b232d91 100644 --- a/cmd/mxcli/testrunner/results.go +++ b/cmd/mxcli/testrunner/results.go @@ -229,14 +229,21 @@ func ParseLogResults(logReader io.Reader, suite *TestSuite, requireAssertions bo if len(parts) >= 4 { name = parts[3] } + // Not the zero status, which is PASS. A test that logs RUN and no + // verdict did not finish: an error left the runner flow, which + // stops it here — a call whose own handler re-raises does exactly + // that (ako/mxcli#1448). Its PASS or FAIL line replaces this. resultMap[id] = &TestResult{ - ID: id, - Name: name, + ID: id, + Name: name, + Status: StatusError, + Message: "started and reported no verdict: an error left the test runner here, which stopped it (the runtime log has the error)", } case "PASS": if r, ok := resultMap[id]; ok { r.Status = StatusPass + r.Message = "" if t, ok := runTimes[id]; ok { r.Duration = time.Since(t) } diff --git a/docs-site/src/tools/test-annotations.md b/docs-site/src/tools/test-annotations.md index 2d1c3e9b83..4cd744b272 100644 --- a/docs-site/src/tools/test-annotations.md +++ b/docs-site/src/tools/test-annotations.md @@ -153,6 +153,35 @@ a `change`, a `create` — is not caught; move it into a microflow and call that The same handler turns an unexpected error in a test without `@throws` into a FAIL reading `exception during execution: `. +### A call with its own error handling + +A call that already has an `on error` clause keeps it and gets no second +handler, so a test runs the statement exactly as production code does. What the +test then reports depends on the clause (measured on Mendix 11.13 with a Java +action that throws): + +| Clause on the call | Without `@throws` | With `@throws` | +|---|---|---| +| none | FAIL `exception during execution: ` | PASS when the message matches | +| `on error rollback` | same as none — it is Mendix's default spelled out, so the runner's handler takes its place | same as none | +| `on error continue` | the error is swallowed; the assertions decide (`$r` is empty) | FAIL `expected an exception but none was thrown` | +| custom handler that ends normally | FAIL `not every assertion was evaluated` when an assertion reads the call's result; an assertion on what the handler did is evaluated | FAIL `expected an exception but none was thrown` | +| custom handler that re-raises (`raise error`) | ERROR with the message — the error leaves the test microflow | ERROR with the message | + +A custom handler that ends normally rejoins the flow after every statement that +reads the failed call's output, because that variable does not exist on the +error path. The assertions read exactly that, so the runner counts the +assertions it evaluated and fails the test when the count is short, instead of +reporting a pass for assertions that never ran. To test the handler's path, +assert on something the handler sets: `declare $Failed Boolean = false;` before +the call, `set $Failed = true;` inside its handler, and `@expect $Failed = true`. +That assertion does not read the call's result, so it is evaluated, and passes +only when the handler ran. + +Under the Docker / `--legacy-runner` path an error that leaves the test — the +re-raising row above — ends the whole runner: that test is reported as an ERROR +that started and gave no verdict, and every later test as not executed. + ## `@setup` Names a **microflow** to call before the test's own statements — a fixture in a diff --git a/mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl b/mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl new file mode 100644 index 0000000000..7bf7545c7e --- /dev/null +++ b/mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl @@ -0,0 +1,166 @@ +-- ako/mxcli#1448 (upstream mendixlabs/mxcli#1550): a call in a test body that +-- had its own `on error` clause got a second `ON ERROR BEGIN … END ERROR` +-- appended by the generator, and the generated MDL did not parse ("mismatched +-- input 'ON' expecting ';'") — the run stopped at injection, before the app +-- started. A call now keeps the author's clause and gets no second one; +-- `on error rollback`, Mendix's default spelled out, is treated like no clause. +-- +-- Setup: test-runner-1448-setup.mdl. Then: +-- +-- mxcli test mdl-examples/bug-tests/test-runner-1448-own-error-handler.test.mdl -p app.mpr --local +-- +-- Measured on 11.13.0 (--local --db-type hsqldb). Before the fix: the parse +-- error above and no test runs. After: +-- +-- 01 FAIL exception during execution: boom: a +-- 02 FAIL exception during execution: boom: a (rollback = no clause) +-- 03 FAIL expected $r = 'X', actual: (continue swallowed it) +-- 04 FAIL not every assertion was evaluated: … (handler skipped the @expect) +-- 05 ERROR boom: a (the handler re-raised) +-- 06 ERROR boom: a +-- 07 PASS 08 PASS +-- 09 FAIL expected an exception but none was thrown +-- 10 FAIL expected an exception but none was thrown +-- 11 ERROR boom: a +-- 12-16 PASS +-- +-- 04 is the false green the fix had to close: a custom handler that ends +-- normally rejoins the flow past every statement reading the failed call's +-- output, so the assertion was never evaluated and the test reported PASS. 16 is +-- its control — an assertion on what the handler did is still evaluated. +-- +-- Under --legacy-runner a test whose error leaves the runner (05, 06, 11) ends +-- the whole run; 05 was reported PASS there (a RUN line with no verdict read as +-- the zero status) and is now an ERROR. That runner also needs a distinct +-- variable name per test. +mdl 1; +/** + * @test 01 plain call that throws + * @expect $r = 'X' + */ +$r = call java action Shop.JA_Fail(Text = 'a'); +/ + +/** + * @test 02 on error rollback, call throws + * @expect $r = 'X' + */ +$r = call java action Shop.JA_Fail(Text = 'a') on error rollback; +/ + +/** + * @test 03 on error continue, call throws + * @expect $r = 'X' + */ +$r = call java action Shop.JA_Fail(Text = 'a') on error continue; +/ + +/** + * @test 04 custom handler that logs, call throws + * @expect $r = 'X' + */ +$r = call java action Shop.JA_Fail(Text = 'a') on error begin + log error node 'T' 'handled'; +end error; +/ + +/** + * @test 05 custom handler that re-raises, call throws + * @expect $r = 'X' + */ +$r = call java action Shop.JA_Fail(Text = 'a') on error begin + log error node 'T' 'handled'; + raise error; +end error; +/ + +/** + * @test 06 custom handler without rollback that re-raises, call throws + * @expect $r = 'X' + */ +$r = call java action Shop.JA_Fail(Text = 'a') on error without rollback begin + log error node 'T' 'handled'; + raise error; +end error; +/ + +/** + * @test 07 throws: plain call + * @throws 'boom' + */ +$r = call java action Shop.JA_Fail(Text = 'a'); +/ + +/** + * @test 08 throws: on error rollback + * @throws 'boom' + */ +$r = call java action Shop.JA_Fail(Text = 'a') on error rollback; +/ + +/** + * @test 09 throws: on error continue + * @throws 'boom' + */ +$r = call java action Shop.JA_Fail(Text = 'a') on error continue; +/ + +/** + * @test 10 throws: custom handler that logs + * @throws 'boom' + */ +$r = call java action Shop.JA_Fail(Text = 'a') on error begin + log error node 'T' 'handled'; +end error; +/ + +/** + * @test 11 throws: custom handler that re-raises + * @throws 'boom' + */ +$r = call java action Shop.JA_Fail(Text = 'a') on error begin + log error node 'T' 'handled'; + raise error; +end error; +/ + +/** + * @test 12 the report: java action with on error rollback, call succeeds + * @expect $Result = 'ABC' + */ +$Result = call java action Shop.JA_Upper(Text = 'abc') on error rollback; +/ + +/** + * @test 13 the report: microflow with on error rollback, call succeeds + * @expect $Result = 'Order 1' + */ +$Result = call microflow Shop.Order_Label(Text = '1') on error rollback; +/ + +/** + * @test 14 on error continue, call succeeds + * @expect $r = 'ABC' + */ +$r = call java action Shop.JA_Upper(Text = 'abc') on error continue; +/ + +/** + * @test 15 custom handler, call succeeds + * @expect $r = 'ABC' + */ +$r = call java action Shop.JA_Upper(Text = 'abc') on error begin + log error node 'T' 'handled'; + raise error; +end error; +/ + +/** + * @test 16 custom handler path, asserted on what the handler did + * @expect $Handled = true + */ +declare $Handled Boolean = false; +$r = call java action Shop.JA_Fail(Text = 'a') on error begin + set $Handled = true; +end error; +/ diff --git a/mdl-examples/bug-tests/test-runner-1448-setup.mdl b/mdl-examples/bug-tests/test-runner-1448-setup.mdl new file mode 100644 index 0000000000..05973be5aa --- /dev/null +++ b/mdl-examples/bug-tests/test-runner-1448-setup.mdl @@ -0,0 +1,26 @@ +-- ako/mxcli#1448 (upstream mendixlabs/mxcli#1550): the setup script for +-- test-runner-1448-own-error-handler.test.mdl. Apply it before running the tests: +-- +-- mxcli exec mdl-examples/bug-tests/test-runner-1448-setup.mdl -p app.mpr +-- +-- JA_Fail throws on every call, so each clause's verdict on an exception can be +-- read off one run. +mdl 1; +create module Shop; + +create java action Shop.JA_Upper (Text: String not null) returns String +as $$ + return Text.toUpperCase(); +$$; + +create java action Shop.JA_Fail (Text: String not null) returns String +as $$ + throw new Exception("boom: " + Text); +$$; + +create microflow Shop.Order_Label ($Text: String) + returns String as $Label +begin + declare $Label String = 'Order ' + $Text; + return $Label; +end; diff --git a/mdl/visitor/flow_statements.go b/mdl/visitor/flow_statements.go new file mode 100644 index 0000000000..3290e1629e --- /dev/null +++ b/mdl/visitor/flow_statements.go @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "github.com/antlr4-go/antlr/v4" + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/grammar/parser" + "github.com/mendixlabs/mxcli/mdl/langver" +) + +// FlowStatement is one statement of a microflow body, located in the text it +// was parsed from. It exists for code that rewrites a body as text and has to +// know where a statement ends and whether it already handles its own errors — +// the `mxcli test` generators, which attach an error handler to every call in +// a test body (ako/mxcli#1448). Searching the text for `on error` cannot answer +// that: the clause may sit on a later line, a custom handler holds statements +// of its own, and the words may be inside a string or a comment. +type FlowStatement struct { + // Rule is the grammar rule the statement matched — the alternative of + // microflowStatement, e.g. "callMicroflowStatement". + Rule string + // Start and Stop are the rune offsets [Start, Stop) of the statement, + // without its annotations and without the `;` that terminates it. + Start, Stop int + // OnError is the statement's own ON ERROR clause, nil when it has none. + // A clause on a statement inside a nested body (an IF branch, a loop, a + // handler) belongs to that statement, which is listed separately. + OnError *FlowOnError +} + +// FlowOnError is an ON ERROR clause, located. +type FlowOnError struct { + Type ast.ErrorHandlingType + // Start and Stop are the rune offsets [Start, Stop) of the whole clause, + // from `on` to `rollback`, `continue`, `end error` or the closing brace. + Start, Stop int +} + +// FlowStatements parses src as a script and returns every microflow-body +// statement in it, nested ones included, in source order. ok is false when src +// does not parse, and the caller must then leave the text alone. +// +// Offsets are in runes, as ANTLR counts them: index []rune(src), not src. +func FlowStatements(src string) (stmts []FlowStatement, ok bool) { + errs := newErrorListener() + lexer := parser.NewMDLLexer(newScriptStream(src, langver.V0)) + lexer.RemoveErrorListeners() + lexer.AddErrorListener(errs) + stream := antlr.NewCommonTokenStream(lexer, antlr.TokenDefaultChannel) + p := parser.NewMDLParser(stream) + p.RemoveErrorListeners() + p.AddErrorListener(errs) + tree := p.Program() + if len(errs.errors) > 0 { + return nil, false + } + ruleNames := p.GetRuleNames() + + var walk func(t antlr.Tree) + walk = func(t antlr.Tree) { + if ms, isStmt := t.(*parser.MicroflowStatementContext); isStmt { + if fs, found := flowStatement(ms, ruleNames); found { + stmts = append(stmts, fs) + } + } + for i := 0; i < t.GetChildCount(); i++ { + walk(t.GetChild(i)) + } + } + walk(tree) + return stmts, true +} + +// flowStatement locates one microflowStatement: the statement rule inside it, +// past any annotations, and that rule's own ON ERROR clause. +func flowStatement(ms *parser.MicroflowStatementContext, ruleNames []string) (FlowStatement, bool) { + for i := 0; i < ms.GetChildCount(); i++ { + inner, isRule := ms.GetChild(i).(antlr.ParserRuleContext) + if !isRule { + continue + } + if _, isAnnotation := inner.(*parser.AnnotationContext); isAnnotation { + continue + } + start, stop := inner.GetStart(), inner.GetStop() + if start == nil || stop == nil { + return FlowStatement{}, false + } + fs := FlowStatement{ + Rule: ruleNames[inner.GetRuleIndex()], + Start: start.GetStart(), + Stop: stop.GetStop() + 1, + } + if oe := ownOnErrorClause(inner); oe != nil { + clause := buildOnErrorClause(oe) + if clause != nil { + fs.OnError = &FlowOnError{ + Type: clause.Type, + Start: oe.GetStart().GetStart(), + Stop: oe.GetStop().GetStop() + 1, + } + } + } + return fs, true + } + return FlowStatement{}, false +} + +// ownOnErrorClause finds the ON ERROR clause that belongs to stmt itself. It +// does not descend into a nested body, whose statements own their clauses. +func ownOnErrorClause(t antlr.Tree) *parser.OnErrorClauseContext { + for i := 0; i < t.GetChildCount(); i++ { + switch c := t.GetChild(i).(type) { + case *parser.OnErrorClauseContext: + return c + case *parser.MicroflowBodyContext: + continue + case antlr.ParserRuleContext: + if found := ownOnErrorClause(c); found != nil { + return found + } + } + } + return nil +} diff --git a/mdl/visitor/flow_statements_test.go b/mdl/visitor/flow_statements_test.go new file mode 100644 index 0000000000..38e86e14b5 --- /dev/null +++ b/mdl/visitor/flow_statements_test.go @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: Apache-2.0 + +package visitor + +import ( + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" +) + +func TestFlowStatementsLocatesEachStatementAndItsOwnClause(t *testing.T) { + src := "mdl 1;\ncreate or modify microflow M.F ()\nbegin\n" + + " $a = call microflow M.A(Msg = 'on error continue'); -- on error rollback\n" + + " $b = call microflow M.B()\n on error rollback;\n" + + " $c = call java action M.C() on error begin\n" + + " $d = call microflow M.D() on error continue;\n" + + " end error;\n" + + " log info node 'T' 'é';\n" + + "end;\n" + stmts, ok := FlowStatements(src) + if !ok { + t.Fatal("the script did not parse") + } + runes := []rune(src) + text := func(start, stop int) string { return string(runes[start:stop]) } + + type want struct { + rule, text string + onError ast.ErrorHandlingType // "" = no clause of its own + clause string + } + wants := []want{ + {"callMicroflowStatement", "$a = call microflow M.A(Msg = 'on error continue')", "", ""}, + {"callMicroflowStatement", "$b = call microflow M.B()\n on error rollback", ast.ErrorHandlingRollback, "on error rollback"}, + {"callJavaActionStatement", "", ast.ErrorHandlingCustom, ""}, + {"callMicroflowStatement", "$d = call microflow M.D() on error continue", ast.ErrorHandlingContinue, "on error continue"}, + {"logStatement", "log info node 'T' 'é'", "", ""}, + } + if len(stmts) != len(wants) { + t.Fatalf("got %d statements, want %d: %+v", len(stmts), len(wants), stmts) + } + for i, w := range wants { + s := stmts[i] + if s.Rule != w.rule { + t.Errorf("statement %d: rule %q, want %q", i, s.Rule, w.rule) + } + if w.text != "" && text(s.Start, s.Stop) != w.text { + t.Errorf("statement %d: text %q, want %q", i, text(s.Start, s.Stop), w.text) + } + switch { + case w.onError == "" && s.OnError != nil: + t.Errorf("statement %d: found a clause %q that is not its own", i, text(s.OnError.Start, s.OnError.Stop)) + case w.onError != "" && s.OnError == nil: + t.Errorf("statement %d: its %s clause was not found", i, w.onError) + case w.onError != "" && s.OnError.Type != w.onError: + t.Errorf("statement %d: clause type %s, want %s", i, s.OnError.Type, w.onError) + case w.clause != "" && text(s.OnError.Start, s.OnError.Stop) != w.clause: + t.Errorf("statement %d: clause text %q, want %q", i, text(s.OnError.Start, s.OnError.Stop), w.clause) + } + } + // The custom handler's clause spans its whole body, nested call included. + if c := stmts[2].OnError; c != nil { + if got := text(c.Start, c.Stop); got[:14] != "on error begin" || got[len(got)-9:] != "end error" { + t.Errorf("custom clause text %q", got) + } + } +} + +func TestFlowStatementsRefusesAScriptThatDoesNotParse(t *testing.T) { + if _, ok := FlowStatements("create microflow M.F () begin $a = call microflow M.A() on error rollback on error continue; end;"); ok { + t.Error("a statement with two clauses parsed") + } +} From 130a4048f18e21bfc0537c338a603915a4894d47 Mon Sep 17 00:00:00 2001 From: Ako Date: Tue, 27 Oct 2026 16:13:12 +0000 Subject: [PATCH 2/2] fix: mxcli test --local fails CE0114 when the app's after-startup microflow applies entity access (ako/mxcli#1654) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mxcli test --local` generates MxTest.RegisterEndpoint, the startup microflow that registers the test endpoint and then calls the project's own after-startup microflow. It was written without "apply entity access", and MxBuild refuses a microflow that does not apply entity access calling one that does (CE0114). In any project whose after-startup microflow applies entity access the build failed, no test ran, and the error said "These errors are in the project, not in this run's tests" about a microflow mxcli generated. Upstream: mendixlabs/mxcli#1645. Measured direction (mx check, Mendix 11.13.0, security Off and Production): non-applying -> applying is CE0114; applying -> non-applying and applying -> applying (with a Java action call beside it) are 0 errors, and an applying microflow is accepted as the after-startup microflow. MDL-SEC22 records the same table for 11.14.0. Fix: - GenerateEndpointMDL writes `@applyentityaccess` on RegisterEndpoint. Since an applying caller may call either kind, a constant serves every project with nothing read from the user's microflow (the suggested mirror would have added a second scrape of describe output). It governs only the flow's own activities: the Java action ignores it, the chained microflow runs with its own setting. Also covers `run --local --test-endpoint`, which shares the generator. - buildFailureHint attributes an error located in MxTest.RegisterEndpoint or MxTest.RegisterTestEndpoint to mxcli — "not part of your project: mxcli generated it for this run … a defect in mxcli … please report it" — with --skip-app-startup as the escape hatch, and says "in the project" only for the errors that are. Verification: on a blank 11.13.0 app with mdl-examples/bug-tests/test-runner-1654-setup.mdl (startup microflow applies entity access and leaves a marker), the baseline binary fails with the reported CE0114; the fixed one passes both tests, the second proving the startup microflow ran at boot. Controls: the same run with --skip-app-startup fails that test ("expected $Count > 0, actual: 0") — the test consumes the marker so an earlier boot's cannot pass it; a non-applying startup microflow still passes both. A binary with only the attribution half shows the new message on the live CE0114. Revert proof (patch method): with endpoint.go and build_attribution.go checked out, TestRegisterEndpointAppliesEntityAccess and TestBuildErrorInGeneratedStartupFlowIsMxclis fail; the project-only control passes either way. Co-Authored-By: Claude Opus 5.5 --- ...erated-registerendpoint-entity-access.json | 1 + .../skills/mendix/test-microflows/SKILL.md | 22 +++-- cmd/mxcli/testrunner/build_attribution.go | 69 +++++++++++++- cmd/mxcli/testrunner/endpoint.go | 20 +++- .../testrunner/startup_entity_access_test.go | 94 +++++++++++++++++++ docs-site/src/tools/running-tests.md | 8 ++ .../bug-tests/test-runner-1654-setup.mdl | 31 ++++++ ...654-startup-applies-entity-access.test.mdl | 41 ++++++++ 8 files changed, 270 insertions(+), 16 deletions(-) create mode 100644 .claude/skills/fix-issue/findings/cmd-mxcli/2026-10-27-mxcli-test-local-ce0114-generated-registerendpoint-entity-access.json create mode 100644 cmd/mxcli/testrunner/startup_entity_access_test.go create mode 100644 mdl-examples/bug-tests/test-runner-1654-setup.mdl create mode 100644 mdl-examples/bug-tests/test-runner-1654-startup-applies-entity-access.test.mdl diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-27-mxcli-test-local-ce0114-generated-registerendpoint-entity-access.json b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-27-mxcli-test-local-ce0114-generated-registerendpoint-entity-access.json new file mode 100644 index 0000000000..f7633311a4 --- /dev/null +++ b/.claude/skills/fix-issue/findings/cmd-mxcli/2026-10-27-mxcli-test-local-ce0114-generated-registerendpoint-entity-access.json @@ -0,0 +1 @@ +{"area": "cmd/mxcli", "date": "2026-10-27", "symptom": "`mxcli test --local` fails the build with `[CE0114] A microflow that does not apply entity access can only call microflows that also do not apply entity access. — at MxTest / Microflow 'RegisterEndpoint' / Call microflow activity ''` whenever the project's after-startup microflow applies entity access; no test runs, and the error says it is \"in the project, not in this run's tests\"", "cause": "`GenerateEndpointMDL` wrote `MxTest.RegisterEndpoint` without `@applyentityaccess`, so it did not apply entity access, and since the 2026-08 chaining fix it calls the project's after-startup microflow. `buildFailureHint` called every error not in a `Test_*` flow \"in the project\", including ones in the two documents the run itself generates", "file": "`cmd/mxcli/testrunner/endpoint.go` (`GenerateEndpointMDL`), `cmd/mxcli/testrunner/build_attribution.go` (`buildFailureHint`, `splitRunnerDocuments`)", "insight": "Measure the rule's DIRECTION before choosing between mirroring the callee and a constant: with mx check on 11.13.0 (security Off and Production), non-applying -> applying is CE0114 while applying -> non-applying and applying -> applying are 0 errors (MDL-SEC22's table says the same for 11.14.0). So `@applyentityaccess` on the generated caller is compatible with every startup microflow, and needs no scrape of the user's flow — which the mirror fix would have added beside the DESCRIBE SETTINGS scrape that already broke once. The setting is neutral at runtime: it governs the caller's own activities only (a Java action ignores it), each callee runs with its own. Prove the chain still runs with a marker the test CONSUMES (`@cleanup none` + delete): a plain count passed on an earlier boot's marker under --skip-app-startup, i.e. the control could not fail. Generated documents with fixed names (RegisterEndpoint, RegisterTestEndpoint) are attributable exactly like the Test_* flows; say so and offer --skip-app-startup as the escape hatch", "refs": ["ako/mxcli#1654", "mendixlabs/mxcli#1645", "mendixlabs/mxcli#1500"], "ce": ["CE0114"]} diff --git a/.claude/skills/mendix/test-microflows/SKILL.md b/.claude/skills/mendix/test-microflows/SKILL.md index e62bb58c8b..0f18a1655a 100644 --- a/.claude/skills/mendix/test-microflows/SKILL.md +++ b/.claude/skills/mendix/test-microflows/SKILL.md @@ -16,14 +16,14 @@ description: "Write and run MDL-based microflow tests with `mxcli test` — anno | 180-222 | Test File Formats | | 223-238 | string Operations | | 239-262 | Running Tests | -| 263-450 | How It Works | -| 451-489 | Check a test file before you run it | -| 490-611 | Writing Good Tests | -| 612-626 | Test File Organization | -| 627-639 | Interpreting Failures | -| 640-661 | CI Integration | -| 662-668 | Related Skills | -| 669-671 | Warm test loop (`mxcli test --local [--watch]`, `--attach`, `run --local --test-endpoint`) | +| 263-456 | How It Works | +| 457-495 | Check a test file before you run it | +| 496-617 | Writing Good Tests | +| 618-632 | Test File Organization | +| 633-645 | Interpreting Failures | +| 646-667 | CI Integration | +| 668-674 | Related Skills | +| 675-677 | Warm test loop (`mxcli test --local [--watch]`, `--attach`, `run --local --test-endpoint`) | # Test Microflows Skill @@ -315,6 +315,12 @@ chained, `--local` ran with the app's startup logic suppressed, and a suite that depended on startup state passed under `--attach` and failed under `--local` for reasons unrelated to the code. +`MxTest.RegisterEndpoint` applies entity access, so it can call a startup +microflow that does or does not (CE0114 only forbids a non-applying microflow +calling an applying one); yours still runs with its own setting. A build error +located in `MxTest.RegisterEndpoint` or `MxTest.RegisterTestEndpoint` is reported +as an mxcli defect, not as an error in your project. + One thing rollback does **not** cover: whatever the startup microflow writes happens at boot, outside any test's transaction, so `@cleanup rollback` does not undo it. Under `--local` that lands in the scratch `_test` database; diff --git a/cmd/mxcli/testrunner/build_attribution.go b/cmd/mxcli/testrunner/build_attribution.go index f49a66b335..e17b44e667 100644 --- a/cmd/mxcli/testrunner/build_attribution.go +++ b/cmd/mxcli/testrunner/build_attribution.go @@ -175,7 +175,7 @@ func resultsFromFailedBuild(problems []docker.BuildProblem, suite *TestSuite) [] // buildFailureHint is appended to the error when a build failure could not be // attributed to any test, which means it is in the project rather than in the -// suite. +// suite — or in one of the documents the run generates besides its tests. // // The errors themselves are already rendered by BuildFailedError.Error(), so // this says what the reader cannot work out from them: that none of them belongs @@ -183,13 +183,34 @@ func resultsFromFailedBuild(problems []docker.BuildProblem, suite *TestSuite) [] // that it is a leftover from an earlier run, with the command to remove it. That // last case is the one that fails every subsequent run of every test file until // someone finds it by hand (mendixlabs/mxcli#1104). +// +// An error in the endpoint documents is mxcli's own, and says so. CE0114 in +// MxTest.RegisterEndpoint used to be reported as "in the project", which sent +// the reader through their own model for a defect in a microflow they never +// wrote (ako/mxcli#1654). func buildFailureHint(other []docker.BuildProblem) string { if len(other) == 0 { return "" } + generated, project := splitRunnerDocuments(other) var b strings.Builder - b.WriteString("\n These errors are in the project, not in this run's tests.") - for _, name := range leftoverFlowsIn(other) { + for _, name := range generated { + b.WriteString(fmt.Sprintf("\n %s is not part of your project: mxcli generated it for this run, %s."+ + " An error in it is a defect in mxcli, not in your project or your tests — please report it"+ + " (`mxcli diag --bundle` collects what is needed).", name, runnerDocuments[name])) + if name == endpointStartupFlow { + b.WriteString("\n Until it is fixed, --skip-app-startup runs the tests without the call to your after-startup microflow.") + } + } + if len(project) == 0 { + return b.String() + } + if len(generated) > 0 { + b.WriteString("\n The other errors are in the project, not in this run's tests.") + } else { + b.WriteString("\n These errors are in the project, not in this run's tests.") + } + for _, name := range leftoverFlowsIn(project) { b.WriteString(fmt.Sprintf( "\n %s is a microflow an earlier `mxcli test` run left behind. Remove it with:"+ "\n mxcli -p -c \"DROP MICROFLOW %s\"", name, name)) @@ -197,6 +218,48 @@ func buildFailureHint(other []docker.BuildProblem) string { return b.String() } +// runnerDocuments are the documents an endpoint run generates besides its test +// microflows, with what each is for. Their names are fixed, so an error located +// in one is the run's own whatever else is in the MxTest module. +var runnerDocuments = map[string]string{ + endpointStartupFlow: "to register the test endpoint at startup and then run your after-startup microflow", + endpointRegisterAction: "to register the test endpoint", +} + +// runnerDocumentPattern matches the `document` MxBuild reports — the document's +// kind and its bare name, `Microflow 'RegisterEndpoint'` or +// `Java action 'RegisterTestEndpoint'`; the module is carried separately. +var runnerDocumentPattern = regexp.MustCompile(`^[A-Za-z ]+ '([^']*)'$`) + +// splitRunnerDocuments separates the problems located in a runner document from +// the rest, naming each runner document once, in the order MxBuild reported it. +func splitRunnerDocuments(problems []docker.BuildProblem) (generated []string, rest []docker.BuildProblem) { + seen := map[string]bool{} + for _, p := range problems { + name := "" + for _, loc := range p.Locations { + if !strings.EqualFold(loc.Module, mxTestModule) { + continue + } + if m := runnerDocumentPattern.FindStringSubmatch(loc.Document); m != nil { + if q := mxTestModule + "." + m[1]; runnerDocuments[q] != "" { + name = q + break + } + } + } + if name == "" { + rest = append(rest, p) + continue + } + if !seen[name] { + seen[name] = true + generated = append(generated, name) + } + } + return generated, rest +} + // leftoverFlowsIn names the generated test microflows among a set of build // problems, deduplicated and in the order MxBuild reported them. // diff --git a/cmd/mxcli/testrunner/endpoint.go b/cmd/mxcli/testrunner/endpoint.go index 3b2061aa68..cf1acea01c 100644 --- a/cmd/mxcli/testrunner/endpoint.go +++ b/cmd/mxcli/testrunner/endpoint.go @@ -67,12 +67,21 @@ func testFlowName(tc TestCase) string { return testFlowPrefix + tc.ID } // // chainAfterStartup, when non-empty, is a microflow the generated startup flow // calls after registering the endpoint — the project's own after-startup -// microflow, which this one displaces. +// microflow, which this one displaces. The test runner passes it so tests see +// the app as it really boots, and "" under --skip-app-startup; a dev loop +// hosting the endpoint (`run --local --test-endpoint`) always passes it. // -// The test runner passes "" : a test run wants a known starting state, and the -// suite is the only thing that should execute. A dev loop hosting the endpoint -// (`run --local --test-endpoint`) passes the real one, because the developer's -// app must still seed its data and do whatever else it does at boot. +// The startup flow applies entity access, whatever the chained microflow does. +// MxBuild refuses a microflow that does not apply entity access calling one +// that does (CE0114), and the flow used to be written without the setting, so +// no project whose after-startup microflow applies entity access could run a +// single test (ako/mxcli#1654). The rule has one direction — measured with mx +// check on 11.13.0, an applying microflow may call an applying or a +// non-applying one at 0 errors, under security Off and Production alike (and +// MDL-SEC22 records the same table for 11.14.0) — so a constant serves every +// project, with nothing to read from the chained microflow first. The setting +// governs only this flow's own two activities: the Java action ignores it, and +// the chained microflow runs with its own. func GenerateEndpointMDL(chainAfterStartup string) string { var b strings.Builder @@ -91,6 +100,7 @@ func GenerateEndpointMDL(chainAfterStartup string) string { b.WriteString("\n$$;\n\n") b.WriteString("/** Registers the mxcli test endpoint at boot. Runs no tests. */\n") + b.WriteString("@applyentityaccess\n") b.WriteString(createFlow(langver.V1) + " " + endpointStartupFlow + " ()\n") b.WriteString("RETURNS Boolean AS $Registered\n") b.WriteString("BEGIN\n") diff --git a/cmd/mxcli/testrunner/startup_entity_access_test.go b/cmd/mxcli/testrunner/startup_entity_access_test.go new file mode 100644 index 0000000000..d1c7931a62 --- /dev/null +++ b/cmd/mxcli/testrunner/startup_entity_access_test.go @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: Apache-2.0 + +package testrunner + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/mdl/ast" + "github.com/mendixlabs/mxcli/mdl/visitor" +) + +// TestRegisterEndpointAppliesEntityAccess pins the generated startup flow's +// "apply entity access" setting (ako/mxcli#1654, mendixlabs/mxcli#1645). +// +// MxTest.RegisterEndpoint calls the project's own after-startup microflow. +// Written without the annotation it did not apply entity access, and mxbuild +// refuses a non-applying microflow that calls an applying one (CE0114): any +// project whose after-startup microflow applies entity access could not run a +// single test. Measured with mx check on 11.13.0, the rule has one direction — +// non-applying → applying is CE0114, applying → non-applying and applying → +// applying are 0 errors, under security Off and Production alike — so a +// RegisterEndpoint that applies entity access may call either kind. The setting +// governs only its own two activities: the Java action ignores it, and each +// chained microflow runs with its own. +func TestRegisterEndpointAppliesEntityAccess(t *testing.T) { + for _, chain := range []string{"", "MyModule.ASU_Startup"} { + prog, errs := visitor.Build(GenerateEndpointMDL(chain)) + if len(errs) > 0 { + t.Fatalf("chain %q: generated MDL does not parse: %v", chain, errs[0]) + } + var found *ast.CreateMicroflowStmt + for _, s := range prog.Statements { + if mf, ok := s.(*ast.CreateMicroflowStmt); ok && mf.Name.String() == endpointStartupFlow { + found = mf + } + } + if found == nil { + t.Fatalf("chain %q: no %s in the generated MDL", chain, endpointStartupFlow) + } + if found.ApplyEntityAccess == nil || !*found.ApplyEntityAccess { + t.Errorf("chain %q: %s does not apply entity access, so it cannot call an after-startup microflow that does (CE0114)", + chain, endpointStartupFlow) + } + } +} + +// TestBuildErrorInGeneratedStartupFlowIsMxclis pins the attribution half of +// ako/mxcli#1654. The CE0114 above was located in MxTest.RegisterEndpoint — a +// document mxcli generated for the run — and the error said "These errors are in +// the project, not in this run's tests", sending the reader through their own +// model for a defect in mxcli's. +func TestBuildErrorInGeneratedStartupFlowIsMxclis(t *testing.T) { + suite := suiteOf("test_1") + ce0114 := problem("CE0114", + "A microflow that does not apply entity access can only call microflows that also do not apply entity access.", + "MxTest", "Microflow 'RegisterEndpoint'", "Call microflow activity 'ASU_Startup'") + inProject := problem("CE0109", "Undefined variable 'x'.", "Sudoku", "Microflow 'SUB_Deal'", "End event") + + t.Run("only the generated flow", func(t *testing.T) { + _, err := resultsForBuildFailure(buildFailure(failedBuild(ce0114)), suite) + if err == nil { + t.Fatal("no error for a failed build") + } + msg := err.Error() + if strings.Contains(msg, "in the project") { + t.Errorf("an error in a document mxcli generated is blamed on the project:\n%s", msg) + } + for _, want := range []string{endpointStartupFlow, "generated", "mxcli", "--skip-app-startup"} { + if !strings.Contains(msg, want) { + t.Errorf("message does not say %q:\n%s", want, msg) + } + } + }) + + t.Run("the generated flow and the project", func(t *testing.T) { + _, err := resultsForBuildFailure(buildFailure(failedBuild(ce0114, inProject)), suite) + msg := err.Error() + if !strings.Contains(msg, endpointStartupFlow) || !strings.Contains(msg, "in the project") { + t.Errorf("want both attributions:\n%s", msg) + } + }) + + t.Run("control: only the project", func(t *testing.T) { + _, err := resultsForBuildFailure(buildFailure(failedBuild(inProject)), suite) + msg := err.Error() + if !strings.Contains(msg, "These errors are in the project, not in this run's tests.") { + t.Errorf("a project error is no longer attributed to the project:\n%s", msg) + } + if strings.Contains(msg, "generated") { + t.Errorf("a project error is attributed to mxcli:\n%s", msg) + } + }) +} diff --git a/docs-site/src/tools/running-tests.md b/docs-site/src/tools/running-tests.md index 0a11a18815..b457152ab7 100644 --- a/docs-site/src/tools/running-tests.md +++ b/docs-site/src/tools/running-tests.md @@ -356,6 +356,14 @@ This keeps a suite behaving the same under `--local` and `--attach`. Note that what the startup microflow writes is not covered by `@cleanup rollback` — it runs at boot, outside any test's transaction. +The generated startup flow, `MxTest.RegisterEndpoint`, **applies entity +access**, so it can call your after-startup microflow whichever way that one is +set: Mendix refuses a microflow that does not apply entity access calling one +that does (CE0114), and allows the other direction. Your microflow still runs +with its own setting. A build error located in `MxTest.RegisterEndpoint` or +`MxTest.RegisterTestEndpoint` is reported as mxcli's, not your project's — those +documents exist only for the run. + ### `@cleanup`: what happens to a test's data `rollback` is the **default**, so a test's database writes do not survive it. diff --git a/mdl-examples/bug-tests/test-runner-1654-setup.mdl b/mdl-examples/bug-tests/test-runner-1654-setup.mdl new file mode 100644 index 0000000000..0ff9d829e5 --- /dev/null +++ b/mdl-examples/bug-tests/test-runner-1654-setup.mdl @@ -0,0 +1,31 @@ +-- ako/mxcli#1654 (upstream mendixlabs/mxcli#1645): the setup script for +-- test-runner-1654-startup-applies-entity-access.test.mdl. Apply it before +-- running the tests: +-- +-- mxcli exec mdl-examples/bug-tests/test-runner-1654-setup.mdl -p app.mpr +-- +-- The project's after-startup microflow applies entity access, which is what +-- the generated MxTest.RegisterEndpoint (it calls this one) could not call. It +-- leaves a marker, so a test can show that it ran at boot. +mdl 1; +create module MyModule; + +create persistent entity MyModule.StartupMarker ( + Note: String(100) +); + +/** Startup flow that applies entity access. */ +@applyentityaccess(true) +create microflow MyModule.ASU_Startup () returns Boolean +begin + $Marker = create MyModule.StartupMarker (Note = 'startup ran') commit; + return true; +end; + +/** Adds two integers. */ +create microflow MyModule.SUB_Add ($A: Integer, $B: Integer) returns Integer +begin + return $A + $B; +end; + +alter settings runtime (AfterStartupMicroflow: 'MyModule.ASU_Startup'); diff --git a/mdl-examples/bug-tests/test-runner-1654-startup-applies-entity-access.test.mdl b/mdl-examples/bug-tests/test-runner-1654-startup-applies-entity-access.test.mdl new file mode 100644 index 0000000000..cc1fc3bf2d --- /dev/null +++ b/mdl-examples/bug-tests/test-runner-1654-startup-applies-entity-access.test.mdl @@ -0,0 +1,41 @@ +-- ako/mxcli#1654 (upstream mendixlabs/mxcli#1645): `mxcli test --local` +-- generates the startup microflow MxTest.RegisterEndpoint, which registers the +-- test endpoint and then calls the project's own after-startup microflow. It was +-- written without "apply entity access", and MxBuild refuses a microflow that +-- does not apply entity access calling one that does (CE0114) — so in a project +-- whose after-startup microflow applies entity access no test could run, and the +-- error said it was "in the project, not in this run's tests". +-- +-- Setup: test-runner-1654-setup.mdl. Then: +-- +-- mxcli test mdl-examples/bug-tests/test-runner-1654-startup-applies-entity-access.test.mdl -p app.mpr --local +-- +-- Measured on 11.13.0 (--local --db-type hsqldb). Before the fix: "build failed +-- … [CE0114] … at MxTest / Microflow 'RegisterEndpoint' / Call microflow +-- activity 'ASU_Startup'" and no test runs. After: both tests PASS — the second +-- one shows the project's startup microflow ran at boot, through the generated +-- flow. It consumes the marker it counts (`@cleanup none`), so it cannot pass on +-- an earlier boot's: its control, the same run with --skip-app-startup, FAILs +-- it ("expected $Count > 0, actual: 0"). +-- +-- CE0114 has one direction (mx check, 11.13.0): non-applying → applying is the +-- error; applying → non-applying and applying → applying are 0 errors, at +-- security Off and Production. So RegisterEndpoint now always applies entity +-- access, and calls a startup microflow of either kind. +mdl 1; +/** + * @test Addition returns the sum + * @expect $result = 4 + */ +$result = call microflow MyModule.SUB_Add(A = 2, B = 2); +/ + +/** + * @test the app's own after-startup microflow ran at boot + * @cleanup none + * @expect $Count > 0 + */ +retrieve $Markers from MyModule.StartupMarker; +$Count = count $Markers; +delete $Markers; +/