Skip to content

4 AL/BC testing patterns from an external BC testing expert's blog (Luc van Vugt, fluxxus.nl) - #159

Open
Michael Dieringer (MichaelDieringer) wants to merge 3 commits into
microsoft:mainfrom
Curabis:community-contribution/vanvugt-blog-patterns
Open

4 AL/BC testing patterns from an external BC testing expert's blog (Luc van Vugt, fluxxus.nl)#159
Michael Dieringer (MichaelDieringer) wants to merge 3 commits into
microsoft:mainfrom
Curabis:community-contribution/vanvugt-blog-patterns

Conversation

@MichaelDieringer

Copy link
Copy Markdown

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]'s Question parameter receives the raw, unsubstituted template when Confirm is called with its own placeholder-substitution overload — the production code must build the string with StrSubstNo first for a handler-side assertion to see the real message.
  • Table Relation Test's OnAfterRemoveTableRelation exclusion 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.
  • Shared/lazy Initialize() fixture data needs an explicit Commit(), or it rolls back with the first test that creates it and silently vanishes for every test after.
  • Assert.IsFalse for a boolean check, not asserterror wrapped around Assert.IsTrueasserterror verifies that an error was raised, not the value under test.

Two other candidates found during mining were rejected: a TestPermissions::InheritFromTestCodeunit enum-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

  • CI frontmatter/format validation passes
  • Domain-owning reviewers confirm placement and accuracy per file

🤖 Generated with Claude Code

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.

@JesperSchulz Jesper Schulz-Wedde (JesperSchulz) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 TableRelation field property in the app" is slightly off — it reads Table Relations Metadata filtered to 1..1999999999, i.e. tenant-wide across installed apps, not just the current app.
  • No token in al-testing-review.md covers TableRelation or OnAfterRemoveTableRelation, so this article may never surface in review. The other three are already reachable via ConfirmHandler, asserterror, Commit, and Library 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.

@MichaelDieringer

Copy link
Copy Markdown
Author

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

@MichaelDieringer

Copy link
Copy Markdown
Author

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.
That's the actual mechanism: the "Commit causes an error" behavior is specific to the explicit AutoRollback attribute, not the undeclared default.
We'll correct transactionmodel-attribute-governs-test-transactions.md's "Best Practice: Default to AutoRollback" wording so it stops conflating "no attribute" with "AutoRollback enforced," since that's what would make an agent flag this exact canonical pattern.

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.

@MichaelDieringer

Michael Dieringer (MichaelDieringer) commented Sep 7, 2026

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Reference: https://github.com/MicrosoftDocs/dynamics365smb-devitpro/blob/main/properties/properties/devenv-transactionmodel-property.md

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks—the rewrite correctly establishes AutoCommit as the default and narrows the explicit Commit() to the deliberate rollback-sentinel pattern. Three connected issues remain:

  1. TestIsolation = Disabled is still listed alongside Codeunit as a supported recommendation. Under Disabled, the early commit is never rolled back at codeunit completion and permanently contaminates the database. The recommended configuration should be Codeunit; mention Disabled only with an explicit verified final teardown.
  2. The good fixture's asserterror Error(RollBackMsg) is still selected by the generic asserterror-needs-expectederror-and-code rule, so the new canonical pattern violates another active rule. Please add explicit precedence/exclusion for this deterministic rollback sentinel.
  3. 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.

Comment thread microsoft/skills/review/al-testing-review.md Outdated
@JesperSchulz

Copy link
Copy Markdown
Contributor

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 wrong

In the first round I wrote that "the evidence says your article is right and the existing one overstates", and asked you to correct transactionmodel-attribute-governs-test-transactions.md as part of this PR. That was backwards. You should not have been asked to make that edit.

The fact

AutoCommit is the documented default transaction model for a test method — TransactionModel Property: "AutoCommit is the default value."

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

codeunit 134915 "ERM Online Mapping Setup" declares no TransactionModel attribute, so it runs under AutoCommit. It is not an AutoRollback test. The existing article's claim is explicitly scoped to applying AutoRollback, so an attribute-less codeunit cannot show that it overstates.

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 asserterror Error(RollBackMessage), which only makes sense if the Commit() is establishing a rollback baseline — the reading in my follow-up — rather than protecting a shared fixture from a per-test rollback that never happens.

What this means for the PR

  1. Please treat my first-round request to edit transactionmodel-attribute-governs-test-transactions.md as withdrawn. Your edit is not wrong as far as it goes — absence of the attribute genuinely is not equivalent to AutoRollback — but it was made on a bad instruction from me, it dropped the article's default recommendation, and it still never says what an attribute-less method actually does.
  2. As it stands this PR ships two domain: testing articles that disagree. The new one says test methods run "under AutoRollback by default"; the edited one says absence is not AutoRollback; neither names AutoCommit. Whatever survives the rewrite, please make both agree and state the default explicitly.

One more from my first round

I 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.

Unaffected

Nothing here touches the other three articles, and the table-relation facts I checked in the first round all hold: codeunit 134926, RemoveTableRelation public with the five-argument signature, zero arguments widening the delete, and the tenant-wide 1..1999999999 filter.

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

Copy link
Copy Markdown
Author

Appreciate the correction — no worries on the churn. 0b08c2f reflects it: the transactionmodel-attribute-governs-test-transactions.md edit from round 1 is superseded, both testing articles now state the AutoCommit default explicitly and agree with each other, and neither treats an attribute-less method as equivalent to AutoRollback. Also resolved the Matjaž Šega/Malcolm Gray attribution issue by deleting the article that had it (see the thread above) — the underlying platform bug is reported closed and I couldn't reproduce it on a current version, so there's no longer an unconditional workaround making that claim.

@MichaelDieringer

Copy link
Copy Markdown
Author

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 AutoCommit default), and the in-file Source provenance you asked for — each surviving externally-inspired article now names its specific fluxxus.nl post and states what was independently verified vs. taken from it, not just disclosed at the PR level. Ready for another look whenever you have time.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants