4 AL/BC testing patterns from an external BC testing expert's blog (Luc van Vugt, fluxxus.nl) - #159
Conversation
Fourth batch from CURABIS ApS, mined from an external BC/NAV testing expert's blog archive (fluxxus.nl). Confirm+StrSubstNo interaction with ConfirmHandler, Table Relation Test's OnAfterRemoveTableRelation exclusion hook (verified against BCApps source, codeunit 134926), committing shared lazy-Initialize fixture data, and Assert.IsFalse vs asserterror for boolean checks.
There was a problem hiding this comment.
Thanks for this — the mining quality is high. Verifying the Table Relation Test pattern against BCApps source instead of taking the blog's word for it is exactly the right instinct, and rejecting two candidates (one for no longer existing in current source) is the kind of restraint this corpus needs.
I re-verified all four claims independently. All four are factually correct.
| Claim | Verified against |
|---|---|
ConfirmHandler receives the raw template |
Reproduced in the source blog; corroborated by microsoft/ALAppExtensions#23935 |
| Table Relation Test exclusion hook | codeunit 134926, exact path; OnAfterRemoveTableRelation is an [IntegrationEvent] and RemoveTableRelation is public with the exact 5-arg signature |
Commit() in a lazy Initialize() |
778 BCApps test files use this exact pattern |
Assert.IsFalse over asserterror |
IsFalse exists in both Assert and Library Assert (130002) |
The table-relation article is the strongest of the four: the pattern only works because RemoveTableRelation is public rather than local, and the bad sample's claim that passing 0 widens the delete is correct — RemoveTableRelation skips SetRange for any zero argument. Samples are independently written rather than copied, and each good/bad pair differs in exactly one thing.
Validation is green locally: frontmatter 0 errors, knowledge index deterministic at 304 articles, 34 review fixtures across 17 domains.
Requesting changes on the following.
1. CLA (blocking)
license/cla is still pending. Please sign it — but note this isn't purely procedural here. The CLA's Originality of Work clause asks that contributions derived from a third party be accompanied by the phrase "Submission containing materials of a third party:" followed by the third-party name and any known licenses. Since this batch is explicitly derived from a named author's blog, that clause is genuinely engaged rather than boilerplate. Please include it when you sign.
2. commit-shared-test-fixture-inside-lazy-initialize.md contradicts existing guidance
This is the one substantive issue. The existing microsoft/knowledge/testing/transactionmodel-attribute-governs-test-transactions.md states that applying AutoRollback to a test whose code calls Commit "produces a runtime error on the first Commit... the test does not complete." Your new article instructs the reader to call Commit() inside Initialize() under exactly that default.
Both files are domain: testing, bc-version: [all], countries: [w1], application-area: [all] — identical applicability with incompatible normative guidance, which is precisely the shape READ's conflict detection is meant to catch. It also has a live consequence: al-testing-review.md already tells agents to flag code that "calls Commit under AutoRollback", so as things stand an agent would flag the canonical BCApps fixture pattern.
The evidence says your article is right and the existing one overstates. src/Layers/W1/Tests/ERM/ERMOnlineMappingSetup.Codeunit.al commits inside Initialize(), declares no TransactionModel attribute at all, and passes — along with 777 other test files.
Since this PR is what surfaces the conflict, please correct the overstatement in transactionmodel-attribute-governs-test-transactions.md as part of this change, so the two articles agree.
3. confirm-needs-strsubstno... presents a platform bug as designed behavior
The article explains that substitution happens "only for the dialog a real user sees," which reads as intended platform behavior. It isn't — it's a reported defect (microsoft/ALAppExtensions#23935), raised by Luc van Vugt himself, with an internal bug filed at the time by Nikola Kukrika (@nikolakukrika).
That issue was closed as COMPLETED in February 2024, but the thread contains no fix confirmation and I found no public release-note evidence either way, so the current status is genuinely unclear. With bc-version: [all] and a Best Practice that tells agents to restructure production code, a silent platform fix would turn this into agents rewriting correct code for no reason.
Please reframe it as a known platform defect with the issue linked. Nikola Kukrika (@nikolakukrika) — since you filed the internal bug and own this folder, could you confirm its disposition before we ship this as unbounded guidance?
Worth adding while you're in there: per the source blog, Message/MessageHandler substitutes correctly and only Confirm is affected. That asymmetry is both good evidence it's a bug and useful protection against an agent over-generalizing the rule to MessageHandler.
4. Minor — table-relation-test-exclude-known-invalid-relations-via-event.md
- Codeunit 134926 ships in BCApps' test app, so only consumers depending on the BC test libraries can subscribe. Worth stating explicitly.
- "walks every
TableRelationfield property in the app" is slightly off — it readsTable Relations Metadatafiltered to1..1999999999, i.e. tenant-wide across installed apps, not just the current app. - No token in
al-testing-review.mdcoversTableRelationorOnAfterRemoveTableRelation, so this article may never surface in review. The other three are already reachable viaConfirmHandler,asserterror,Commit, andLibrary Assert. Happy to add the tokens separately if you'd rather keep this PR scoped to knowledge.
Note that /microsoft/knowledge/testing/ is owned by Nikola Kukrika (@nikolakukrika), ventselartur and Bugsy (@pchriste-microsoft-com), so this needs one of them to approve regardless of my review. Only flag and guard have run so far — the frontmatter and index workflows still need to go green in CI once a maintainer approves the run.
Nice work overall. Item 1 is quick; 2 and 3 are the ones that need real attention.
|
@microsoft-github-policy-service agree [company="CURABIS ApS"] Submission containing materials of a third party: Luc van Vugt (fluxxus.nl) - blog content, no explicit license found. |
|
Point 1: the fast one... done... Point 2: Verified directly in ERMOnlineMappingSetup.Codeunit.al (codeunit 134915). No TransactionModel attribute at all, Commit() inside Initialize(), and manual cleanup via asserterror Error(...) at the end rather than relying on automatic rollback. Point 3: Checked the issue directly. No fix confirmation visible in the thread either, just closed as completed with no comment. Will reframe as a known, unconfirmed platform defect with the issue linked, and add the Message/MessageHandler asymmetry as supporting evidence per your note. Point 4: Fair, will note the test-app dependency explicitly, correct the "every TableRelation field property" wording to the actual tenant-wide Table Relations Metadata scope, and add the missing al-testing-review.md tokens in this PR rather than leaving it unreachable. |
|
@microsoft-github-policy-service agree company="CURABIS ApS" Submission containing materials of a third party: Luc van Vugt (fluxxus.nl) - blog content, no explicit license found. |
- 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>
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Follow-up after 1e22b51: CLA/disclosure now pass, and the Table Relation dependency and tenant-wide metadata wording are improved. The remaining transaction, applicability, fixture, and routing issues are inline.
Please also preserve durable provenance in the knowledge files themselves: each externally inspired article should identify its specific source and what was derived from it. A PR-level disclosure is useful for CLA review but will not accompany the rule when an agent consumes it later.
|
|
||
| ## Description | ||
|
|
||
| A test codeunit that creates master/setup data once, guarded by an `IsInitialized` flag, to avoid repeating expensive setup across many `[Test]` methods depends on that data surviving into every later test. Each `[Test]` method runs under `AutoRollback` by default, so data inserted during the first test's call to `Initialize()` rolls back at the end of that test. `IsInitialized` is a variable, not persisted data, so it still reads `true` on the next test — but the fixture rows it points to are already gone. |
There was a problem hiding this comment.
The core transaction explanation is incorrect. Microsoft's TransactionModel reference identifies AutoCommit as the default, and current BCApps tests call Commit() without an explicit attribute. Under AutoCommit, the method commits at its boundary without this added call; under AutoRollback, Commit() is rejected; under function-level TestIsolation, the fixture is rolled back between methods; and under codeunit-level isolation, the outer codeunit transaction already preserves it between methods. The explicit Commit() pattern in BCApps often establishes a rollback baseline inside a test, not this universal shared-fixture rule. Please remove or fundamentally rewrite this article around a demonstrated transaction/isolation configuration.
There was a problem hiding this comment.
Fixed in 0b08c2f — the article was rewritten around the same correction you made independently in your retraction: AutoCommit is the documented default TransactionModel, not AutoRollback. The rewritten article now explains the actual mechanism — Commit() inside Initialize() protects the fixture from the test method's own later deliberate asserterror-based rollback (the BCApps ERMOnlineMappingSetup pattern), not from an AutoRollback default that doesn't exist for an attribute-less method — and makes the fixture's survival across test methods explicitly dependent on TestIsolation (Disabled/Codeunit survive across methods, Function does not).
There was a problem hiding this comment.
Thanks—the rewrite correctly establishes AutoCommit as the default and narrows the explicit Commit() to the deliberate rollback-sentinel pattern. Three connected issues remain:
TestIsolation = Disabledis still listed alongsideCodeunitas a supported recommendation. UnderDisabled, the early commit is never rolled back at codeunit completion and permanently contaminates the database. The recommended configuration should beCodeunit; mentionDisabledonly with an explicit verified final teardown.- The good fixture's
asserterror Error(RollBackMsg)is still selected by the genericasserterror-needs-expectederror-and-coderule, so the new canonical pattern violates another active rule. Please add explicit precedence/exclusion for this deterministic rollback sentinel. - The fixtures create no persisted fixture data, contain only one test, and show no runner/isolation context, so they cannot demonstrate either the cross-method failure or the proposed fix. Please make the sample executable with a later test and explicit Codeunit isolation context.
With those changes, the transaction model and the agent guidance should align.
|
Correction to my own review. The transaction analysis in my first round was wrong, and I would rather retract it explicitly than leave it standing next to a follow-up that contradicts it. What I got wrongIn the first round I wrote that "the evidence says your article is right and the existing one overstates", and asked you to correct The fact
Worth noting for anyone following this thread: the TransactionModel attribute page — the one you naturally reach for — never names a default at all. That is probably how this got past both of us. Why my evidence never supported my conclusion
I counted 778 files sharing the pattern and treated that as confirming the article's stated mechanism. Prevalence of a practice is not evidence for the reason given for it — and that distinction is most of what this corpus is for. The giveaway was in the file I quoted. It ends with a deliberate What this means for the PR
One more from my first roundI wrote that the ConfirmHandler issue was "raised by Luc van Vugt himself". microsoft/ALAppExtensions#23935 was opened by Matjaž Šega; Luc blogged it and credited Malcolm Gray. My follow-up corrected that inline, but the error originated in the round that presented a table asserting I had independently verified all four claims and that all four were factually correct. That table was overconfident. One of the four has since been ruled incorrect, and one of its supporting attributions was wrong. I would rather say so plainly, since "verified against source" is the bar this repository asks contributors to meet. UnaffectedNothing here touches the other three articles, and the table-relation facts I checked in the first round all hold: codeunit 134926, Sorry for the churn. This one was my error, not yours. |
- 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>
|
Appreciate the correction — no worries on the churn. 0b08c2f reflects it: the |
|
Jesper Schulz-Wedde (@JesperSchulz) Addressed this round in 0b08c2f (pushed 2026-09-08): the 6 inline issues above, the transaction-model contradiction from your retraction (both articles now state and agree on the |
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Thank you for the substantial rewrite. The Confirm rule removal, TransactionModel correction, OnPrem scope, Boolean-rule precedence, routing, and per-article attribution address most of the previous feedback.
The remaining transaction concerns are tracked in the existing conversation, so I am not opening duplicate threads for them. One additional factual qualification is needed below because it changes which table relations the article tells agents to reject. We recognize the extra round is inconvenient; this is limited to behavior that materially affects the rule's correctness. The branch also currently conflicts with main in the TransactionModel article and testing review skill and will need to be updated before merge.
|
|
||
| ## Description | ||
|
|
||
| Codeunit 134926 "Table Relation Test" (shipped in BCApps' test app — only consumers that depend on the BC test libraries can subscribe to it) reads Table Relations Metadata tenant-wide across every installed app, not just the current one, and fails the moment a related field's type or length doesn't match what the relation requires — the related field must match the largest related field's length, and its type must match (except a field may relate to both `Code` and `Text`, which resolves to `Text`). A field with a legitimate, intentional relation shape has no per-field override in its own object definition; the check runs with no built-in escape hatch. The validation test method itself is `[Scope('OnPrem')]`: it only runs from an on-premises test surface, not from a cloud-targeted test app, so this whole exception mechanism — and the check it works around — is only reachable where that test can actually execute. |
There was a problem hiding this comment.
The length rule needs one qualification. Current Table Relation Test requires exact length when the metadata contains an unconditional relation. For conditional-only relations, it permits the source field to be longer than the largest related field and fails only when the source is shorter. Please document those two cases separately; otherwise agents will recommend exclusions or schema changes for conditional relations that the standard test accepts.
Summary
Fourth batch from CURABIS ApS, this time mined from a source outside CURABIS itself: Luc van Vugt's fluxxus.nl, a long-running blog by a BC/NAV developer who authored a book on automated testing and ATDD in Business Central. The blog runs 2009–2023 and is mostly legacy classic-client/C-AL content; these 4 are the patterns that survived a full read-through, are still current in modern AL, and cleared BCQuality's admission test.
Confirm()+StrSubstNo: a[ConfirmHandler]'sQuestionparameter receives the raw, unsubstituted template whenConfirmis called with its own placeholder-substitution overload — the production code must build the string withStrSubstNofirst for a handler-side assertion to see the real message.OnAfterRemoveTableRelationexclusion hook — verified directly against BCApps source (codeunit 134926 "Table Relation Test",src/Layers/W1/Tests/Misc/TableRelationTest.Codeunit.al) rather than taken on the blog's word alone.Initialize()fixture data needs an explicitCommit(), or it rolls back with the first test that creates it and silently vanishes for every test after.Assert.IsFalsefor a boolean check, notasserterrorwrapped aroundAssert.IsTrue—asserterrorverifies that an error was raised, not the value under test.Two other candidates found during mining were rejected: a
TestPermissions::InheritFromTestCodeunitenum-value restriction (too thin/overlapping the existing permission-tests rule) and a suite-wide fixture-injection event pattern from 2018/2020 NAV content that no longer exists anywhere in current BCApps source (verified absent, not proposed).Test plan
🤖 Generated with Claude Code