From a78f36ed86c843bb3c65b527977c7332f057f30b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 07:36:48 +0000 Subject: [PATCH] fix(test): attribute a failed build to the test that caused it An @expect that is syntactically valid but only rejected by MxBuild took down an entire `mxcli test --local` run: no test results at all, valid tests in the same file never executed, and the cause arrived as ~200 lines of mxbuild JSON with the real error among dozens of unrelated Atlas warnings. BuildResult parsed only status and message and left the rest of the response unread, though mxbuild returns every problem with a severity, an error code and a location. Measured on 11.13, a failing build returns 18 problems of which one is the error, so printing the body meant 11,580 bytes in which nothing marked the line that mattered. Filtering to errors renders it as: [CE0117] Error(s) in expression. -- at MxTest / Microflow 'Test_test_3' / Decision '$result = 3' The location's document names the generated test microflow, so it maps back to the test exactly: that test is reported ERROR with the consistency message and every other test as SKIP -- never PASS, because nothing ran. An error in the project rather than the suite is reported as such instead of being blamed on a test. The finding's other suggested remedy -- refusing an unbound variable at injection time rather than letting the build find it -- was implemented and then removed. It passed check-mdl's 465 scripts and the whole unit suite and was still wrong: `mxcli test` execs its microflows, and the microflow validator's scope model tracks variables where they are ASSIGNED. Reusing it to check READS refuses valid work. Two independent holes, both surfaced only by `make test-integration`: - $latestHttpResponse is a Mendix system variable that no MDL statement declares; it is populated after SEND REST REQUEST - a loop iterator is registered only when the list's type is known Both refuse a microflow `mx check` accepts at 0 errors. A variable model built for checking writes only has to know the names being bound; the read side has to know every name that can legally be in scope, including ones the platform supplies. That set cannot be enumerated confidently here, and each miss refuses a working microflow -- so the build stays the authority and this change makes its verdict legible instead. Nothing is lost by dropping it: CE0109 reaches the build and is attributed to its test by the same path as CE0117. Controls: an error in a generated test microflow produces per-test rows, an error in the user's own model produces none, and no test may be reported PASS after a failed build. The response shape is captured from a real failing build rather than inferred -- no fixture here had recorded one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JXnEgoc2NQP1Y2TWMCMXC4 --- .../skills/fix-issue/findings/cmd-mxcli.jsonl | 1 + CHANGELOG.md | 8 + cmd/mxcli/docker/localapp.go | 2 +- cmd/mxcli/docker/mxserve.go | 133 +++++++++++++++ cmd/mxcli/docker/mxserve_problems_test.go | 130 +++++++++++++++ cmd/mxcli/docker/runlocal.go | 2 +- cmd/mxcli/testrunner/build_attribution.go | 154 ++++++++++++++++++ .../testrunner/build_attribution_test.go | 140 ++++++++++++++++ cmd/mxcli/testrunner/runner_endpoint.go | 15 ++ 9 files changed, 583 insertions(+), 2 deletions(-) create mode 100644 cmd/mxcli/docker/mxserve_problems_test.go create mode 100644 cmd/mxcli/testrunner/build_attribution.go create mode 100644 cmd/mxcli/testrunner/build_attribution_test.go diff --git a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl index 28f9dc5415..24e2612f77 100644 --- a/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl +++ b/.claude/skills/fix-issue/findings/cmd-mxcli.jsonl @@ -97,3 +97,4 @@ {"area": "cmd/mxcli", "date": "2026-09-01", "symptom": "After `mxcli test --local`, a live `mxcli run --local` serving the SAME project starts answering **HTTP 200 with a zero-byte body** on every microflow-backed resource \u2014 not a 500, not an error page \u2014 while source-backed ones keep working, so half the app looks fine. The runtime log shows `java.lang.NoClassDefFoundError` on a project class. In a two-app solution it surfaces as tests failing in the OTHER app.", "cause": "The test run recompiles the project's Java into `deployment/run/bin`, which is the classpath the running JVM is holding open. Measured on a real 11.13 project: after one test run all 134 class files have **new inodes and byte-identical content** \u2014 every one deleted and rewritten. A JVM loads classes lazily, so one it has not reached yet can fail permanently. mxcli cannot prevent this: mxbuild's Gradle pass owns the compile and the deployment directory cannot be moved (ledger \u00a7150). So it warns instead, which is what was missing.", "file": "`cmd/mxcli/devloop_recompile_warning.go` (new \u2014 `warnIfDevLoopServing`, `recompileWarning`), `cmd/mxcli/cmd_test_run.go`; reads the existing `cmd/mxcli/devloop_handshake.go`", "insight": "**The mechanism already existed and a duplicate would have broken it.** `mxcli run --local` publishes `devLoopHandshake` at `.mxcli/run-local.json` for `mxcli constant set --apply` \u2014 same path, same pid-liveness staleness check, plus the `adminPass` and `bootConfig` that `--apply` and `--attach` depend on. A second state file was written at that path before this was noticed; it parsed fine (JSON ignores unknown fields) but its WRITER would have silently dropped those two keys. Grep the path before inventing a file. **The liveness check is the feature**: a `run --local` killed or ended by its development licence (\u00a760, measured lifetimes under six hours) leaves the file behind, and a warning driven by the file alone fires forever \u2014 one that is always wrong teaches the reader to skip it. It **warns rather than refuses**, since the warm loop exists so an app can stay up while you work and the reporting project runs two apps that way; neither `--attach` nor `--skip-build` builds, so neither warns. The finding's cost was diagnosis, not breakage \u2014 108 log lines and a wrong hypothesis about a different app, for something whose remedy is one restart \u2014 so the warning names the symptom (HTTP 200, empty body), the part nobody guesses. Controls, end-to-end against a real `run --local`: the warning carries that app's actual pid and port and its handshake still has adminPass and 9 bootConfig keys afterwards; with the loop stopped, and with a stale dead-pid handshake, the same command is silent. Reported as mxcli-formula1 FINDINGS \u00a781."} {"area": "cmd/mxcli", "date": "2026-09-03", "symptom": "`mxcli brain check` reports an entry as MISFILED even though the entry is correct and its anchor points at a real document — the anchor's target is simply of a document type the catalog's `objects` view does not index", "cause": "Misfiling was decided by comparing the shard against the modules of *resolved* anchors. An entry whose only anchor came back NotIndexable had an empty resolved-module list, so the comparison found no match and reported it misfiled — reintroducing, through the misfiling axis, exactly the false staleness that the NotIndexable state exists on the anchor axis to prevent", "file": "`cmd/mxcli/brain/entry.go` (`MisfiledIn`)", "insight": "When a check has two axes, an 'unknown' outcome on one of them must not be read as a negative on the other. The fix is to make misfiling *undecidable* rather than false when nothing resolved: with no resolved anchor there is no evidence about where the entry belongs, and an anchor that truly names nothing is already a failure on its own axis. Caught in development by a table test whose control stubbed the guard to `if false` — a control that deletes the block instead fails to compile on unused variables, which is not a control", "refs": ["ako/mxcli#385", "PROPOSAL_project_brain.md A1"]} {"area": "cmd/mxcli", "date": "2026-09-03", "symptom": "`mxcli brain check` exits 1 on a requirement that is simply not built yet — the entry is correct and current, and the check reports its anchor as NOT FOUND", "cause": "Requirements were recorded as ordinary brain entries, but an entry's anchor was assumed to point BACKWARD at something that exists. A decision's unresolved anchor means the decision is stale; a requirement's unresolved anchor means the work is not done. Same syntax, opposite meaning, and the store had no way to tell them apart", "file": "`cmd/mxcli/brain/entry.go` (`Kind`), `cmd/mxcli/brain/check.go` (`checkSlice`)", "insight": "Before adding a record type to an existing store, ask what a FAILED validation means for it — not just what it looks like. Requirements and decisions share the anchor syntax exactly, which is what made them look like the same thing; they differ only in the direction the anchor points, and that difference is the whole lifecycle. Measured before designing: one unbuilt requirement filed as a decision took `brain check` to exit 1, which settled it in one command. The inversion then pays for itself — a requirement is 'built' when its anchors resolve, so `brain plan` reports progress derived from the model (measured 0/1 -> 1/0 after creating the microflow, with the plan file untouched) instead of a status column that goes stale silently", "refs": ["ako/mxcli#385"]} +{"area": "cmd/mxcli", "cause": "BuildResult parsed only status/restartRequired/message and left everything else in Raw, so a failed build was reported by dumping the whole response body; nothing looked at the per-problem severity/errorCode/locations mxbuild actually returns. The consistency error that stopped the build therefore arrived unmarked among the warnings, and no test was named.", "ce": ["CE0109", "CE0117"], "date": "2026-09-03", "file": "cmd/mxcli/docker/mxserve.go (BuildProblem/BuildLocation/Errors/ErrorSummary/BuildFailedError), cmd/mxcli/testrunner/build_attribution.go (new), wired at cmd/mxcli/testrunner/runner_endpoint.go", "insight": "**The serve /build response already carries everything needed to attribute a build failure, and nothing was reading it.** Measured on 11.13: `problems` is an OBJECT whose inner `problems` list holds each consistency message with severity, errorCode and locations[] {module, document, element} \u2014 the document being `Microflow 'Test_test_3'` WITHOUT its module. So an error in a generated test microflow maps back exactly. The ratio is the point: a failing blank app returns 18 problems of which 1 is the error, and printing the body meant 11,580 bytes in which nothing marked the line that mattered; filtering severity==Error renders it as one line naming the test AND the decision. **Do not guess a response shape \u2014 POST to the serve API and look.** `mxbuild --serve --host=127.0.0.1 --port=N` plus a curl to /build is the whole harness, and no fixture in the repo had ever recorded a FAILING build.\n\n**The abandoned half is the more useful lesson.** Catching these earlier \u2014 refusing an unbound variable in an IF condition at injection time \u2014 was built, passed 465 check-mdl scripts and the whole unit suite, and was WRONG. `mxcli test` execs its microflows, and the microflow validator's scope model tracks variables where they are ASSIGNED; reusing it to check READS refuses valid work. Two independent holes, both found only by `make test-integration`: `$latestHttpResponse` is a Mendix system variable that no MDL statement declares, and a loop iterator is registered only when the list's type is known (`if listType, ok := fb.varTypes[...]`). Both refuse a microflow `mx check` accepts at 0 errors. **Generalisable: a variable model built for checking writes is not a variable model for checking reads** \u2014 the write side only has to know the names being bound, the read side has to know every name that can legally be in scope, including ones the platform supplies. Before reusing any scope model in the opposite direction, enumerate what populates it and assume the list is incomplete.\n\n**Process: `make check-mdl` is NOT the over-reach guard for an exec-path change.** It runs `mxcli check` with no project, so it exercises syntax only; a new refusal on the exec path sails through all 465 scripts. `make test-integration` (what CI runs, and runnable locally with mxbuild cached) execs every doctype script against a real project and runs `mx check` on the result \u2014 that is the guard, and skipping it cost a red CI. Also note the exec/check validator split (#833): a validator fix wired only into ValidateMicroflowBody looks correct under `check -p` and does nothing under `exec`.", "refs": ["ako/mxcli-sudoku FINDINGS #46 follow-up"], "symptom": "`mxcli test --local`: an @expect that is syntactically valid but only fails inside mxbuild takes down the ENTIRE run \u2014 `Error: local runtime: build failed: The project cannot be deployed, because it contains errors.` No test results at all, valid tests in the same file never run, and the cause arrives as ~200 lines of mxbuild JSON in which the real error sits among dozens of unrelated Atlas warnings."} diff --git a/CHANGELOG.md b/CHANGELOG.md index 087a6c3dbd..0e7c97413d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +- **A failed build now says which test caused it** (ako/mxcli-sudoku FINDINGS #46 follow-up) — an `@expect` that is syntactically valid but only rejected by MxBuild took down an entire `mxcli test --local` run: no test results at all, valid tests in the same file never executed, and the cause arrived as ~200 lines of mxbuild JSON with the real error among dozens of unrelated Atlas warnings. + + `BuildResult` parsed only the status and message and left the rest of the response unread, though mxbuild returns every problem with a severity, an error code and a location. Measured on 11.13, a failing build returns **18 problems of which one is the error**, so printing the body meant 11,580 bytes in which nothing marked the line that mattered. Filtering to errors renders it as `[CE0117] Error(s) in expression. — at MxTest / Microflow 'Test_test_3' / Decision '$result = 3'`. + + The location's document names the generated test microflow, so it maps back to the test exactly: that test is reported `ERROR` with the consistency message, every other test as `SKIP` — never `PASS`, because nothing ran. An error in the project rather than the suite is reported as such instead of being blamed on a test. + + Catching these earlier — refusing an unbound variable at injection time rather than letting the build find it — was built and then **removed**. The microflow validator tracks variables where they are *assigned*, and reusing that model to check *reads* refuses valid microflows: `$latestHttpResponse` is a Mendix system variable no MDL statement declares, and a loop iterator is only registered when the list's type is known. Both are accepted by `mx check` at 0 errors. The scope model is right for the bar it was built for and wrong for this one, so the build stays the authority. + - **`CATALOG.strings` indexes every translatable string, not five hand-picked kinds** — `SHOW LANGUAGES` listed 8 of a project's 9 languages and `search` could not find a widget caption that `DESCRIBE TRANSLATIONS` had just listed. The index was filled by per-type extractors reaching five sites (page title, enum caption, three microflow message templates), so a text anywhere else was never indexed: measured on a stock 11.13 app, **69 of 3265 texts and 8 of 9 languages**. A language present only on an unindexed site is *invisible* rather than undercounted, which also blinded lint rule QUAL005 — it discovers its language set from the same table. The rows now come from the type-agnostic `Texts$Text` walk that `DESCRIBE TRANSLATIONS` already uses, so the two subsystems cannot disagree about what the project contains; the typed path keeps only the strings that are *not* translatable (URLs, log node names, REST paths, documentation, and the `Microflows$StringTemplate` a workflow name is stored in). `StringContext` now names the site — `Forms$ActionButton.Caption` rather than `page_title` — and `ObjectType` is derived from the unit `$Type` mechanically, so a document type Mendix adds later is named correctly with nobody maintaining a list. Same project after: 1496 rows, 9 languages, counts identical to an independent BSON walk. Atlas design templates are ~70% of the corpus and are indexed rather than dropped, because `CREATE TRANSLATIONS` writes them and a `SHOW LANGUAGES` that excluded them would reopen the same split; `ObjectType` is how a consumer filters them. diff --git a/cmd/mxcli/docker/localapp.go b/cmd/mxcli/docker/localapp.go index a44b60072b..f951f26115 100644 --- a/cmd/mxcli/docker/localapp.go +++ b/cmd/mxcli/docker/localapp.go @@ -212,7 +212,7 @@ func StartLocalApp(opts LocalAppOptions) (*LocalApp, error) { } if !build.OK() { app.Stop() - return nil, fmt.Errorf("build failed: %s\n%s", build.Message, string(build.Raw)) + return nil, &BuildFailedError{Result: build} } } diff --git a/cmd/mxcli/docker/mxserve.go b/cmd/mxcli/docker/mxserve.go index 951b544c81..a48d69a2cf 100644 --- a/cmd/mxcli/docker/mxserve.go +++ b/cmd/mxcli/docker/mxserve.go @@ -11,6 +11,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "sync" "syscall" "time" @@ -69,12 +70,98 @@ type BuildResult struct { Status string `json:"status"` RestartRequired bool `json:"restartRequired"` Message string `json:"message"` + Problems BuildProblems `json:"problems"` Raw json.RawMessage `json:"-"` } +// BuildProblems is the serve response's problems object. The consistency errors +// are in the inner list; the outer one carries only the summary. +type BuildProblems struct { + Problems []BuildProblem `json:"problems"` +} + +// BuildProblem is one consistency message from a build. +// +// Only the fields anything reads are declared. Severity is the load-bearing one: +// a failing build of a blank 11.13 app returns 18 problems of which 16 are +// warnings and one a deprecation, so printing the response wholesale buries the +// single error that actually stopped the build. +type BuildProblem struct { + Severity string `json:"severity"` // "Error", "Warning", "Deprecation" + Message string `json:"message"` + ErrorCode string `json:"errorCode"` // e.g. "CE0109" + Locations []BuildLocation `json:"locations"` +} + +// BuildLocation is where a problem was found. Document is the human name Studio +// Pro would show — `Microflow 'Test_test_3'` — which is what lets a caller +// attribute an error to the document it generated. +type BuildLocation struct { + Module string `json:"module"` + Document string `json:"document"` + Element string `json:"element"` +} + // OK reports whether the build succeeded. func (r *BuildResult) OK() bool { return r.Status == "Success" } +// Errors returns only the problems that failed the build. +func (r *BuildResult) Errors() []BuildProblem { + var out []BuildProblem + for _, p := range r.Problems.Problems { + if strings.EqualFold(p.Severity, "Error") { + out = append(out, p) + } + } + return out +} + +// ErrorSummary renders the build errors one per line, with the code and the +// document each was found in. +// +// This is what a caller should print instead of the raw response body: the body +// is ~200 lines of JSON in which the real error sits among unrelated Atlas +// warnings, and a reader has no way to tell which line failed the build. +// Returns "" when the response carried no structured errors, so a caller can +// fall back rather than print nothing. +func (r *BuildResult) ErrorSummary() string { + errs := r.Errors() + if len(errs) == 0 { + return "" + } + var b strings.Builder + for i, p := range errs { + if i > 0 { + b.WriteString("\n") + } + b.WriteString(" ") + if p.ErrorCode != "" { + b.WriteString("[" + p.ErrorCode + "] ") + } + b.WriteString(p.Message) + if loc := p.Where(); loc != "" { + b.WriteString(" — at " + loc) + } + } + return b.String() +} + +// Where renders a problem's first location as `Module / Document / Element`, +// skipping the parts the response left empty. +func (p BuildProblem) Where() string { + if len(p.Locations) == 0 { + return "" + } + l := p.Locations[0] + parts := make([]string, 0, 3) + for _, s := range []string{l.Module, l.Document, l.Element} { + if s != "" { + parts = append(parts, s) + } + } + return strings.Join(parts, " / ") +} + // ServeServer wraps a long-lived `mxbuild --serve` process and its build API. type ServeServer struct { Host string @@ -284,3 +371,49 @@ func (w *syncBuffer) String() string { defer w.mu.Unlock() return w.b.String() } + +// buildFailureDetail renders what to show a user after a failed build. +// +// The structured error list when the response carried one, and the raw body only +// as a fallback. Printing the body was the previous behaviour everywhere, and it +// is close to useless: on a blank 11.13 app a single consistency error arrives +// alongside 16 Atlas warnings in ~200 lines of JSON, with nothing marking which +// one stopped the build. +func buildFailureDetail(r *BuildResult) string { + if r == nil { + return "" + } + if summary := r.ErrorSummary(); summary != "" { + return summary + } + return string(r.Raw) +} + +// BuildFailedError is returned when mxbuild rejected the model. +// +// It carries the parsed result so a caller can do something better than print +// the message: `mxcli test` maps each error back to the generated test microflow +// it was found in, which is the difference between "the build failed" and +// "test 3's assertion does not compile". +type BuildFailedError struct { + Result *BuildResult +} + +func (e *BuildFailedError) Error() string { + msg := "build failed" + if e.Result != nil && e.Result.Message != "" { + msg += ": " + e.Result.Message + } + if detail := buildFailureDetail(e.Result); detail != "" { + msg += "\n" + detail + } + return msg +} + +// BuildErrors returns the consistency errors that failed the build. +func (e *BuildFailedError) BuildErrors() []BuildProblem { + if e == nil || e.Result == nil { + return nil + } + return e.Result.Errors() +} diff --git a/cmd/mxcli/docker/mxserve_problems_test.go b/cmd/mxcli/docker/mxserve_problems_test.go new file mode 100644 index 0000000000..824786a91d --- /dev/null +++ b/cmd/mxcli/docker/mxserve_problems_test.go @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: Apache-2.0 + +package docker + +import ( + "encoding/json" + "strings" + "testing" +) + +// serveFailureBody is the shape mxbuild 11.13's serve /build returns when the +// model does not deploy, trimmed to the fields that matter. +// +// Captured from a real response rather than written from the docs: nothing in +// this repo had ever parsed a failing build, and the nesting is easy to get +// wrong — `problems` is an OBJECT with its own `problems` list inside, and the +// outer `errors` list carries only the summary sentence, not the consistency +// errors. The ratio is real too: a blank app returns 16 warnings and a +// deprecation alongside the single error that stopped the build. +const serveFailureBody = `{ + "status": "Failure", + "message": "The project cannot be deployed, because it contains errors.", + "problems": { + "errors": [{"message": "The project cannot be deployed, because it contains errors.", "details": ""}], + "problems": [ + { + "severity": "Warning", + "message": "No 'On click' action specified.", + "errorCode": "CW0055", + "locations": [{"element": "Menu item", "document": "Menu 'Tablet_Menu'", "module": "Atlas_Core"}] + }, + { + "severity": "Deprecation", + "message": "Something is deprecated.", + "errorCode": "CD0001", + "locations": [] + }, + { + "severity": "Error", + "message": "Undefined variable 'nosuchvar'.", + "errorCode": "CE0109", + "locations": [{"element": "End event", "document": "Microflow 'Test_test_3'", "module": "MxTest"}] + } + ] + } +}` + +func TestBuildResultErrors(t *testing.T) { + var r BuildResult + if err := json.Unmarshal([]byte(serveFailureBody), &r); err != nil { + t.Fatalf("decode: %v", err) + } + if r.OK() { + t.Fatal("a Failure status must not read as OK") + } + + // The filter is the whole point: 3 problems in, 1 error out. + errs := r.Errors() + if len(errs) != 1 { + t.Fatalf("Errors() = %d, want 1 (warnings and deprecations must not be reported as errors)", len(errs)) + } + if errs[0].ErrorCode != "CE0109" { + t.Errorf("errorCode = %q, want CE0109", errs[0].ErrorCode) + } + if got := errs[0].Where(); got != "MxTest / Microflow 'Test_test_3' / End event" { + t.Errorf("Where() = %q", got) + } +} + +func TestBuildResultErrorSummary(t *testing.T) { + var r BuildResult + if err := json.Unmarshal([]byte(serveFailureBody), &r); err != nil { + t.Fatalf("decode: %v", err) + } + + summary := r.ErrorSummary() + for _, want := range []string{"CE0109", "Undefined variable 'nosuchvar'", "Microflow 'Test_test_3'"} { + if !strings.Contains(summary, want) { + t.Errorf("summary %q does not carry %q", summary, want) + } + } + // The noise the summary exists to drop. + for _, unwanted := range []string{"CW0055", "Tablet_Menu", "Deprecation"} { + if strings.Contains(summary, unwanted) { + t.Errorf("summary should not carry the warning %q", unwanted) + } + } +} + +// TestBuildFailureDetailFallsBackToRaw guards the case that keeps this safe on a +// future mxbuild: if the response carries no structured errors, the caller must +// still see the body rather than an empty message. +func TestBuildFailureDetailFallsBackToRaw(t *testing.T) { + r := &BuildResult{Status: "Failure", Message: "nope", Raw: json.RawMessage(`{"status":"Failure"}`)} + if got := buildFailureDetail(r); !strings.Contains(got, `"status":"Failure"`) { + t.Errorf("detail = %q, want the raw body as a fallback", got) + } + + // And with structured errors it must prefer them. + var parsed BuildResult + if err := json.Unmarshal([]byte(serveFailureBody), &parsed); err != nil { + t.Fatalf("decode: %v", err) + } + parsed.Raw = json.RawMessage(serveFailureBody) + got := buildFailureDetail(&parsed) + if strings.Contains(got, "CW0055") { + t.Errorf("detail should be the filtered summary, not the raw body: %q", got) + } + if !strings.Contains(got, "CE0109") { + t.Errorf("detail %q lost the error", got) + } +} + +// TestBuildFailedErrorMessage covers what a user sees when a build fails. +func TestBuildFailedErrorMessage(t *testing.T) { + var r BuildResult + if err := json.Unmarshal([]byte(serveFailureBody), &r); err != nil { + t.Fatalf("decode: %v", err) + } + err := &BuildFailedError{Result: &r} + msg := err.Error() + for _, want := range []string{"build failed", "cannot be deployed", "CE0109", "Test_test_3"} { + if !strings.Contains(msg, want) { + t.Errorf("message %q does not carry %q", msg, want) + } + } + if len(err.BuildErrors()) != 1 { + t.Errorf("BuildErrors() = %d, want 1", len(err.BuildErrors())) + } +} diff --git a/cmd/mxcli/docker/runlocal.go b/cmd/mxcli/docker/runlocal.go index 81741c5aeb..73defaed7a 100644 --- a/cmd/mxcli/docker/runlocal.go +++ b/cmd/mxcli/docker/runlocal.go @@ -645,7 +645,7 @@ func RunLocal(opts LocalRunOptions) error { return fmt.Errorf("initial build: %w", err) } if !build.OK() { - return fmt.Errorf("initial build failed: %s\n%s", build.Message, string(build.Raw)) + return fmt.Errorf("initial build failed: %s\n%s", build.Message, buildFailureDetail(build)) } // 5b. Bundle the browser client (web/dist). The serve Deploy target writes the diff --git a/cmd/mxcli/testrunner/build_attribution.go b/cmd/mxcli/testrunner/build_attribution.go new file mode 100644 index 0000000000..b67523e37a --- /dev/null +++ b/cmd/mxcli/testrunner/build_attribution.go @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: Apache-2.0 + +// Attributing a failed build back to the test that caused it. +// +// An @expect the runner cannot compile is already an ERROR at parse time. What +// is left is everything only MxBuild decides — an undefined variable (CE0109), a +// String compared to a number (CE0117) — where nothing upstream of the build has +// the information to object. +// +// Checking those earlier was tried and abandoned. The microflow validator tracks +// variables where they are ASSIGNED, and reusing that model to check READS +// produces false refusals: `$latestHttpResponse` is a Mendix system variable that +// no MDL statement declares, and a loop iterator is only registered when the +// list's type is known. Both refuse a microflow `mx check` accepts at 0 errors. +// The scope model is right for the bar it was built for and wrong for this one, +// so the build stays the authority and this file makes its verdict legible. +// +// When that happens the build fails, the runtime never boots, and the run +// produces no test results at all. The failure is real; what was missing is +// saying which test caused it. MxBuild locates every problem +// (`module` + `document`), and the test microflows are generated with names this +// package chose, so the mapping back is exact rather than a guess. +// +// ako/mxcli-sudoku FINDINGS #46, follow-up. +package testrunner + +import ( + "fmt" + "regexp" + "strings" + + "github.com/mendixlabs/mxcli/cmd/mxcli/docker" +) + +// testFlowDocumentPattern matches the `document` MxBuild reports for a generated +// test microflow. +// +// MxBuild names the document WITHOUT its module — `Microflow 'Test_test_3'`, +// with the module carried separately in the same location — so this matches the +// bare name and the module is checked alongside it. Anchored on the generated +// prefix, so an error in the user's own microflow is never blamed on a test. +var testFlowDocumentPattern = regexp.MustCompile(`^Microflow '(Test_[^']*)'$`) + +// attributeBuildProblems splits a failed build's errors into those belonging to +// a generated test microflow and those that do not. +// +// The second group matters as much as the first: an error in the user's own +// model also fails the build, and reporting it as a test failure would send them +// looking in the wrong place. +func attributeBuildProblems(problems []docker.BuildProblem, suite *TestSuite) (map[string][]docker.BuildProblem, []docker.BuildProblem) { + byTest := map[string][]docker.BuildProblem{} + var other []docker.BuildProblem + + // Keyed on the BARE document name MxBuild reports, not the qualified one. + known := map[string]string{} // "Test_test_3" -> test ID + if suite != nil { + for _, tc := range suite.Tests { + known[strings.TrimPrefix(testFlowName(tc), mxTestModule+".")] = tc.ID + } + } + + for _, p := range problems { + id := "" + for _, loc := range p.Locations { + if !strings.EqualFold(loc.Module, mxTestModule) { + continue + } + m := testFlowDocumentPattern.FindStringSubmatch(loc.Document) + if m == nil { + continue + } + if tid, ok := known[m[1]]; ok { + id = tid + break + } + } + if id == "" { + other = append(other, p) + continue + } + byTest[id] = append(byTest[id], p) + } + return byTest, other +} + +// buildProblemMessage renders one test's build errors for its result row. +func buildProblemMessage(problems []docker.BuildProblem) string { + parts := make([]string, 0, len(problems)) + for _, p := range problems { + msg := p.Message + if p.ErrorCode != "" { + msg = p.ErrorCode + ": " + msg + } + if p.Locations != nil && p.Where() != "" { + // Only the element is worth repeating here — the module and document + // are the test itself, which the row already names. + if el := p.Locations[0].Element; el != "" { + msg += " (at " + el + ")" + } + } + parts = append(parts, msg) + } + return "the assertion could not be built: " + strings.Join(parts, "; ") +} + +// resultsFromFailedBuild turns a failed build into one result per test, so a run +// that cannot boot still reports something per test instead of nothing at all. +// +// A test MxBuild blamed becomes an ERROR carrying the consistency message. Every +// other test becomes a SKIP: they were not run and must not be counted as +// passing — the whole point of this area is that a test framework never reports +// green for something it did not evaluate. +// +// Returns nil when no error could be attributed to a test, so the caller reports +// the build failure as it always did rather than inventing per-test rows for a +// problem in the user's own model. +func resultsFromFailedBuild(problems []docker.BuildProblem, suite *TestSuite) []TestResult { + byTest, _ := attributeBuildProblems(problems, suite) + if len(byTest) == 0 || suite == nil { + return nil + } + + results := make([]TestResult, 0, len(suite.Tests)) + for _, tc := range suite.Tests { + r := newResult(tc) + if ps, ok := byTest[tc.ID]; ok { + r.Status = StatusError + r.Message = buildProblemMessage(ps) + } else { + r.Status = StatusSkip + r.Message = "not run: another test in this run failed to build" + } + results = append(results, r) + } + return results +} + +// 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. +func buildFailureHint(other []docker.BuildProblem) string { + if len(other) == 0 { + return "" + } + var b strings.Builder + b.WriteString("\n The build errors are in the project, not in the tests:") + for _, p := range other { + b.WriteString(fmt.Sprintf("\n %s %s", p.ErrorCode, p.Message)) + if w := p.Where(); w != "" { + b.WriteString(" — at " + w) + } + } + return b.String() +} diff --git a/cmd/mxcli/testrunner/build_attribution_test.go b/cmd/mxcli/testrunner/build_attribution_test.go new file mode 100644 index 0000000000..ed626abfbc --- /dev/null +++ b/cmd/mxcli/testrunner/build_attribution_test.go @@ -0,0 +1,140 @@ +// SPDX-License-Identifier: Apache-2.0 + +package testrunner + +import ( + "strings" + "testing" + + "github.com/mendixlabs/mxcli/cmd/mxcli/docker" +) + +// problem builds one MxBuild problem located in a document. +func problem(code, msg, module, document, element string) docker.BuildProblem { + return docker.BuildProblem{ + Severity: "Error", + ErrorCode: code, + Message: msg, + Locations: []docker.BuildLocation{{Module: module, Document: document, Element: element}}, + } +} + +func suiteOf(ids ...string) *TestSuite { + s := &TestSuite{Name: "suite"} + for _, id := range ids { + s.Tests = append(s.Tests, TestCase{ID: id, Name: "name of " + id}) + } + return s +} + +// TestAttributeBuildProblems covers the mapping from an MxBuild location back to +// the test whose generated microflow it names. +// +// The shape is measured, not assumed: on Mendix 11.13 a serve build reports the +// document WITHOUT its module — `Microflow 'Test_test_3'` — with the module in +// the same location object. +func TestAttributeBuildProblems(t *testing.T) { + suite := suiteOf("test_1", "test_3") + + t.Run("an error in a generated test microflow is attributed to it", func(t *testing.T) { + p := problem("CE0117", "Error(s) in expression.", "MxTest", "Microflow 'Test_test_3'", "End event") + byTest, other := attributeBuildProblems([]docker.BuildProblem{p}, suite) + if len(other) != 0 { + t.Fatalf("unattributed: %v", other) + } + if got := byTest["test_3"]; len(got) != 1 { + t.Fatalf("byTest[test_3] = %v, want 1 problem", got) + } + }) + + t.Run("an error in the user's own model is not blamed on a test", func(t *testing.T) { + // The whole point of the split: reporting this as a test failure sends + // the reader to the wrong file. + p := problem("CE0109", "Undefined variable 'x'.", "Sudoku", "Microflow 'SUB_Deal'", "End event") + byTest, other := attributeBuildProblems([]docker.BuildProblem{p}, suite) + if len(byTest) != 0 { + t.Errorf("must not attribute a project error to a test: %v", byTest) + } + if len(other) != 1 { + t.Errorf("other = %v, want the problem", other) + } + }) + + t.Run("a microflow in MxTest that is not a generated test is not attributed", func(t *testing.T) { + // MxTest also holds the endpoint registration flow. + p := problem("CE0109", "Undefined variable 'x'.", "MxTest", "Microflow 'RegisterEndpoint'", "End event") + byTest, other := attributeBuildProblems([]docker.BuildProblem{p}, suite) + if len(byTest) != 0 || len(other) != 1 { + t.Errorf("byTest=%v other=%v — a non-test MxTest document must stay unattributed", byTest, other) + } + }) + + t.Run("a generated name from a different run is not attributed", func(t *testing.T) { + // test_9 is not in this suite; claiming it would invent a result. + p := problem("CE0117", "Error(s) in expression.", "MxTest", "Microflow 'Test_test_9'", "End event") + byTest, other := attributeBuildProblems([]docker.BuildProblem{p}, suite) + if len(byTest) != 0 || len(other) != 1 { + t.Errorf("byTest=%v other=%v — an unknown test id must stay unattributed", byTest, other) + } + }) +} + +// TestResultsFromFailedBuild covers what a run reports when the build fails: the +// blamed test is an ERROR and every other test is a SKIP. +// +// No test may be reported as passing. A build that never produced a runtime +// evaluated nothing, and a suite that reports green for something it did not +// evaluate is the defect this whole area exists to prevent. +func TestResultsFromFailedBuild(t *testing.T) { + suite := suiteOf("test_1", "test_2", "test_3") + p := problem("CE0117", "Error(s) in expression.", "MxTest", "Microflow 'Test_test_2'", "End event") + + results := resultsFromFailedBuild([]docker.BuildProblem{p}, suite) + if len(results) != 3 { + t.Fatalf("got %d results, want one per test", len(results)) + } + + byID := map[string]TestResult{} + for _, r := range results { + byID[r.ID] = r + if r.Status == StatusPass { + t.Errorf("%s reported PASS after a failed build — nothing ran", r.ID) + } + } + + if got := byID["test_2"]; got.Status != StatusError { + t.Errorf("test_2 status = %v, want ERROR", got.Status) + } else { + for _, want := range []string{"CE0117", "Error(s) in expression"} { + if !strings.Contains(got.Message, want) { + t.Errorf("test_2 message %q does not carry %q", got.Message, want) + } + } + } + for _, id := range []string{"test_1", "test_3"} { + if got := byID[id]; got.Status != StatusSkip { + t.Errorf("%s status = %v, want SKIP", id, got.Status) + } + } +} + +// TestResultsFromFailedBuildDeclinesUnattributableErrors is the control on the +// other side: when the failure is in the user's model, the runner must NOT +// manufacture per-test rows. Without this, every build error in the project +// would be reported as a suite of skipped tests and the real cause would vanish. +func TestResultsFromFailedBuildDeclinesUnattributableErrors(t *testing.T) { + suite := suiteOf("test_1") + p := problem("CE0109", "Undefined variable 'x'.", "Sudoku", "Microflow 'SUB_Deal'", "End event") + + if results := resultsFromFailedBuild([]docker.BuildProblem{p}, suite); results != nil { + t.Fatalf("expected no results for a project-level failure, got %v", results) + } + + _, other := attributeBuildProblems([]docker.BuildProblem{p}, suite) + hint := buildFailureHint(other) + for _, want := range []string{"in the project, not in the tests", "CE0109", "SUB_Deal"} { + if !strings.Contains(hint, want) { + t.Errorf("hint %q does not mention %q", hint, want) + } + } +} diff --git a/cmd/mxcli/testrunner/runner_endpoint.go b/cmd/mxcli/testrunner/runner_endpoint.go index d94e90d75a..f0d03069a5 100644 --- a/cmd/mxcli/testrunner/runner_endpoint.go +++ b/cmd/mxcli/testrunner/runner_endpoint.go @@ -3,6 +3,7 @@ package testrunner import ( + "errors" "fmt" "io" "os" @@ -99,6 +100,20 @@ func (s *testAppSession) applyModelChange(projectPath string) (string, error) { func runViaEndpoint(opts RunOptions, suite *TestSuite, token string, timeout time.Duration, w io.Writer) (*SuiteResult, error) { sess, err := bootForTests(opts, token, timeout, w) if err != nil { + // A build MxBuild rejected because of a generated test microflow is that + // test's problem, not the run's. Reporting it as an ERROR row — and the + // rest as SKIP — says which assertion broke, where the bare failure said + // only that the project would not deploy (FINDINGS #46 follow-up). + var bf *docker.BuildFailedError + if errors.As(err, &bf) { + if results := resultsFromFailedBuild(bf.BuildErrors(), suite); results != nil { + return &SuiteResult{Name: suite.Name, Tests: results, Started: time.Now()}, nil + } + // Not the tests' doing: the model itself does not build. Say so + // rather than letting the reader assume a test is at fault. + _, other := attributeBuildProblems(bf.BuildErrors(), suite) + return nil, fmt.Errorf("%w%s", err, buildFailureHint(other)) + } return nil, err } defer sess.stop()