mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
830eaff68f
* Add 4 more AL/BC testing patterns from Luc van Vugt's fluxxus.nl blog 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. * 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> * 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> * 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. * Fix four merge-critical issues from Jesper's 2026-09-22 review - al-testing-review.md: the generic ExpectedError cue's asserterror Assert.IsTrue/IsFalse exclusion was unconditional, but the specialized rule it deferred to only claims the pure-inversion shape. A test expecting the guarded Boolean-returning call itself to raise fell through both routes. Narrowed the exclusion to the same inversion-only condition the specialized cue already uses. - asserterror-needs-expectederror-and-code.md: the rollback-sentinel exception (a trailing asserterror Error(...) used purely to force a fixture rollback, not to verify a specific failure) previously lived only in skill routing prose. Encoded it directly in the article's Anti Pattern section so every consumer of the knowledge base sees it, not just this one skill. - commit-shared-test-fixture-inside-lazy-initialize.good.al/.bad.al: replaced hand-rolled Item.Init()/Insert(true) with LibraryInventory.CreateItem, so the canonical fixture doesn't itself trigger use-library-codeunits-for-test-fixtures. - table-relation-test-exclude-known-invalid-relations-via-event.good.al/ .bad.al: declared minimal "Sample Setup"/"Sample Header" tables inline instead of referencing undefined symbols, matching this repo's own convention that every fixture is self-contained. * Give the table-relation-test fixtures a real relation to exclude and one to protect The "Category Code" field had no TableRelation at all, so the good subscriber's RemoveTableRelation call targeted metadata that never existed - a no-op. Added a real TableRelation to "Sample Setup" on that field (the one known exception to exclude) and a second, ordinary self-referencing relation ("Parent No." -> "Sample Header"."No.") with no exception. The good fixture now removes only the first; the bad fixture's table-wide removal (field/related table/field all 0) now demonstrably also strips the second, showing the actual anti-pattern instead of removing nothing meaningful. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
28 lines
2.8 KiB
Markdown
28 lines
2.8 KiB
Markdown
---
|
|
bc-version: [all]
|
|
domain: testing
|
|
keywords: [asserterror, expectederror, expectederrorcode, negative-test, error-code]
|
|
technologies: [al]
|
|
countries: [w1]
|
|
application-area: [all]
|
|
---
|
|
|
|
# Pin asserterror to a specific error with ExpectedError and ExpectedErrorCode
|
|
|
|
## Description
|
|
|
|
`asserterror` passes when the guarded statement raises any error at all. That is too permissive for a negative test: a typo, a missing setup record, or a permission failure all raise errors, so a bare `asserterror` can go green while never exercising the rule it claims to verify — false confidence that the validation works. Constrain it. `Assert.ExpectedError(text)` checks the message of the error that was actually raised, and `Assert.ExpectedErrorCode(code)` checks its error code. Together they assert that the specific failure occurred, turning "something went wrong" into "the right thing went wrong for the right reason".
|
|
|
|
## Best Practice
|
|
|
|
Follow every `asserterror` with a verification of the error it expects, and prefer the reusable `Library Assert` helpers over hardcoded literals. For a mandatory-field check, `Assert.ExpectedTestFieldError(FieldCaption, ExpectedValue)` encapsulates both the message and the `TestField` code, so the test survives caption or code changes and does not repeat that knowledge in every method. For other errors, pair `Assert.ExpectedError` with a stable substring — ideally a shared `Label`, not an inline sentence — and, where known, `Assert.ExpectedErrorCode`. When a needed check is missing from the shared library, extend `Library Assert` (or your own assert library) with a helper rather than hardcoding message text and codes across tests; matching on a code or an invariant fragment keeps the test from going blind to the wrong error when a caption is localized.
|
|
|
|
See sample: [`asserterror-needs-expectederror-and-code.good.al`](asserterror-needs-expectederror-and-code.good.al).
|
|
|
|
## Anti Pattern
|
|
|
|
`asserterror DoInvalid();` with nothing after it. The test asserts only that the call failed somehow; swap the validation for a different bug and the test still passes, certifying a guard that may no longer fire. A negative test that cannot tell one error from another verifies almost nothing.
|
|
|
|
Not an instance of this anti-pattern: a trailing `asserterror Error(SomeLabel)` used purely as an end-of-test rollback sentinel to undo a lazily-initialized shared fixture's scratch changes (see `commit-shared-test-fixture-inside-lazy-initialize.md`). That `Error` call exists to force a rollback, not to verify that a specific failure occurred — the sentinel's own text is not meant to be asserted against, and adding an `ExpectedError` there would just duplicate the label without checking anything the test doesn't already control.
|
|
|
|
See sample: [`asserterror-needs-expectederror-and-code.bad.al`](asserterror-needs-expectederror-and-code.bad.al).
|