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 <noreply@anthropic.com>
This commit is contained in:
Michael Dieringer 2026-09-08 19:02:03 +02:00
parent dcd79afc08
commit 1392521a8e
10 changed files with 65 additions and 61 deletions

View file

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