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] 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 4560a18..9ebf3f8 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 2c13967..f2af637 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 0879e6a..c550697 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 22fe8ed..1550350 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 8aa3ab8..0a6d480 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 eaf73af..fdf5be0 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`.