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.
This commit is contained in:
Michael Dieringer 2026-09-22 14:35:48 +02:00
parent 69b09db3a6
commit e3d9b8eb25
6 changed files with 62 additions and 7 deletions

View file

@ -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`.