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.
This commit is contained in:
Michael Dieringer 2026-09-21 22:49:39 +02:00
parent 1392521a8e
commit 69b09db3a6
6 changed files with 100 additions and 16 deletions

View file

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