From 12b7f73d24f350cac08d801f0c3b37d85acf43b0 Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Sat, 5 Sep 2026 08:49:44 +0200 Subject: [PATCH 1/6] Add 4 more AL/BC testing patterns from Luc van Vugt's fluxxus.nl blog Fourth batch from CURABIS ApS, mined from an external BC/NAV testing expert's blog archive (fluxxus.nl). Confirm+StrSubstNo interaction with ConfirmHandler, Table Relation Test's OnAfterRemoveTableRelation exclusion hook (verified against BCApps source, codeunit 134926), committing shared lazy-Initialize fixture data, and Assert.IsFalse vs asserterror for boolean checks. --- ...test-fixture-inside-lazy-initialize.bad.al | 19 ++++++++++++++ ...est-fixture-inside-lazy-initialize.good.al | 20 ++++++++++++++ ...red-test-fixture-inside-lazy-initialize.md | 26 +++++++++++++++++++ ...onfirmhandler-sees-substituted-text.bad.al | 9 +++++++ ...nfirmhandler-sees-substituted-text.good.al | 9 +++++++ ...re-confirmhandler-sees-substituted-text.md | 26 +++++++++++++++++++ ...e-known-invalid-relations-via-event.bad.al | 11 ++++++++ ...-known-invalid-relations-via-event.good.al | 10 +++++++ ...clude-known-invalid-relations-via-event.md | 26 +++++++++++++++++++ ...-not-asserterror-for-boolean-checks.bad.al | 18 +++++++++++++ ...not-asserterror-for-boolean-checks.good.al | 18 +++++++++++++ ...alse-not-asserterror-for-boolean-checks.md | 26 +++++++++++++++++++ 12 files changed, 218 insertions(+) create mode 100644 microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al create mode 100644 microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al create mode 100644 microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md create mode 100644 microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.bad.al create mode 100644 microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.good.al create mode 100644 microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.md create mode 100644 microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al create mode 100644 microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al create mode 100644 microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.md create mode 100644 microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al create mode 100644 microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.good.al create mode 100644 microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al new file mode 100644 index 00000000..50f5d542 --- /dev/null +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al @@ -0,0 +1,19 @@ +codeunit 50142 "Sample Test Library" +{ + var + Initialized: Boolean; + + procedure Initialize() + begin + if Initialized then + exit; + + CreateSharedFixtureData(); + Initialized := true; + end; + + local procedure CreateSharedFixtureData() + begin + // insert master/setup data shared across every test in this codeunit + end; +} diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al new file mode 100644 index 00000000..bb5e470a --- /dev/null +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al @@ -0,0 +1,20 @@ +codeunit 50142 "Sample Test Library" +{ + var + Initialized: Boolean; + + procedure Initialize() + begin + if Initialized then + exit; + + CreateSharedFixtureData(); + Commit(); + Initialized := true; + end; + + local procedure CreateSharedFixtureData() + begin + // insert master/setup data shared across every test in this codeunit + end; +} diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md new file mode 100644 index 00000000..dd468784 --- /dev/null +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [initialize, isinitialized, shared-fixture, commit, autorollback, lazy-initialization] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Commit shared fixture data created inside a lazy Initialize(), or later tests lose it + +## Description + +A test codeunit that creates master/setup data once, guarded by an `IsInitialized` flag, to avoid repeating expensive setup across many `[Test]` methods depends on that data surviving into every later test. Each `[Test]` method runs under `AutoRollback` by default, so data inserted during the first test's call to `Initialize()` rolls back at the end of that test. `IsInitialized` is a variable, not persisted data, so it still reads `true` on the next test — but the fixture rows it points to are already gone. + +## Best Practice + +Call `Commit()` at the end of a lazy/shared `Initialize()` procedure, once the shared fixture data is created, so it survives past the first test's rollback boundary. Pair this with a `TestIsolation`-enabled test runner so the committed fixture is still cleaned up at the end of the full run. + +See sample: `commit-shared-test-fixture-inside-lazy-initialize.good.al`. + +## Anti Pattern + +A shared `Initialize()` guarded by `IsInitialized` that creates fixture records but never commits. The first test that runs it passes; every later test in the same codeunit either fails to find the fixture data or silently re-triggers setup logic that `IsInitialized` was meant to skip. + +See sample: `commit-shared-test-fixture-inside-lazy-initialize.bad.al`. diff --git a/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.bad.al b/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.bad.al new file mode 100644 index 00000000..acb825f4 --- /dev/null +++ b/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.bad.al @@ -0,0 +1,9 @@ +codeunit 50140 "Sample Confirm Usage" +{ + procedure ConfirmDeletion(RecordCount: Integer): Boolean + var + ConfirmMsg: Label 'Do you want to delete %1 records?'; + begin + exit(Confirm(ConfirmMsg, false, RecordCount)); + end; +} diff --git a/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.good.al b/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.good.al new file mode 100644 index 00000000..3a529fbb --- /dev/null +++ b/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.good.al @@ -0,0 +1,9 @@ +codeunit 50140 "Sample Confirm Usage" +{ + procedure ConfirmDeletion(RecordCount: Integer): Boolean + var + ConfirmMsg: Label 'Do you want to delete %1 records?'; + begin + exit(Confirm(StrSubstNo(ConfirmMsg, RecordCount), false)); + end; +} 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 new file mode 100644 index 00000000..6601f94f --- /dev/null +++ b/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [confirm, confirmhandler, strsubstno, placeholder, question, substitution] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Build the Confirm message with StrSubstNo, or a ConfirmHandler sees the raw template + +## 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. + +## Best Practice + +When a `ConfirmHandler` needs to assert on the actual message text, build the string with `StrSubstNo(Text, Value)` in the production code first, and pass the already-substituted string to `Confirm()` with no further placeholder arguments. + +See sample: `confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.good.al`. + +## Anti Pattern + +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`. diff --git a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al new file mode 100644 index 00000000..22fe8eda --- /dev/null +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al @@ -0,0 +1,11 @@ +codeunit 50141 "Sample Table Relation Test Ext" +{ + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Table Relation Test", 'OnAfterRemoveTableRelation', '', false, false)] + local procedure ExcludeSampleFieldFromTableRelationTest(var TableRelationsMetadata: Record "Table Relations Metadata" temporary) + var + TableRelationTest: Codeunit "Table Relation Test"; + begin + // Removes every relation on the whole table, not just the one known exception + TableRelationTest.RemoveTableRelation(TableRelationsMetadata, Database::"Sample Header", 0, 0, 0); + end; +} diff --git a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al new file mode 100644 index 00000000..8aa3ab8e --- /dev/null +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al @@ -0,0 +1,10 @@ +codeunit 50141 "Sample Table Relation Test Ext" +{ + [EventSubscriber(ObjectType::Codeunit, Codeunit::"Table Relation Test", 'OnAfterRemoveTableRelation', '', false, false)] + local procedure ExcludeSampleFieldFromTableRelationTest(var TableRelationsMetadata: Record "Table Relations Metadata" temporary) + var + TableRelationTest: Codeunit "Table Relation Test"; + begin + TableRelationTest.RemoveTableRelation(TableRelationsMetadata, Database::"Sample Header", 10, Database::"Sample Setup", 1); + end; +} 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 new file mode 100644 index 00000000..383ebe45 --- /dev/null +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [table-relation-test, tablerelationsmetadata, onafterremovetablerelation, field-length, field-type] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Exclude a known-valid TableRelation exception via OnAfterRemoveTableRelation + +## 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. + +## Best Practice + +Subscribe to `OnAfterRemoveTableRelation` and call the codeunit's own `RemoveTableRelation(TableRelationsMetadata, TableID, FieldID, RelatedTableID, RelatedFieldID)` to strike the one known-valid relation before the test evaluates it, scoped as narrowly as the exception actually is. + +See sample: `table-relation-test-exclude-known-invalid-relations-via-event.good.al`. + +## Anti Pattern + +Excluding an entire table's relations (or disabling the whole test codeunit) to work around one known exception. This discards the check's coverage for every other relation on that table, or in the app, not just the one that needed an exception. + +See sample: `table-relation-test-exclude-known-invalid-relations-via-event.bad.al`. diff --git a/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al new file mode 100644 index 00000000..07ad6b2c --- /dev/null +++ b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al @@ -0,0 +1,18 @@ +codeunit 50143 "Sample Doc Amount Test" +{ + Subtype = Test; + + [Test] + procedure DocAmountIsNotVerifiedWhenLinesAreMissing() + var + Assert: Codeunit Assert; + PurchHeader: Record "Purchase Header"; + begin + asserterror Assert.IsTrue(VerifyDocAmount(PurchHeader), 'Doc. amount should not verify with no lines.'); + end; + + local procedure VerifyDocAmount(var PurchHeader: Record "Purchase Header"): Boolean + begin + exit(false); + end; +} diff --git a/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.good.al b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.good.al new file mode 100644 index 00000000..eb21d6cb --- /dev/null +++ b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.good.al @@ -0,0 +1,18 @@ +codeunit 50143 "Sample Doc Amount Test" +{ + Subtype = Test; + + [Test] + procedure DocAmountIsNotVerifiedWhenLinesAreMissing() + var + Assert: Codeunit Assert; + PurchHeader: Record "Purchase Header"; + begin + Assert.IsFalse(VerifyDocAmount(PurchHeader), 'Doc. amount should not verify with no lines.'); + end; + + local procedure VerifyDocAmount(var PurchHeader: Record "Purchase Header"): Boolean + begin + exit(false); + end; +} diff --git a/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md new file mode 100644 index 00000000..926e4349 --- /dev/null +++ b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [assert, isfalse, istrue, asserterror, boolean-check, negative-test] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Use Assert.IsFalse to check a boolean result, not asserterror around Assert.IsTrue + +## Description + +`asserterror` exists to assert that a statement raises a runtime error; it is not a general-purpose way to invert a boolean check. Wrapping `asserterror Assert.IsTrue(SomeFunc(), Msg)` to verify that `SomeFunc()` returns `false` tests whether `Assert.IsTrue`'s own error-raising behavior fired, not the value `SomeFunc()` actually returned. + +## Best Practice + +When the code under test returns a `Boolean` rather than raising an error, assert the value directly with `Assert.IsFalse(SomeFunc(), Msg)` (or `Assert.IsTrue` for the positive case). Reserve `asserterror` for statements expected to actually raise an error. + +See sample: `use-assert-isfalse-not-asserterror-for-boolean-checks.good.al`. + +## Anti Pattern + +`asserterror Assert.IsTrue(SomeFunc(), Msg);` to verify `SomeFunc()` is `false`. It passes today because `Assert.IsTrue` happens to raise an error on failure, but it verifies the assertion helper's error-raising behavior, not the value under test. + +See sample: `use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al`. From dcd79afc089fd2abca09b866b317949ede3caf98 Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Mon, 7 Sep 2026 21:06:58 +0200 Subject: [PATCH 2/6] Address Jesper Schulz-Wedde's review on PR #159 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - transactionmodel-attribute-governs-test-transactions.md: the "Commit causes an error" behavior is specific to an explicitly declared AutoRollback attribute. A test method with no TransactionModel attribute at all is a distinct, valid shape — BCApps' own codeunit 134915 "ERM Online Mapping Setup" commits inside a lazy Initialize() with no attribute declared, cleaning up via a manual asserterror at the end. Evidence for commit-shared-test-fixture- inside-lazy-initialize.md (this PR), which is correct as submitted. - confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.md: reframe as a known, unconfirmed-fix platform defect (microsoft/ALAppExtensions#23935) rather than designed behavior; add the Message/MessageHandler asymmetry as supporting evidence. - table-relation-test-exclude-known-invalid-relations-via-event.md: note the test-app-only consumer dependency; correct "walks every TableRelation field property in the app" to the actual tenant-wide Table Relations Metadata scope across installed apps. - Wire confirm-needs-strsubstno, commit-shared-test-fixture-inside- lazy-initialize, and table-relation-test-exclude-known-invalid- relations-via-event into al-testing-review.md's candidate-selection cues. Co-Authored-By: Claude Sonnet 5 --- ...trsubstno-before-confirmhandler-sees-substituted-text.md | 6 +++++- ...lation-test-exclude-known-invalid-relations-via-event.md | 2 +- .../transactionmodel-attribute-governs-test-transactions.md | 6 +++--- microsoft/skills/review/al-testing-review.md | 3 +++ 4 files changed, 12 insertions(+), 5 deletions(-) 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 6601f94f..f92a7830 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 383ebe45..a3af2be8 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 c9c159ac..ae24f1f4 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 c96ac833..33016f16 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`. From 1392521a8e1a5ef484ab3e2505d88be1a0ec50b3 Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Tue, 8 Sep 2026 19:02:03 +0200 Subject: [PATCH 3/6] Address second round of Jesper Schulz-Wedde's review on PR #159 - commit-shared-test-fixture-inside-lazy-initialize.md: fundamentally rewritten. AutoCommit is the documented default TransactionModel, not AutoRollback. Explains the real mechanism (Commit() protects a fixture from the test method's own later deliberate rollback, per Codeunit.Run/ TransactionModel-property semantics) and the TestIsolation dependency (Disabled/Codeunit survive across methods, Function does not). Fixtures rewritten to demonstrate the actual failure/success shape. - transactionmodel-attribute-governs-test-transactions.md: now states the AutoCommit default explicitly and agrees with the article above, closing the contradiction Jesper flagged between the two testing articles. - Deleted confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text (.md/.good.al/.bad.al): the underlying platform bug (microsoft/ ALAppExtensions#23935) was closed as completed in Feb 2024; cannot be reproduced or bc-version-pinned on any currently supported version. - table-relation-test-exclude-known-invalid-relations-via-event.md: added the [Scope('OnPrem')] boundary verified against BCApps' Table Relation Test codeunit. - use-assert-isfalse-not-asserterror-for-boolean-checks.md: added a Scope section resolving the overlap with asserterror-needs-expectederror-and-code. - al-testing-review.md: fixed the shared-fixture cue to catch the actual anti-pattern instead of the compliant shape, added the missing cue for use-assert-isfalse-not-asserterror-for-boolean-checks, wired precedence between it and the generic asserterror rule, and removed the cue for the deleted article. - Added in-file Source provenance (specific fluxxus.nl post per article, with what was independently verified vs. taken from the post) to the three surviving externally-inspired articles, per Jesper's request that provenance live in the knowledge file itself, not only the PR description. Co-Authored-By: Claude Sonnet 5 --- ...test-fixture-inside-lazy-initialize.bad.al | 17 ++++++++++- ...est-fixture-inside-lazy-initialize.good.al | 15 +++++++++- ...red-test-fixture-inside-lazy-initialize.md | 16 +++++++--- ...onfirmhandler-sees-substituted-text.bad.al | 9 ------ ...nfirmhandler-sees-substituted-text.good.al | 9 ------ ...re-confirmhandler-sees-substituted-text.md | 30 ------------------- ...clude-known-invalid-relations-via-event.md | 8 +++-- ...del-attribute-governs-test-transactions.md | 8 +++-- ...alse-not-asserterror-for-boolean-checks.md | 8 +++++ microsoft/skills/review/al-testing-review.md | 6 ++-- 10 files changed, 65 insertions(+), 61 deletions(-) delete mode 100644 microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.bad.al delete mode 100644 microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.good.al delete mode 100644 microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.md diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al index 50f5d542..171ce5d4 100644 --- a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al @@ -1,9 +1,12 @@ codeunit 50142 "Sample Test Library" { + Subtype = Test; + var Initialized: Boolean; + RollBackMsg: Label 'Revert back the tables to their original state.'; - procedure Initialize() + local procedure Initialize() begin if Initialized then exit; @@ -16,4 +19,16 @@ codeunit 50142 "Sample Test Library" begin // insert master/setup data shared across every test in this codeunit end; + + [Test] + procedure FirstTestUsesSharedFixture() + begin + Initialize(); + + // exercise/verify against the shared fixture, then make scratch changes of its own + + asserterror Error(RollBackMsg); + // the deliberate rollback above also erases the never-committed fixture; + // Initialized still reads true on the next test, but the rows are gone + end; } diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al index bb5e470a..1aa16c8c 100644 --- a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al @@ -1,9 +1,12 @@ codeunit 50142 "Sample Test Library" { + Subtype = Test; + var Initialized: Boolean; + RollBackMsg: Label 'Revert back the tables to their original state.'; - procedure Initialize() + local procedure Initialize() begin if Initialized then exit; @@ -17,4 +20,14 @@ codeunit 50142 "Sample Test Library" begin // insert master/setup data shared across every test in this codeunit end; + + [Test] + procedure FirstTestUsesSharedFixture() + begin + Initialize(); + + // exercise/verify against the shared fixture, then make scratch changes of its own + + asserterror Error(RollBackMsg); + end; } diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md index dd468784..3321e05d 100644 --- a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: testing -keywords: [initialize, isinitialized, shared-fixture, commit, autorollback, lazy-initialization] +keywords: [initialize, isinitialized, shared-fixture, commit, autocommit, asserterror, testisolation, lazy-initialization] technologies: [al] countries: [w1] application-area: [all] @@ -11,16 +11,24 @@ application-area: [all] ## Description -A test codeunit that creates master/setup data once, guarded by an `IsInitialized` flag, to avoid repeating expensive setup across many `[Test]` methods depends on that data surviving into every later test. Each `[Test]` method runs under `AutoRollback` by default, so data inserted during the first test's call to `Initialize()` rolls back at the end of that test. `IsInitialized` is a variable, not persisted data, so it still reads `true` on the next test — but the fixture rows it points to are already gone. +A test method with no `[TransactionModel(...)]` attribute defaults to `AutoCommit` (see `transactionmodel-attribute-governs-test-transactions.md`): a method that completes without error commits automatically at its own boundary, with no explicit `Commit()` needed. So a lazy/shared `Initialize()` — guarded by an `IsInitialized` flag, creating master/setup data once to avoid repeating expensive setup across many `[Test]` methods — does not need `Commit()` just to survive into the next test method; under the default model it already will. (Declaring `[TransactionModel(AutoRollback)]` instead is not compatible with this pattern at all: `AutoRollback` assumes the code under test never commits, and a `Commit()` call under it raises a runtime error.) + +What an early `Commit()` inside `Initialize()` actually guards against is the test method's *own later, deliberate* rollback — the BCApps cleanup idiom of ending a test with `asserterror Error(SomeLabel)` to undo demo-data mutations that method made, so the run doesn't permanently dirty the database. Per the documented `Codeunit.Run` transaction semantics, changes are committed at the end of an execution "unless an error occurs" — an unhandled error rolls back whatever wasn't already committed. `Commit()` closes out the fixture's own transaction immediately, so it is unaffected by whatever the rest of that method does afterward, including that end-of-test error. Without the early `Commit()`, the same deliberate rollback wipes out the fixture too, even though `IsInitialized` still reads `true` on the next test, since it's a plain variable, not persisted data. BCApps' `codeunit 134915 "ERM Online Mapping Setup"` shows exactly this shape: no `TransactionModel` attribute, `Commit()` inside a lazy `Initialize()`, and the test itself ends with `asserterror Error(RollBackMessage)`. + +Protecting the fixture from that same-method rollback is necessary but not sufficient for the fixture to reach a *later* test method — that also depends on the executing test runner's `TestIsolation`. Under `Disabled` (the property's own documented default) or `Codeunit` (used by BCApps' own `TestRunner`, `CLITestRunner`, and `SnapTestRunner` codeunits), nothing rolls back until the whole test codeunit finishes, so the already-committed fixture survives across every method run before then. Under `Function`, the runner rolls back all database changes — explicitly including ones already committed via `Commit()` — after every single test method; no amount of committing inside `Initialize()` makes a fixture shared across methods survive that regime, because the whole premise of a lazy, once-per-codeunit fixture doesn't hold when every method is isolated from every other. ## Best Practice -Call `Commit()` at the end of a lazy/shared `Initialize()` procedure, once the shared fixture data is created, so it survives past the first test's rollback boundary. Pair this with a `TestIsolation`-enabled test runner so the committed fixture is still cleaned up at the end of the full run. +When a test method's own cleanup relies on ending in a deliberate error to roll back its scratch changes, call `Commit()` once, inside the lazy `Initialize()` guard, right after the shared fixture is created — before that cleanup-triggering error can run. This pattern only delivers a fixture shared across test methods when the executing runner's `TestIsolation` is `Disabled` or `Codeunit`; do not recommend it, or pair it with, a `Function`-isolated runner — that configuration undoes the committed fixture after every method regardless. See sample: `commit-shared-test-fixture-inside-lazy-initialize.good.al`. ## Anti Pattern -A shared `Initialize()` guarded by `IsInitialized` that creates fixture records but never commits. The first test that runs it passes; every later test in the same codeunit either fails to find the fixture data or silently re-triggers setup logic that `IsInitialized` was meant to skip. +A shared `Initialize()` guarded by `IsInitialized` that creates fixture records without committing, in a test method that ends with a deliberate `asserterror Error(...)` to undo its own scratch changes, run under a `Disabled`- or `Codeunit`-isolated test runner. That rollback also erases the never-committed fixture; the next test still finds `IsInitialized = true` but the rows it depends on are gone. (Under a `Function`-isolated runner the fixture is lost regardless of `Commit()`, for the unrelated reason above — that is a runner-configuration problem, not this anti-pattern.) See sample: `commit-shared-test-fixture-inside-lazy-initialize.bad.al`. + +## Source + +The shared/lazy `Initialize()` pattern and its `Commit()` call are drawn from Luc van Vugt's "Let's talk about Shared Fixture and how to profit from this with the Dynamics NAV Test Toolkit": https://www.fluxxus.nl/index.php/bc/let39s-talk-about-shared-fixture-and-how-to-profit-from-this-with-the-dynamics-nav-test-toolkit/. That post shows the `Commit()` call in its `Initialize()` example but does not explain the transaction mechanics behind it; the `AutoCommit`-default, `Codeunit.Run`-error, and `TestIsolation`-level analysis above is this article's own, verified independently against Microsoft's TransactionModel/TestIsolation documentation and BCApps' `codeunit 134915 "ERM Online Mapping Setup"` source, not taken from the post. diff --git a/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.bad.al b/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.bad.al deleted file mode 100644 index acb825f4..00000000 --- a/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.bad.al +++ /dev/null @@ -1,9 +0,0 @@ -codeunit 50140 "Sample Confirm Usage" -{ - procedure ConfirmDeletion(RecordCount: Integer): Boolean - var - ConfirmMsg: Label 'Do you want to delete %1 records?'; - begin - exit(Confirm(ConfirmMsg, false, RecordCount)); - end; -} diff --git a/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.good.al b/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.good.al deleted file mode 100644 index 3a529fbb..00000000 --- a/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.good.al +++ /dev/null @@ -1,9 +0,0 @@ -codeunit 50140 "Sample Confirm Usage" -{ - procedure ConfirmDeletion(RecordCount: Integer): Boolean - var - ConfirmMsg: Label 'Do you want to delete %1 records?'; - begin - exit(Confirm(StrSubstNo(ConfirmMsg, RecordCount), false)); - end; -} 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 deleted file mode 100644 index f92a7830..00000000 --- a/microsoft/knowledge/testing/confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.md +++ /dev/null @@ -1,30 +0,0 @@ ---- -bc-version: [all] -domain: testing -keywords: [confirm, confirmhandler, strsubstno, placeholder, question, substitution] -technologies: [al] -countries: [w1] -application-area: [all] ---- - -# Build the Confirm message with StrSubstNo, or a ConfirmHandler sees the raw template - -## 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. 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 - -When a `ConfirmHandler` needs to assert on the actual message text, build the string with `StrSubstNo(Text, Value)` in the production code first, and pass the already-substituted string to `Confirm()` with no further placeholder arguments. - -See sample: `confirm-needs-strsubstno-before-confirmhandler-sees-substituted-text.good.al`. - -## Anti Pattern - -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 a3af2be8..f680f1a4 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,11 +11,11 @@ application-area: [all] ## Description -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. +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. The validation test method itself is `[Scope('OnPrem')]`: it only runs from an on-premises test surface, not from a cloud-targeted test app, so this whole exception mechanism — and the check it works around — is only reachable where that test can actually execute. ## Best Practice -Subscribe to `OnAfterRemoveTableRelation` and call the codeunit's own `RemoveTableRelation(TableRelationsMetadata, TableID, FieldID, RelatedTableID, RelatedFieldID)` to strike the one known-valid relation before the test evaluates it, scoped as narrowly as the exception actually is. +Subscribe to `OnAfterRemoveTableRelation` and call the codeunit's own `RemoveTableRelation(TableRelationsMetadata, TableID, FieldID, RelatedTableID, RelatedFieldID)` to strike the one known-valid relation before the test evaluates it, scoped as narrowly as the exception actually is. Because the test itself is `[Scope('OnPrem')]`, do not recommend subscribing to it as a way to guard a cloud-targeted app's test suite — the subscription has no effect where the test never runs. See sample: `table-relation-test-exclude-known-invalid-relations-via-event.good.al`. @@ -24,3 +24,7 @@ See sample: `table-relation-test-exclude-known-invalid-relations-via-event.good. Excluding an entire table's relations (or disabling the whole test codeunit) to work around one known exception. This discards the check's coverage for every other relation on that table, or in the app, not just the one that needed an exception. See sample: `table-relation-test-exclude-known-invalid-relations-via-event.bad.al`. + +## Source + +The `OnAfterRemoveTableRelation` exclusion technique is drawn from Luc van Vugt's "How-to: Test your Table Relations (2)": https://www.fluxxus.nl/index.php/bc/how-to-test-your-table-relations-2/. The codeunit/event signature, the `[Scope('OnPrem')]` boundary, and the tenant-wide `Table Relations Metadata` scope described above were verified directly against BCApps' `codeunit 134926 "Table Relation Test"` source, not taken from the post. diff --git a/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md b/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md index ae24f1f4..0f4f3bd2 100644 --- a/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md +++ b/microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md @@ -11,11 +11,11 @@ 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 "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. +`[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`. **`AutoCommit` is the documented default** — a test method with no `[TransactionModel(...)]` attribute at all runs under `AutoCommit`, not `AutoRollback` and not `None` (Microsoft's TransactionModel property reference states this explicitly: "AutoCommit is the default value"). The "a call to `Commit` produces a runtime error" behavior is specific to the *explicitly declared* `AutoRollback` attribute. BCApps' own canonical pattern for a lazily-initialized shared fixture (see `codeunit 134915 "ERM Online Mapping Setup"`) declares no `TransactionModel` attribute at all — so it runs under the `AutoCommit` default — 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. Per the same reference, under `AutoCommit` an error, even one caught by `asserterror`, still rolls back the transaction — but "only to the point at which `Commit` was called" if the code being tested committed first. 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 -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`. +Leave `[TransactionModel(...)]` undeclared to get the `AutoCommit` default when the codeunit's own tests rely on that default's behavior — for example a lazily-initialized shared fixture that commits once and cleans up its own scratch changes with a manual `asserterror`-based rollback (see `commit-shared-test-fixture-inside-lazy-initialize.md`); do not treat that absence as equivalent to declaring `AutoRollback`. When declaring `[TransactionModel(...)]` explicitly instead, 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 an intentional, suite-wide reliance on `AutoCommit` with a `TestIsolation`-enabled test runner so committed changes are reverted at a higher scope. See sample: [`transactionmodel-attribute-governs-test-transactions.good.al`](transactionmodel-attribute-governs-test-transactions.good.al). @@ -24,3 +24,7 @@ See sample: [`transactionmodel-attribute-governs-test-transactions.good.al`](tra 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). + +## Source + +The `AutoCommit`-is-default claim and the exact rollback-to-last-`Commit` mechanics are quoted from Microsoft's TransactionModel Property reference: https://learn.microsoft.com/en-us/previous-versions/dynamicsnav-2018-developer/TransactionModel-Property. The current AL [TransactionModel attribute](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/attributes/devenv-transactionmodel-attribute) page describes the same three values but never states a default; this older property reference is the citable source for that fact. diff --git a/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md index 926e4349..df1f7919 100644 --- a/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md +++ b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md @@ -24,3 +24,11 @@ See sample: `use-assert-isfalse-not-asserterror-for-boolean-checks.good.al`. `asserterror Assert.IsTrue(SomeFunc(), Msg);` to verify `SomeFunc()` is `false`. It passes today because `Assert.IsTrue` happens to raise an error on failure, but it verifies the assertion helper's error-raising behavior, not the value under test. See sample: `use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al`. + +## Source + +Drawn from Luc van Vugt's "TDD in NAV – ASSERTERROR or IsFalse": https://www.fluxxus.nl/index.php/bc/tdd-in-nav-asserterror-or-isfalse/. The post's own example and reasoning — reserve `asserterror` for the product code actually raising an error, use `Assert.IsFalse`/`Assert.IsTrue` to check a boolean the test framework itself computes — carries over directly; the overlap with `asserterror-needs-expectederror-and-code.md` below is this repository's own addition, not from the source. + +## Scope + +This rule and `asserterror-needs-expectederror-and-code.md` can both match `asserterror Assert.IsTrue(SomeFunc(), Msg);` with nothing after it — the generic rule sees a bare `asserterror`, this one sees `asserterror` wrapping an `Assert.IsTrue`/`Assert.IsFalse` call used to invert a boolean. This rule wins for that shape: the fix is to replace the construct with a direct `Assert.IsFalse`/`Assert.IsTrue` call, not to add `Assert.ExpectedError`/`Assert.ExpectedErrorCode` after it. `asserterror-needs-expectederror-and-code.md` still applies on its own to every other bare `asserterror`, including one guarding `Assert.IsTrue`/`Assert.IsFalse` where the intent genuinely is to assert that the guarded call itself raises an error (for example, asserting that a validation helper errors before it can even return a boolean). diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index 33016f16..d7087d07 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -49,9 +49,9 @@ The following targeted checks cover every current `testing` article. Treat each - An `AutoCommit` test runs under a `Subtype = TestRunner` codeunit that omits `TestIsolation` or sets it to `Disabled`, leaving committed data between tests — `testisolation-belongs-on-the-test-runner`. Require runner/repository context; a standalone test file cannot prove which runner executes it. - 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`. +- `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`. Exclude `asserterror Assert.IsTrue(...)` / `asserterror Assert.IsFalse(...)` guarding a `Boolean`-returning call — that shape belongs to `use-assert-isfalse-not-asserterror-for-boolean-checks` instead, which wins for it. +- `asserterror` wraps `Assert.IsTrue(BooleanExpression, ...)` (or the `IsFalse` mirror) solely to invert the boolean result of the guarded call, rather than to assert that call itself raises an error — `use-assert-isfalse-not-asserterror-for-boolean-checks`. +- A shared/lazy `Initialize()`-style fixture helper creates fixture data without a following `Commit()`, in a test method whose body later forces its own rollback (for example `asserterror Error(...)` used for end-of-test cleanup) — `commit-shared-test-fixture-inside-lazy-initialize`. The presence of `Commit()` after the fixture is the compliant shape, not the signal to look for; the missing-`Commit()` shape combined with a later deliberate rollback is the anti-pattern. Require runner/repository context for the `TestIsolation` value: a standalone test file cannot prove which runner executes it, and under `Function`-level isolation this whole pattern is moot regardless of `Commit()` — do not raise the finding when the executing runner's `TestIsolation` is known to be `Function`. - 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. From 69b09db3a6378f45f49ca4eb745ad6e20f0502f7 Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Mon, 21 Sep 2026 22:49:39 +0200 Subject: [PATCH 4/6] Fix remaining correctness issues from Jesper's 2026-09-15 re-review - commit-shared-test-fixture-inside-lazy-initialize: three sub-issues. Recommended TestIsolation = Codeunit instead of listing Disabled as an equal option - Disabled never rolls back at all ("tests are not isolated from each other" per the property's own docs), so a fixture this pattern commits under Disabled is permanent database contamination unless something else tears it down; Disabled is now only mentioned alongside that explicit teardown requirement. Added precedence in al-testing-review.md so the deliberate end-of-test asserterror Error(...) rollback sentinel isn't also flagged by the generic asserterror-needs-expectederror-and-code rule. Rewrote both fixtures to actually demonstrate the pattern: persisted fixture data (an Item record) instead of an empty comment, a second [Test] method that depends on the fixture surviving into it, and an explicit Subtype = TestRunner / TestIsolation = Codeunit runner codeunit. - table-relation-test-exclude-known-invalid-relations-via-event: the length/type rule was stated as one global requirement. Verified ValidateFieldRelation in codeunit 134926 directly (BCApps reference clone) and split it into the two branches the source actually has: a field with any unconditional relation needs exact length and exact resolved type; a field whose relations are all conditional only fails on being shorter (longer is fine) than the largest related field, and when the required type is specifically Code, a Text source passes too - a tolerance that does not apply on the unconditional side and does not extend to a required Text. Rebased onto upstream/main (one conflict in transactionmodel-attribute-governs-test-transactions.md - upstream had already linked its sample references via the READ convention, ours added a Source section; merged both). Also converted the 3 remaining plain-backtick sample references in this PR to the READ-convention markdown-link form, same fix as #156/#157/#158. --- ...test-fixture-inside-lazy-initialize.bad.al | 48 +++++++++++++++++-- ...est-fixture-inside-lazy-initialize.good.al | 43 ++++++++++++++++- ...red-test-fixture-inside-lazy-initialize.md | 8 ++-- ...clude-known-invalid-relations-via-event.md | 11 +++-- ...alse-not-asserterror-for-boolean-checks.md | 4 +- microsoft/skills/review/al-testing-review.md | 2 +- 6 files changed, 100 insertions(+), 16 deletions(-) diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al index 171ce5d4..2c13967e 100644 --- a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al @@ -4,6 +4,7 @@ codeunit 50142 "Sample Test Library" var Initialized: Boolean; + SharedItemNo: Code[20]; RollBackMsg: Label 'Revert back the tables to their original state.'; local procedure Initialize() @@ -12,23 +13,62 @@ codeunit 50142 "Sample Test Library" exit; CreateSharedFixtureData(); + // BUG: no Commit() here. The fixture below is still inside this + // test method's own transaction. Initialized := true; end; local procedure CreateSharedFixtureData() + var + Item: Record Item; begin - // insert master/setup data shared across every test in this codeunit + Item.Init(); + Item."No." := 'SAMPLE-SHARED'; + Item.Insert(true); + SharedItemNo := Item."No."; end; [Test] procedure FirstTestUsesSharedFixture() + var + Item: Record Item; begin Initialize(); - // exercise/verify against the shared fixture, then make scratch changes of its own + Item.Get(SharedItemNo); + Item.Description := 'Scratch change this test makes and does not need to keep.'; + Item.Modify(); asserterror Error(RollBackMsg); - // the deliberate rollback above also erases the never-committed fixture; - // Initialized still reads true on the next test, but the rows are gone + // The deliberate rollback above also erases the never-committed + // fixture from CreateSharedFixtureData(). Initialized still reads + // true on the next test, but the row it points at is gone. + end; + + [Test] + procedure SecondTestStillFindsSharedFixture() + var + Item: Record Item; + begin + Initialize(); + + // Fails here: Initialize() saw Initialized = true and returned + // immediately, so it never recreated the fixture - and the first + // test's rollback took the original row with it. + Item.Get(SharedItemNo); + end; +} + +codeunit 50143 "Sample Test Runner" +{ + // Codeunit isolation alone does not save this fixture: TestIsolation + // only controls whether committed changes survive between methods, and + // this fixture was never committed in the first place. + Subtype = TestRunner; + TestIsolation = Codeunit; + + trigger OnRun() + begin + Codeunit.Run(Codeunit::"Sample Test Library"); end; } diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al index 1aa16c8c..0879e6aa 100644 --- a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al @@ -4,6 +4,7 @@ codeunit 50142 "Sample Test Library" var Initialized: Boolean; + SharedItemNo: Code[20]; RollBackMsg: Label 'Revert back the tables to their original state.'; local procedure Initialize() @@ -17,17 +18,55 @@ codeunit 50142 "Sample Test Library" end; local procedure CreateSharedFixtureData() + var + Item: Record Item; begin - // insert master/setup data shared across every test in this codeunit + Item.Init(); + Item."No." := 'SAMPLE-SHARED'; + Item.Insert(true); + SharedItemNo := Item."No."; end; [Test] procedure FirstTestUsesSharedFixture() + var + Item: Record Item; begin Initialize(); - // exercise/verify against the shared fixture, then make scratch changes of its own + Item.Get(SharedItemNo); + Item.Description := 'Scratch change this test makes and does not need to keep.'; + Item.Modify(); asserterror Error(RollBackMsg); + // Rolls back the Modify() above, but not the fixture: that was + // already committed inside Initialize(). + end; + + [Test] + procedure SecondTestStillFindsSharedFixture() + var + Item: Record Item; + begin + // Runs after FirstTestUsesSharedFixture's deliberate rollback. + // Initialize() sees Initialized = true and does nothing, but the + // committed fixture it created earlier is still there to Get(). + Initialize(); + + Item.Get(SharedItemNo); + end; +} + +codeunit 50143 "Sample Test Runner" +{ + // Codeunit isolation: everything this codeunit's tests commit, + // including the shared fixture, survives from one test method to the + // next, and rolls back only once every method in the codeunit has run. + Subtype = TestRunner; + TestIsolation = Codeunit; + + trigger OnRun() + begin + Codeunit.Run(Codeunit::"Sample Test Library"); end; } diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md index 3321e05d..7ec16dff 100644 --- a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.md @@ -15,19 +15,19 @@ A test method with no `[TransactionModel(...)]` attribute defaults to `AutoCommi What an early `Commit()` inside `Initialize()` actually guards against is the test method's *own later, deliberate* rollback — the BCApps cleanup idiom of ending a test with `asserterror Error(SomeLabel)` to undo demo-data mutations that method made, so the run doesn't permanently dirty the database. Per the documented `Codeunit.Run` transaction semantics, changes are committed at the end of an execution "unless an error occurs" — an unhandled error rolls back whatever wasn't already committed. `Commit()` closes out the fixture's own transaction immediately, so it is unaffected by whatever the rest of that method does afterward, including that end-of-test error. Without the early `Commit()`, the same deliberate rollback wipes out the fixture too, even though `IsInitialized` still reads `true` on the next test, since it's a plain variable, not persisted data. BCApps' `codeunit 134915 "ERM Online Mapping Setup"` shows exactly this shape: no `TransactionModel` attribute, `Commit()` inside a lazy `Initialize()`, and the test itself ends with `asserterror Error(RollBackMessage)`. -Protecting the fixture from that same-method rollback is necessary but not sufficient for the fixture to reach a *later* test method — that also depends on the executing test runner's `TestIsolation`. Under `Disabled` (the property's own documented default) or `Codeunit` (used by BCApps' own `TestRunner`, `CLITestRunner`, and `SnapTestRunner` codeunits), nothing rolls back until the whole test codeunit finishes, so the already-committed fixture survives across every method run before then. Under `Function`, the runner rolls back all database changes — explicitly including ones already committed via `Commit()` — after every single test method; no amount of committing inside `Initialize()` makes a fixture shared across methods survive that regime, because the whole premise of a lazy, once-per-codeunit fixture doesn't hold when every method is isolated from every other. +Protecting the fixture from that same-method rollback is necessary but not sufficient for the fixture to reach a *later* test method — that also depends on the executing test runner's `TestIsolation`. Under `Disabled` (the property's own documented default) or `Codeunit` (used by BCApps' own `TestRunner`, `CLITestRunner`, and `SnapTestRunner` codeunits), nothing rolls back until the whole test codeunit finishes, so the already-committed fixture survives across every method run before then. These two are not interchangeable, though: `Codeunit` rolls back everything once the codeunit's last method completes, so the environment is clean afterward; `Disabled` never rolls back anything at all — "tests are not isolated from each other" is the property's own description — so a fixture this pattern commits stays in the database permanently unless something else explicitly deletes it. Under `Function`, the runner rolls back all database changes — explicitly including ones already committed via `Commit()` — after every single test method; no amount of committing inside `Initialize()` makes a fixture shared across methods survive that regime, because the whole premise of a lazy, once-per-codeunit fixture doesn't hold when every method is isolated from every other. ## Best Practice -When a test method's own cleanup relies on ending in a deliberate error to roll back its scratch changes, call `Commit()` once, inside the lazy `Initialize()` guard, right after the shared fixture is created — before that cleanup-triggering error can run. This pattern only delivers a fixture shared across test methods when the executing runner's `TestIsolation` is `Disabled` or `Codeunit`; do not recommend it, or pair it with, a `Function`-isolated runner — that configuration undoes the committed fixture after every method regardless. +When a test method's own cleanup relies on ending in a deliberate error to roll back its scratch changes, call `Commit()` once, inside the lazy `Initialize()` guard, right after the shared fixture is created — before that cleanup-triggering error can run. This pattern only delivers a fixture shared across test methods when the executing runner's `TestIsolation` is `Disabled` or `Codeunit`; do not recommend it, or pair it with, a `Function`-isolated runner — that configuration undoes the committed fixture after every method regardless. Recommend `TestIsolation = Codeunit`: it gives every method in the codeunit the same shared, committed fixture and still leaves the database clean once the codeunit finishes. Recommend `Disabled` only alongside an explicit, verified teardown step that removes the fixture data at the end of the run — without one, the committed fixture is permanent contamination, not a controlled trade-off. -See sample: `commit-shared-test-fixture-inside-lazy-initialize.good.al`. +See sample: [`commit-shared-test-fixture-inside-lazy-initialize.good.al`](commit-shared-test-fixture-inside-lazy-initialize.good.al). ## Anti Pattern A shared `Initialize()` guarded by `IsInitialized` that creates fixture records without committing, in a test method that ends with a deliberate `asserterror Error(...)` to undo its own scratch changes, run under a `Disabled`- or `Codeunit`-isolated test runner. That rollback also erases the never-committed fixture; the next test still finds `IsInitialized = true` but the rows it depends on are gone. (Under a `Function`-isolated runner the fixture is lost regardless of `Commit()`, for the unrelated reason above — that is a runner-configuration problem, not this anti-pattern.) -See sample: `commit-shared-test-fixture-inside-lazy-initialize.bad.al`. +See sample: [`commit-shared-test-fixture-inside-lazy-initialize.bad.al`](commit-shared-test-fixture-inside-lazy-initialize.bad.al). ## Source 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 f680f1a4..3f7d7780 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,19 +11,24 @@ application-area: [all] ## Description -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. The validation test method itself is `[Scope('OnPrem')]`: it only runs from an on-premises test surface, not from a cloud-targeted test app, so this whole exception mechanism — and the check it works around — is only reachable where that test can actually execute. +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 validates each field's type and length against what its relations require — but the exact rule depends on whether that field has an *unconditional* relation (a `Table Relations Metadata` row with `Condition Field No. = 0`) among its relations, or only *conditional* ones: + +- If any relation is unconditional, the field's length must equal *exactly* the largest related field's length, and its type must exactly match the required type — resolved to `Text` when the related fields themselves mix `Code` and `Text`. +- If every relation for that field is conditional, the requirement relaxes: the field only needs to be *at least* as long as the largest related field (longer is accepted; only shorter fails), and when the required type is specifically `Code`, both a `Code` and a `Text` source field pass. That `Code`/`Text` tolerance is conditional-only — it does not apply on the unconditional side, and it does not extend to a required type of `Text` (a `Code` source field does not satisfy a required `Text`). + +A field with a legitimate, intentional relation shape outside both of these tolerances has no per-field override in its own object definition; the check runs with no built-in escape hatch beyond them. The validation test method itself is `[Scope('OnPrem')]`: it only runs from an on-premises test surface, not from a cloud-targeted test app, so this whole exception mechanism — and the check it works around — is only reachable where that test can actually execute. ## Best Practice Subscribe to `OnAfterRemoveTableRelation` and call the codeunit's own `RemoveTableRelation(TableRelationsMetadata, TableID, FieldID, RelatedTableID, RelatedFieldID)` to strike the one known-valid relation before the test evaluates it, scoped as narrowly as the exception actually is. Because the test itself is `[Scope('OnPrem')]`, do not recommend subscribing to it as a way to guard a cloud-targeted app's test suite — the subscription has no effect where the test never runs. -See sample: `table-relation-test-exclude-known-invalid-relations-via-event.good.al`. +See sample: [`table-relation-test-exclude-known-invalid-relations-via-event.good.al`](table-relation-test-exclude-known-invalid-relations-via-event.good.al). ## Anti Pattern Excluding an entire table's relations (or disabling the whole test codeunit) to work around one known exception. This discards the check's coverage for every other relation on that table, or in the app, not just the one that needed an exception. -See sample: `table-relation-test-exclude-known-invalid-relations-via-event.bad.al`. +See sample: [`table-relation-test-exclude-known-invalid-relations-via-event.bad.al`](table-relation-test-exclude-known-invalid-relations-via-event.bad.al). ## Source diff --git a/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md index df1f7919..182a62b5 100644 --- a/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md +++ b/microsoft/knowledge/testing/use-assert-isfalse-not-asserterror-for-boolean-checks.md @@ -17,13 +17,13 @@ application-area: [all] When the code under test returns a `Boolean` rather than raising an error, assert the value directly with `Assert.IsFalse(SomeFunc(), Msg)` (or `Assert.IsTrue` for the positive case). Reserve `asserterror` for statements expected to actually raise an error. -See sample: `use-assert-isfalse-not-asserterror-for-boolean-checks.good.al`. +See sample: [`use-assert-isfalse-not-asserterror-for-boolean-checks.good.al`](use-assert-isfalse-not-asserterror-for-boolean-checks.good.al). ## Anti Pattern `asserterror Assert.IsTrue(SomeFunc(), Msg);` to verify `SomeFunc()` is `false`. It passes today because `Assert.IsTrue` happens to raise an error on failure, but it verifies the assertion helper's error-raising behavior, not the value under test. -See sample: `use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al`. +See sample: [`use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al`](use-assert-isfalse-not-asserterror-for-boolean-checks.bad.al). ## Source diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index d7087d07..eaf73af6 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -49,7 +49,7 @@ The following targeted checks cover every current `testing` article. Treat each - An `AutoCommit` test runs under a `Subtype = TestRunner` codeunit that omits `TestIsolation` or sets it to `Disabled`, leaving committed data between tests — `testisolation-belongs-on-the-test-runner`. Require runner/repository context; a standalone test file cannot prove which runner executes it. - 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`. Exclude `asserterror Assert.IsTrue(...)` / `asserterror Assert.IsFalse(...)` guarding a `Boolean`-returning call — that shape belongs to `use-assert-isfalse-not-asserterror-for-boolean-checks` instead, which wins for it. +- `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`. Exclude `asserterror Assert.IsTrue(...)` / `asserterror Assert.IsFalse(...)` guarding a `Boolean`-returning call — that shape belongs to `use-assert-isfalse-not-asserterror-for-boolean-checks` instead, which wins for it. Also exclude a trailing `asserterror Error(...)` used purely as an end-of-test rollback sentinel after a lazy `Initialize()` fixture already committed — that shape belongs to `commit-shared-test-fixture-inside-lazy-initialize`, which wins for it; the sentinel's own error text is not meant to be asserted against. - `asserterror` wraps `Assert.IsTrue(BooleanExpression, ...)` (or the `IsFalse` mirror) solely to invert the boolean result of the guarded call, rather than to assert that call itself raises an error — `use-assert-isfalse-not-asserterror-for-boolean-checks`. - A shared/lazy `Initialize()`-style fixture helper creates fixture data without a following `Commit()`, in a test method whose body later forces its own rollback (for example `asserterror Error(...)` used for end-of-test cleanup) — `commit-shared-test-fixture-inside-lazy-initialize`. The presence of `Commit()` after the fixture is the compliant shape, not the signal to look for; the missing-`Commit()` shape combined with a later deliberate rollback is the anti-pattern. Require runner/repository context for the `TestIsolation` value: a standalone test file cannot prove which runner executes it, and under `Function`-level isolation this whole pattern is moot regardless of `Commit()` — do not raise the finding when the executing runner's `TestIsolation` is known to be `Function`. - Changed code subscribes to `OnAfterRemoveTableRelation`, calls `RemoveTableRelation`, or references `Codeunit "Table Relation Test"`/134926 — `table-relation-test-exclude-known-invalid-relations-via-event`. From e3d9b8eb2514a1fdfce81ef95064cd30fbf424a3 Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Tue, 22 Sep 2026 14:35:48 +0200 Subject: [PATCH 5/6] Fix four merge-critical issues from Jesper's 2026-09-22 review - al-testing-review.md: the generic ExpectedError cue's asserterror Assert.IsTrue/IsFalse exclusion was unconditional, but the specialized rule it deferred to only claims the pure-inversion shape. A test expecting the guarded Boolean-returning call itself to raise fell through both routes. Narrowed the exclusion to the same inversion-only condition the specialized cue already uses. - asserterror-needs-expectederror-and-code.md: the rollback-sentinel exception (a trailing asserterror Error(...) used purely to force a fixture rollback, not to verify a specific failure) previously lived only in skill routing prose. Encoded it directly in the article's Anti Pattern section so every consumer of the knowledge base sees it, not just this one skill. - commit-shared-test-fixture-inside-lazy-initialize.good.al/.bad.al: replaced hand-rolled Item.Init()/Insert(true) with LibraryInventory.CreateItem, so the canonical fixture doesn't itself trigger use-library-codeunits-for-test-fixtures. - table-relation-test-exclude-known-invalid-relations-via-event.good.al/ .bad.al: declared minimal "Sample Setup"/"Sample Header" tables inline instead of referencing undefined symbols, matching this repo's own convention that every fixture is self-contained. --- ...sserterror-needs-expectederror-and-code.md | 2 ++ ...test-fixture-inside-lazy-initialize.bad.al | 5 ++-- ...est-fixture-inside-lazy-initialize.good.al | 5 ++-- ...e-known-invalid-relations-via-event.bad.al | 26 +++++++++++++++++ ...-known-invalid-relations-via-event.good.al | 29 +++++++++++++++++++ microsoft/skills/review/al-testing-review.md | 2 +- 6 files changed, 62 insertions(+), 7 deletions(-) diff --git a/microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md b/microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md index 4560a186..9ebf3f84 100644 --- a/microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md +++ b/microsoft/knowledge/testing/asserterror-needs-expectederror-and-code.md @@ -23,4 +23,6 @@ See sample: [`asserterror-needs-expectederror-and-code.good.al`](asserterror-nee `asserterror DoInvalid();` with nothing after it. The test asserts only that the call failed somehow; swap the validation for a different bug and the test still passes, certifying a guard that may no longer fire. A negative test that cannot tell one error from another verifies almost nothing. +Not an instance of this anti-pattern: a trailing `asserterror Error(SomeLabel)` used purely as an end-of-test rollback sentinel to undo a lazily-initialized shared fixture's scratch changes (see `commit-shared-test-fixture-inside-lazy-initialize.md`). That `Error` call exists to force a rollback, not to verify that a specific failure occurred — the sentinel's own text is not meant to be asserted against, and adding an `ExpectedError` there would just duplicate the label without checking anything the test doesn't already control. + See sample: [`asserterror-needs-expectederror-and-code.bad.al`](asserterror-needs-expectederror-and-code.bad.al). diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al index 2c13967e..f2af637f 100644 --- a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.bad.al @@ -3,6 +3,7 @@ codeunit 50142 "Sample Test Library" Subtype = Test; var + LibraryInventory: Codeunit "Library - Inventory"; Initialized: Boolean; SharedItemNo: Code[20]; RollBackMsg: Label 'Revert back the tables to their original state.'; @@ -22,9 +23,7 @@ codeunit 50142 "Sample Test Library" var Item: Record Item; begin - Item.Init(); - Item."No." := 'SAMPLE-SHARED'; - Item.Insert(true); + LibraryInventory.CreateItem(Item); SharedItemNo := Item."No."; end; diff --git a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al index 0879e6aa..c550697f 100644 --- a/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al +++ b/microsoft/knowledge/testing/commit-shared-test-fixture-inside-lazy-initialize.good.al @@ -3,6 +3,7 @@ codeunit 50142 "Sample Test Library" Subtype = Test; var + LibraryInventory: Codeunit "Library - Inventory"; Initialized: Boolean; SharedItemNo: Code[20]; RollBackMsg: Label 'Revert back the tables to their original state.'; @@ -21,9 +22,7 @@ codeunit 50142 "Sample Test Library" var Item: Record Item; begin - Item.Init(); - Item."No." := 'SAMPLE-SHARED'; - Item.Insert(true); + LibraryInventory.CreateItem(Item); SharedItemNo := Item."No."; end; diff --git a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al index 22fe8eda..1550350d 100644 --- a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al @@ -1,3 +1,29 @@ +table 50144 "Sample Setup" +{ + fields + { + field(1; "Primary Key"; Code[10]) { } + field(2; "Default Category Code"; Code[20]) { } + } + keys + { + key(PK; "Primary Key") { Clustered = true; } + } +} + +table 50145 "Sample Header" +{ + fields + { + field(1; "No."; Code[20]) { } + field(10; "Category Code"; Code[20]) { } + } + keys + { + key(PK; "No.") { Clustered = true; } + } +} + codeunit 50141 "Sample Table Relation Test Ext" { [EventSubscriber(ObjectType::Codeunit, Codeunit::"Table Relation Test", 'OnAfterRemoveTableRelation', '', false, false)] diff --git a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al index 8aa3ab8e..0a6d4807 100644 --- a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al @@ -1,3 +1,32 @@ +table 50144 "Sample Setup" +{ + fields + { + field(1; "Primary Key"; Code[10]) { } + field(2; "Default Category Code"; Code[20]) { } + } + keys + { + key(PK; "Primary Key") { Clustered = true; } + } +} + +table 50145 "Sample Header" +{ + fields + { + field(1; "No."; Code[20]) { } + // A known exception: this field is allowed to reference "Sample + // Setup" loosely (no TableRelation enforced here on purpose), so + // the standard Table Relation Test would otherwise reject it. + field(10; "Category Code"; Code[20]) { } + } + keys + { + key(PK; "No.") { Clustered = true; } + } +} + codeunit 50141 "Sample Table Relation Test Ext" { [EventSubscriber(ObjectType::Codeunit, Codeunit::"Table Relation Test", 'OnAfterRemoveTableRelation', '', false, false)] diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index eaf73af6..fdf5be0e 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -49,7 +49,7 @@ The following targeted checks cover every current `testing` article. Treat each - An `AutoCommit` test runs under a `Subtype = TestRunner` codeunit that omits `TestIsolation` or sets it to `Disabled`, leaving committed data between tests — `testisolation-belongs-on-the-test-runner`. Require runner/repository context; a standalone test file cannot prove which runner executes it. - 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`. Exclude `asserterror Assert.IsTrue(...)` / `asserterror Assert.IsFalse(...)` guarding a `Boolean`-returning call — that shape belongs to `use-assert-isfalse-not-asserterror-for-boolean-checks` instead, which wins for it. Also exclude a trailing `asserterror Error(...)` used purely as an end-of-test rollback sentinel after a lazy `Initialize()` fixture already committed — that shape belongs to `commit-shared-test-fixture-inside-lazy-initialize`, which wins for it; the sentinel's own error text is not meant to be asserted against. +- `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`. Exclude `asserterror Assert.IsTrue(...)` / `asserterror Assert.IsFalse(...)` only when it is used solely to invert the guarded call's Boolean result (the same condition `use-assert-isfalse-not-asserterror-for-boolean-checks` cues on below, which wins for that shape) — not when the test expects the guarded Boolean-returning call itself to raise an error, which this rule still owns even though it happens to wrap an `Assert.IsTrue`/`IsFalse` call. Also exclude a trailing `asserterror Error(...)` used purely as an end-of-test rollback sentinel after a lazy `Initialize()` fixture already committed — that shape belongs to `commit-shared-test-fixture-inside-lazy-initialize`, which wins for it; the sentinel's own error text is not meant to be asserted against. - `asserterror` wraps `Assert.IsTrue(BooleanExpression, ...)` (or the `IsFalse` mirror) solely to invert the boolean result of the guarded call, rather than to assert that call itself raises an error — `use-assert-isfalse-not-asserterror-for-boolean-checks`. - A shared/lazy `Initialize()`-style fixture helper creates fixture data without a following `Commit()`, in a test method whose body later forces its own rollback (for example `asserterror Error(...)` used for end-of-test cleanup) — `commit-shared-test-fixture-inside-lazy-initialize`. The presence of `Commit()` after the fixture is the compliant shape, not the signal to look for; the missing-`Commit()` shape combined with a later deliberate rollback is the anti-pattern. Require runner/repository context for the `TestIsolation` value: a standalone test file cannot prove which runner executes it, and under `Function`-level isolation this whole pattern is moot regardless of `Commit()` — do not raise the finding when the executing runner's `TestIsolation` is known to be `Function`. - Changed code subscribes to `OnAfterRemoveTableRelation`, calls `RemoveTableRelation`, or references `Codeunit "Table Relation Test"`/134926 — `table-relation-test-exclude-known-invalid-relations-via-event`. From 89f648928f4fc36491a5cb2119338a5cbb94298b Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Tue, 22 Sep 2026 20:59:56 +0200 Subject: [PATCH 6/6] Give the table-relation-test fixtures a real relation to exclude and one to protect The "Category Code" field had no TableRelation at all, so the good subscriber's RemoveTableRelation call targeted metadata that never existed - a no-op. Added a real TableRelation to "Sample Setup" on that field (the one known exception to exclude) and a second, ordinary self-referencing relation ("Parent No." -> "Sample Header"."No.") with no exception. The good fixture now removes only the first; the bad fixture's table-wide removal (field/related table/field all 0) now demonstrably also strips the second, showing the actual anti-pattern instead of removing nothing meaningful. --- ...e-known-invalid-relations-via-event.bad.al | 14 +++++++++++-- ...-known-invalid-relations-via-event.good.al | 20 +++++++++++++++---- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al index 1550350d..d95d9f2e 100644 --- a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.bad.al @@ -16,7 +16,14 @@ table 50145 "Sample Header" fields { field(1; "No."; Code[20]) { } - field(10; "Category Code"; Code[20]) { } + field(10; "Category Code"; Code[10]) + { + TableRelation = "Sample Setup"."Primary Key"; + } + field(11; "Parent No."; Code[20]) + { + TableRelation = "Sample Header"."No."; + } } keys { @@ -31,7 +38,10 @@ codeunit 50141 "Sample Table Relation Test Ext" var TableRelationTest: Codeunit "Table Relation Test"; begin - // Removes every relation on the whole table, not just the one known exception + // Removes every relation on the whole table (field/related table/ + // related field all 0), not just the one known exception - this + // also strips "Parent No." -> "Sample Header"."No.", which had no + // exception and should have stayed covered by the standard test. TableRelationTest.RemoveTableRelation(TableRelationsMetadata, Database::"Sample Header", 0, 0, 0); end; } diff --git a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al index 0a6d4807..26db161a 100644 --- a/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al +++ b/microsoft/knowledge/testing/table-relation-test-exclude-known-invalid-relations-via-event.good.al @@ -16,10 +16,20 @@ table 50145 "Sample Header" fields { field(1; "No."; Code[20]) { } - // A known exception: this field is allowed to reference "Sample - // Setup" loosely (no TableRelation enforced here on purpose), so - // the standard Table Relation Test would otherwise reject it. - field(10; "Category Code"; Code[20]) { } + // A known exception: "Category Code" predates "Sample Setup" and + // can carry a value that no longer resolves to a real row there, + // so the standard Table Relation Test would otherwise reject it - + // excluded via OnAfterRemoveTableRelation below. + field(10; "Category Code"; Code[10]) + { + TableRelation = "Sample Setup"."Primary Key"; + } + // An ordinary relation with no exception - ExcludeSampleFieldFrom + // TableRelationTest below must leave this one checked. + field(11; "Parent No."; Code[20]) + { + TableRelation = "Sample Header"."No."; + } } keys { @@ -34,6 +44,8 @@ codeunit 50141 "Sample Table Relation Test Ext" var TableRelationTest: Codeunit "Table Relation Test"; begin + // Removes only the one known exception. "Parent No." -> "Sample + // Header"."No." is untouched and stays covered by the standard test. TableRelationTest.RemoveTableRelation(TableRelationsMetadata, Database::"Sample Header", 10, Database::"Sample Setup", 1); end; }