diff --git a/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.md b/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.md index 6601f94..f92a783 100644 --- a/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.md +++ b/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.md @@ -11,7 +11,7 @@ application-area: [all] ## Description -`Confirm`'s placeholder-substitution overload — `Confirm('text %1', false, Value)` — substitutes the placeholder only for the dialog a real user sees. Inside a `[ConfirmHandler]`, the `Question` parameter received is the literal, unsubstituted template string (`'text %1'`), not the value-filled text. A test that asserts `Question` against the expected substituted message either fails outright or silently checks the wrong thing. +`Confirm`'s placeholder-substitution overload — `Confirm('text %1', false, Value)` — substitutes the placeholder only for the dialog a real user sees. Inside a `[ConfirmHandler]`, the `Question` parameter received is the literal, unsubstituted template string (`'text %1'`), not the value-filled text. This is a reported platform defect (microsoft/ALAppExtensions#23935), not documented or intended behavior — treat it as a known, unconfirmed-fix issue rather than a permanent platform rule; if it is ever fixed, this workaround becomes unnecessary rather than wrong. Notably, `Message`/`[MessageHandler]` substitutes correctly — only `Confirm` is affected, which is itself evidence this is a bug rather than a deliberate design choice. A test that asserts `Question` against the expected substituted message either fails outright or silently checks the wrong thing. ## Best Practice @@ -24,3 +24,7 @@ See sample: `confirm-needs-strsubstno-before-confirmhandler-sees-substituted-tex Calling `Confirm('text %1', false, Value)` and then asserting the substituted text against `Question` inside a `[ConfirmHandler]`. `Question` holds the raw `'text %1'` template, so the assertion never matches the intended message. See sample: `confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.bad.al`. + +## Source + +Reported by Luc van Vugt: https://github.com/microsoft/ALAppExtensions/issues/23935 — an internal Microsoft bug was filed from that report; the issue's fix status is not confirmed as of this writing. diff --git a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.md b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.md index 383ebe4..a3af2be 100644 --- a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.md +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.md @@ -11,7 +11,7 @@ application-area: [all] ## Description -Codeunit 134926 "Table Relation Test" walks every `TableRelation` field property in the app and fails the moment a related field's type or length doesn't match what the relation requires — the related field must match the largest related field's length, and its type must match (except a field may relate to both `Code` and `Text`, which resolves to `Text`). A field with a legitimate, intentional relation shape has no per-field override in its own object definition; the check runs across the whole app with no built-in escape hatch. +Codeunit 134926 "Table Relation Test" (shipped in BCApps' test app — only consumers that depend on the BC test libraries can subscribe to it) reads Table Relations Metadata tenant-wide across every installed app, not just the current one, and fails the moment a related field's type or length doesn't match what the relation requires — the related field must match the largest related field's length, and its type must match (except a field may relate to both `Code` and `Text`, which resolves to `Text`). A field with a legitimate, intentional relation shape has no per-field override in its own object definition; the check runs with no built-in escape hatch. ## Best Practice diff --git a/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md b/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md index c9c159a..ae24f1f 100644 --- a/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md +++ b/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md @@ -11,16 +11,16 @@ application-area: [all] ## Description -`[TransactionModel(...)]` declares how a test method interacts with the database's write transaction. The attribute applies only to methods inside a codeunit with `SubType = Test` and takes one of three values: `AutoRollback`, `AutoCommit`, or `None`. The choice must match the code being exercised — in particular, whether that code calls `Commit()`. Per the platform reference, "if the code that you test includes calls to the COMMIT Method, then set the TransactionModel property on the test method to AutoCommit." Applying `AutoRollback` to a test that drives code which calls `Commit` produces a runtime error on the first Commit, not a meaningful assertion failure — the test does not complete, and the reviewer sees an infrastructure error instead of a business-logic verdict. +`[TransactionModel(...)]` declares how a test method interacts with the database's write transaction. The attribute applies only to methods inside a codeunit with `SubType = Test` and takes one of three values: `AutoRollback`, `AutoCommit`, or `None`. The "a call to `Commit` produces a runtime error" behavior is specific to the *explicitly declared* `AutoRollback` attribute — it is not what an undeclared/default test method does. BCApps' own canonical pattern for a lazily-initialized shared fixture (see `codeunit 134915 "ERM Online Mapping Setup"`) declares no `TransactionModel` attribute at all, calls `Commit()` inside its `Initialize()` helper, and cleans up manually with a deliberate `asserterror Error(...)` at the end rather than relying on automatic rollback — this is a legitimate, common pattern, not a bug. When a test method *does* declare `AutoRollback` explicitly, the choice must match the code being exercised: per the platform reference, "if the code that you test includes calls to the COMMIT Method, then set the TransactionModel property on the test method to AutoCommit." Applying `AutoRollback` to a test that drives code which calls `Commit` produces a runtime error on the first Commit, not a meaningful assertion failure. ## Best Practice -Default to `AutoRollback`: it opens a write transaction at the start of the test, runs the test body, and rolls back at the end, leaving the database in its original state. Pick `AutoCommit` only when the code under test genuinely calls `Commit` — posting routines, job-queue handlers, integration flows — and make the test exercise that commit path. Pair the test codeunit with a `TestIsolation`-enabled test runner so committed changes are reverted at a higher scope. Pick `None` only for read-only tests or tests that drive UI code without writing from the test method itself. +When declaring `[TransactionModel(...)]` explicitly, pick `AutoRollback` for a test whose own logic and the code it exercises make no `Commit` call, `AutoCommit` when the code under test genuinely calls `Commit` — posting routines, job-queue handlers, integration flows — and make the test exercise that commit path, and `None` for a read-only test or one that drives UI code without writing from the test method itself. Pair `AutoCommit` with a `TestIsolation`-enabled test runner so committed changes are reverted at a higher scope. A lazily-initialized shared fixture that commits once and relies on a manual `asserterror`-based cleanup, with no `TransactionModel` attribute declared at all, is a distinct and equally valid pattern — do not treat the absence of the attribute as equivalent to declaring `AutoRollback`. See sample: [`transactionmodel-attribute-governs-test-transactions.good.al`](transactionmodel-attribute-governs-test-transactions.good.al). ## Anti Pattern -Applying `AutoRollback` to every test method without checking whether the tested business logic calls `Commit`. The test throws at the first Commit, leaving no verdict on the behavior it intended to verify; in a CI run this looks like a flake or a setup bug, not a specification mismatch. The mirror-image anti-pattern is defaulting to `AutoCommit` across the suite "to avoid the error" — without a `TestIsolation` runner this permanently dirties the test database between runs and produces order-dependent test outcomes. +Declaring `[TransactionModel(AutoRollback)]` explicitly on a test method without checking whether the tested business logic calls `Commit`. The test throws at the first Commit, leaving no verdict on the behavior it intended to verify; in a CI run this looks like a flake or a setup bug, not a specification mismatch. The mirror-image anti-pattern is defaulting to `AutoCommit` across the suite "to avoid the error" — without a `TestIsolation` runner this permanently dirties the test database between runs and produces order-dependent test outcomes. Flagging a `Commit()` call in a test method that declares no `TransactionModel` attribute at all is not this anti-pattern — that shape does not error, and is BCApps' own documented pattern for shared lazy fixtures. See sample: [`transactionmodel-attribute-governs-test-transactions.bad.al`](transactionmodel-attribute-governs-test-transactions.bad.al). diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index c96ac83..33016f1 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -50,6 +50,9 @@ The following targeted checks cover every current `testing` article. Treat each - A permission-sensitive test uses `TestPermissions = Disabled`, claims to test a restricted user without `"Permissions Mock"`/`"Library - Lower Permissions"`, or declares `[TestPermissions(...)]` without applying that context — `permission-tests-must-lower-the-execution-context`. - Test fixture code manually calls `Init`/`Insert`, invents keys or prerequisite records, or bypasses available `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, or equivalent library codeunits — `use-library-codeunits-for-test-fixtures`. - `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`. +- A `[ConfirmHandler]` asserts `Question` against a substituted message from a `Confirm` call using its placeholder-substitution overload — `confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text`. +- A shared/lazy `Initialize()`-style fixture helper calls `Commit()` — `commit-shared-test-fixture-inside-lazy-initialize`. +- Changed code subscribes to `OnAfterRemoveTableRelation`, calls `RemoveTableRelation`, or references `Codeunit "Table Relation Test"`/134926 — `table-relation-test-exclude-known-invalid-relations-via-event`. - A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. Only nonoptional handlers have to execute: a listed handler declared `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` is optional by design, so do not treat it as unmatched when the run never raises the notification. Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.