Address Jesper Schulz-Wedde's review on PR #159

- 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 <noreply@anthropic.com>
This commit is contained in:
Michael Dieringer 2026-09-07 21:06:58 +02:00
parent 12b7f73d24
commit dcd79afc08
4 changed files with 12 additions and 5 deletions

View file

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