mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 06:36:55 +01:00
Refine self-improvement review guidance
Narrow IsHandled, label-scope, UI-handler, checkpoint, and bulk-operation guidance to evidence-backed false-positive boundaries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
wenjiefan
parent
841b4e7cab
commit
5f1cff2fb6
21 changed files with 99 additions and 155 deletions
|
|
@ -6,14 +6,12 @@ codeunit 50241 "IsHandled Init Bad Sample"
|
|||
DiscountPct: Decimal;
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
// IsHandled is never initialized before the first raise, so flow depends
|
||||
// on the variable's default rather than an explicit, documented intent.
|
||||
OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled);
|
||||
if not IsHandled then
|
||||
DiscountPct := 5;
|
||||
|
||||
// Bug: IsHandled is not reset. If the first subscriber set it true, the
|
||||
// payment-discount default below is silently skipped too.
|
||||
// Bug: execution continues when the first event set IsHandled to true,
|
||||
// and that stale value is passed to a different publisher.
|
||||
OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled);
|
||||
if not IsHandled then
|
||||
DiscountPct += 2;
|
||||
|
|
|
|||
|
|
@ -6,17 +6,17 @@ codeunit 50240 "IsHandled Init Good Sample"
|
|||
DiscountPct: Decimal;
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
IsHandled := false;
|
||||
// A freshly declared local Boolean is false.
|
||||
OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled);
|
||||
if not IsHandled then
|
||||
DiscountPct := 5;
|
||||
if IsHandled then
|
||||
exit;
|
||||
DiscountPct := 5;
|
||||
|
||||
// Reset before reusing the same variable for the next event so a
|
||||
// subscriber that handled the first raise can't suppress this one.
|
||||
IsHandled := false;
|
||||
// Reaching this point proves that IsHandled is still false.
|
||||
OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled);
|
||||
if not IsHandled then
|
||||
DiscountPct += 2;
|
||||
if IsHandled then
|
||||
exit;
|
||||
DiscountPct += 2;
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
|
|
|
|||
|
|
@ -7,20 +7,20 @@ countries: [w1]
|
|||
application-area: [all]
|
||||
---
|
||||
|
||||
# Initialize IsHandled to false before publishing
|
||||
# Reset IsHandled before publishing only when its value can carry over
|
||||
|
||||
## Description
|
||||
|
||||
A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable in by reference, so its incoming value decides whether the default logic is skipped. A freshly declared Boolean starts as `false`, but the same variable is frequently reused to raise several events in one routine, and after the first raise it may already be `true`. Assigning `IsHandled := false;` on the line immediately before every raise makes the control flow deterministic and self-documenting, and prevents a stale `true` from silently suppressing logic the author never meant to make skippable. Generated code often reuses one `IsHandled` across several raises without resetting it.
|
||||
A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable by reference, so a pre-existing `true` can affect the following control flow. AL [automatically initializes Boolean variables to `false`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-al-variables#initialization), so a freshly declared local Boolean passed to one event is already deterministic. The same is true when control flow proves the variable is `false`; for example, reaching a second raise after `if IsHandled then exit;` proves that the first raise did not leave it `true`.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Set `IsHandled := false;` immediately before each `OnBeforeX(…, IsHandled)` raise, then guard the default logic with `if IsHandled then exit;` or `if not IsHandled then …`. Do this even when the variable was just declared: the explicit reset documents intent and stays correct if a second event raise is added to the routine later. This applies only to events that carry a `var IsHandled: Boolean`; an `OnBefore` event with no `IsHandled` parameter needs no reset.
|
||||
Reset `IsHandled := false;` before a raise only when the value might otherwise carry over as `true`: the same variable is reused after an earlier raise without a control-flow proof that it is false, the value comes from an input parameter, field, or global, or earlier code seeds it. A reset on a guaranteed-false fresh local can be retained for readability, but its absence is not a correctness finding.
|
||||
|
||||
See sample: `initialize-ishandled-to-false-before-publishing.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Raising `OnBeforeX(…, IsHandled)` with a variable whose value carries over from an earlier raise, so a subscriber that handled the first event unintentionally suppresses the second routine's default logic. Detection: an `IsHandled` variable passed to more than one event in a routine without an intervening `IsHandled := false;`, or any `OnBefore…` raise that passes an `IsHandled` variable without an intervening `IsHandled := false;`.
|
||||
Raising `OnBeforeX(…, IsHandled)` when the variable can still be `true` from an earlier raise or another source, so the new publisher call starts with stale state. Do not match a single raise using a fresh local Boolean, or a later raise reached only after `if IsHandled then exit;`.
|
||||
|
||||
See sample: `initialize-ishandled-to-false-before-publishing.bad.al`.
|
||||
|
|
|
|||
|
|
@ -16,11 +16,18 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
|
|||
{
|
||||
procedure NormalizeCustomerNames()
|
||||
var
|
||||
NormalizeState: Record "Perf Normalize State";
|
||||
LastCustomerNo: Code[20];
|
||||
begin
|
||||
// The outer loop owns checkpoints; the per-row loop contains no Commit.
|
||||
while NormalizeNextChunk(LastCustomerNo) do
|
||||
NormalizeState.Get('CUSTOMER');
|
||||
LastCustomerNo := NormalizeState."Last Customer No.";
|
||||
|
||||
while NormalizeNextChunk(LastCustomerNo) do begin
|
||||
// Persist progress in the same transaction as the completed chunk.
|
||||
NormalizeState."Last Customer No." := LastCustomerNo;
|
||||
NormalizeState.Modify();
|
||||
Commit();
|
||||
end;
|
||||
end;
|
||||
|
||||
local procedure NormalizeNextChunk(var LastCustomerNo: Code[20]): Boolean
|
||||
|
|
@ -58,3 +65,17 @@ codeunit 50128 "Perf Sample CommitInLoop Good"
|
|||
exit(true);
|
||||
end;
|
||||
}
|
||||
|
||||
table 50128 "Perf Normalize State"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; Code; Code[10]) { }
|
||||
field(2; "Last Customer No."; Code[20]) { }
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; Code) { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -13,16 +13,18 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
Commit ends the current write transaction. Calling it inside a per-row loop produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with the platform's ability to batch write operations. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). When the batch is too large for one transaction, the fix is not a per-row Commit but bounded checkpoints that select an exact list of at most N keys and process only those rows.
|
||||
Commit ends the current write transaction. Calling it inside a per-row loop usually produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with batching. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`).
|
||||
|
||||
A durability checkpoint inside an outer batch loop can be valid only when the same transaction persists a progress marker or state that makes retries strictly exclude completed work, the checkpoint follows a complete business unit, and errors propagate instead of being swallowed. Restart safety and bounded retrieval are separate requirements: a persisted watermark can make retries safe, but an outer `FindSet` over the full remaining tail with periodic commits still retrieves the complete set because [`FindSet` is not implemented as `TOP X`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#get-find-findset-and-next).
|
||||
|
||||
## Best Practice
|
||||
|
||||
If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. `FindSet` is optimized for reading the complete filtered set and isn't implemented as `TOP X`, so calling it over the remaining tail and breaking after N rows does not bound retrieval. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Commit after the bounded inner loop returns and persist its last selected key as the next watermark. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`.
|
||||
If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Persist the last selected key in the same transaction as the completed chunk, then commit after the bounded helper returns. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. Let errors escape so failed work is not recorded as complete. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`.
|
||||
|
||||
See sample: `avoid-commit-inside-loops.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Placing Commit inside `repeat ... until Next() = 0` is almost always a mistake: it is unusual for the correctness of the operation to depend on per-row commits, and the cost of starting a new transaction on every row dominates the work. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint.
|
||||
Placing Commit inside `repeat ... until Next() = 0` without persisted progress is almost always a mistake: retries re-enter already committed work, while the cost of starting a transaction on every row dominates the operation. A progress variable held only in memory is not restart-safe. A full-tail `FindSet` with a commit every N rows is not bounded retrieval, even if a persisted watermark makes it restart-safe. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint.
|
||||
|
||||
See sample: `avoid-commit-inside-loops.bad.al`.
|
||||
|
|
|
|||
|
|
@ -15,12 +15,12 @@ application-area: [all]
|
|||
|
||||
## Best Practice
|
||||
|
||||
Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table-extension triggers, event subscribers, global triggers, or media fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`).
|
||||
Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). A visible loop for progress UX is acceptable only when evidence shows the equivalent bulk call already executes as individual operations and the loop preserves trigger and business semantics.
|
||||
|
||||
See sample: `prefer-modifyall-over-per-row-modify.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior.
|
||||
A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects or bulk fallback condition. A progress dialog alone does not exempt this loop. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior.
|
||||
|
||||
See sample: `prefer-modifyall-over-per-row-modify.bad.al`.
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [modifyall, deleteall, regression, triggers, media, getglobaltabletriggermask, subscriber]
|
||||
keywords: [modifyall, deleteall, regression, triggers, media, security-filtering, companion-fields, subscriber, progress]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
|
|
@ -11,12 +11,12 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
`ModifyAll` and `DeleteAll` usually execute as single SQL statements, but the platform falls back to a fetch-then-row-by-row loop under specific conditions. Per the upstream guidance, the regression is triggered by any of: global database triggers defined via `GetGlobalTableTriggerMask` or `GetDatabaseTableTriggerSetup` (so that `OnDatabaseDelete`/`OnGlobalDelete` must run); event subscribers on the table's `OnBeforeDelete`/`OnAfterDelete` (for `DeleteAll`) or `OnBeforeModify`/`OnAfterModify` (for `ModifyAll`); or "adding a Media or MediaSet table field to either the table or table extension." Each of these forces the platform to materialize each affected row in AL.
|
||||
`ModifyAll` and `DeleteAll` can limit SQL calls, but Microsoft documents that they [revert to individual calls](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#modifyall-and-deleteall) when the table has trigger code, related modify/delete/global/database event subscribers, active security filtering, `Media` or `MediaSet` fields, or fields added through companion tables. These conditions must be assessed from the target table and runtime context, not only from the visible bulk call.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Before introducing any of the above on a table — a global trigger registration, a `Modify`/`Delete` subscriber, a media or media-set field — note every `ModifyAll`/`DeleteAll` that targets the table and assess whether the regression cost is acceptable. The upstream guidance is explicit: "There should be a very good reason for doing any of the above since they will significantly regress performance of `ModifyAll` and/or `DeleteAll`." Once a table has regressed, multiple `ModifyAll` calls each iterate the rows themselves, so consolidating to one explicit `FindSet`+`Modify` loop becomes faster than chaining several `ModifyAll` calls.
|
||||
Before introducing a fallback condition, audit the `ModifyAll`/`DeleteAll` call sites that target the table and assess the regression cost. Once a bulk path already executes row by row, one explicit loop can be reasonable when it preserves the same trigger semantics and adds required per-row progress UX; consolidating several regressed bulk calls into one pass can also avoid repeated iteration. This is a narrow equivalence check, not a generic progress-dialog exemption: when no fallback condition applies, retain the bulk API.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Adding a media field to a hot table — or subscribing to its modify/delete events from a generic logging codeunit — without auditing the bulk-write call sites. The schema change is mechanical; the performance change is invisible at the call site and only surfaces when a previously fast `ModifyAll` starts paying the per-row trigger cost in production. The mirror anti-pattern is chaining several `ModifyAll` calls on a table that has already regressed; each one re-iterates the same rows.
|
||||
Adding a fallback condition to a hot table without auditing bulk-write call sites, or replacing a working bulk API with a per-row loop solely to show progress. The mirror anti-pattern is chaining several bulk calls on a table that already falls back, causing repeated row-by-row passes.
|
||||
|
|
|
|||
|
|
@ -1,11 +0,0 @@
|
|||
codeunit 50262 "Sample Label Scope Bad"
|
||||
{
|
||||
procedure LookupCustomer(CustomerNo: Code[20])
|
||||
var
|
||||
Customer: Record Customer;
|
||||
GreetingMsg: Label 'Hello %1', Comment = '%1 = Customer Name';
|
||||
begin
|
||||
if Customer.Get(CustomerNo) then
|
||||
Message(GreetingMsg, Customer.Name);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,13 +0,0 @@
|
|||
codeunit 50263 "Sample Label Scope Good"
|
||||
{
|
||||
var
|
||||
GreetingMsg: Label 'Hello %1', Comment = '%1 = Customer Name';
|
||||
|
||||
procedure LookupCustomer(CustomerNo: Code[20])
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
if Customer.Get(CustomerNo) then
|
||||
Message(GreetingMsg, Customer.Name);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,30 +1,18 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: style
|
||||
keywords: [label, scope, procedure, translation, localization, xliff]
|
||||
keywords: [label, scope, procedure, translation, localization, xliff, false-positive]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Declare Labels at object scope, not inside procedure `var` blocks
|
||||
# Procedure-local Labels are valid
|
||||
|
||||
## Description
|
||||
|
||||
`Label` is the AL declaration that participates in the translation pipeline: the build extracts every Label declared in an object into the `.xlf` file shipped to translators, and the runtime substitutes the localized value when the object is loaded. Translation tooling discovers Labels by walking the object's top-level declarations.
|
||||
|
||||
Labels declared inside a procedure-local `var` block are still **compiled** as Label values, but their participation in localization is fragile: depending on the BC version, the build pipeline, and the translation toolchain in use, procedure-local Labels may be missed during XLIFF extraction, may be re-emitted with auto-generated keys that change between builds, or may not be addressable by reviewers triaging translations. The reliable, supported pattern is to declare every Label in the object's top-level `var` block.
|
||||
|
||||
The same rule applies to all object types that own behavior: codeunits, pages, tables, reports, queries, and their extensions. For shared messages used by multiple objects, declare the Label in the most appropriate owning object and reference it — do not duplicate the literal across procedure-scoped declarations in several places.
|
||||
The AL language supports `Label` variables at both object and procedure scope. Microsoft documents the [Label data type](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-using-labels#label-data-type) without imposing an object-scope requirement, and the translation pipeline generates an XLF file containing [all labels used by the extension](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-work-with-translation-files#generating-the-xliff-file). There is no documented correctness or localization defect caused solely by declaring a Label in a procedure-local `var` block.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Move every `Label` to the object's top-level `var` block. Use the appropriate suffix (`Msg`, `Err`, `Qst`, `Lbl`, `Tok`, `Txt`) on the variable name so reviewers and the translation team can see at a glance what role the string plays. Pair non-translatable strings (URLs, JSON/XML fragments, integration tokens) with `Locked = true`, as covered by `label-locked-for-non-translatable.md`.
|
||||
|
||||
See sample: `labels-declared-at-object-scope.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Declaring `Label` inside a procedure-local `var` block — `procedure Lookup() var GreetingMsg: Label 'Hello %1';` — couples the translatable string to one procedure, hides it from object-level review, and depends on a translation pipeline behavior that is not part of the AL language contract.
|
||||
|
||||
See sample: `labels-declared-at-object-scope.bad.al`.
|
||||
Choose object scope when a Label is reused or when an established repository convention prefers central declarations; choose procedure scope when the Label belongs to one procedure. Do not report a correctness or localization finding solely because a Label is local. An explicit object-scope convention is at most a low-severity maintainability preference. This guidance applies equally to production and test apps: test code still needs localization where its strings are user-facing or translator-facing.
|
||||
|
|
|
|||
|
|
@ -1,43 +1,28 @@
|
|||
codeunit 50401 "Test UI Handlers Bad"
|
||||
codeunit 50401 "Test UI Handler Proof Bad"
|
||||
{
|
||||
Subtype = Test;
|
||||
|
||||
// Several wiring mistakes, each of which fails at runtime rather than as a
|
||||
// clean assertion the reviewer can read:
|
||||
// * A UI call with no listed handler -> "unhandled UI" abort (the Message
|
||||
// below has no handler).
|
||||
// * The mirror mistake, listing a handler the path never hits, instead
|
||||
// fails with "handler function was not executed".
|
||||
// * A handler that hardcodes its answer and asserts inline, with no
|
||||
// enqueue/dequeue -> nothing proves the RIGHT dialog fired the RIGHT
|
||||
// number of times, and a failed inline assert can be swallowed by the
|
||||
// calling UI operation.
|
||||
[Test]
|
||||
[HandlerFunctions('ConfirmHandler')]
|
||||
procedure PostDocumentConfirmsAndMessages()
|
||||
[HandlerFunctions('CustomerCardHandler')]
|
||||
procedure CustomerCardActionSucceeds()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
// No Initialize(): a value leaked by an earlier test corrupts this one.
|
||||
RunPostingThatConfirmsAndMessages();
|
||||
// No AssertEmpty(): a missing or extra dialog goes unnoticed.
|
||||
Customer.Get('10000');
|
||||
ActionSucceeded := true;
|
||||
|
||||
Page.RunModal(Page::"Customer Card", Customer);
|
||||
|
||||
// This only proves a value assigned before the action stayed true.
|
||||
Assert.IsTrue(ActionSucceeded, 'The customer card action failed.');
|
||||
end;
|
||||
|
||||
local procedure RunPostingThatConfirmsAndMessages()
|
||||
[ModalPageHandler]
|
||||
procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card")
|
||||
begin
|
||||
// Raises a Confirm AND a Message, but only ConfirmHandler is listed:
|
||||
// the Message has nothing to intercept it -> unhandled-UI runtime abort.
|
||||
if Confirm('Post this document?', false) then
|
||||
Message('Posting completed.');
|
||||
end;
|
||||
|
||||
[ConfirmHandler]
|
||||
procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean)
|
||||
begin
|
||||
// Hardcoded expectation and hardcoded reply. If the wrong dialog fires,
|
||||
// this inline assert may never surface as the test's verdict.
|
||||
Assert.AreEqual('Post this document?', Question, 'Wrong confirm.');
|
||||
Reply := true;
|
||||
end;
|
||||
|
||||
var
|
||||
Assert: Codeunit "Library Assert";
|
||||
ActionSucceeded: Boolean;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,57 +1,28 @@
|
|||
codeunit 50400 "Test UI Handlers Good"
|
||||
codeunit 50400 "Test UI Handler Capture Good"
|
||||
{
|
||||
Subtype = Test;
|
||||
|
||||
[Test]
|
||||
[HandlerFunctions('ConfirmHandler,PostMessageHandler')]
|
||||
procedure PostDocumentConfirmsAndMessages()
|
||||
[HandlerFunctions('CustomerCardHandler')]
|
||||
procedure CustomerCardShowsSelectedCustomer()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Initialize();
|
||||
Customer.Get('10000');
|
||||
CapturedCustomerNo := '';
|
||||
|
||||
// [GIVEN] the test enqueues, in interaction order, what each handler
|
||||
// will see and how it should answer: the Confirm's expected
|
||||
// question plus the reply to return, then the expected Message.
|
||||
LibraryVariableStorage.Enqueue('Post this document?'); // expected question (substring)
|
||||
LibraryVariableStorage.Enqueue(true); // reply ConfirmHandler returns
|
||||
LibraryVariableStorage.Enqueue('Posting completed.'); // expected message (substring)
|
||||
Page.RunModal(Page::"Customer Card", Customer);
|
||||
|
||||
// [WHEN] the code under test raises the Confirm and then the Message
|
||||
RunPostingThatConfirmsAndMessages();
|
||||
|
||||
// [THEN] every enqueued expectation was consumed exactly once
|
||||
LibraryVariableStorage.AssertEmpty();
|
||||
Assert.AreEqual(Customer."No.", CapturedCustomerNo, 'The customer card opened for the wrong customer.');
|
||||
end;
|
||||
|
||||
local procedure Initialize()
|
||||
[ModalPageHandler]
|
||||
procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card")
|
||||
begin
|
||||
// Clear leftover values so a value leaked by an earlier test cannot
|
||||
// cascade into this one.
|
||||
LibraryVariableStorage.Clear();
|
||||
end;
|
||||
|
||||
local procedure RunPostingThatConfirmsAndMessages()
|
||||
begin
|
||||
// Stands in for the production routine that confirms, then messages.
|
||||
if Confirm('Post this document?', false) then
|
||||
Message('Posting completed.');
|
||||
end;
|
||||
|
||||
[ConfirmHandler]
|
||||
procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean)
|
||||
begin
|
||||
// Verify the RIGHT dialog fired (substring match), then return the
|
||||
// reply the test enqueued for it.
|
||||
Assert.ExpectedConfirm(LibraryVariableStorage.DequeueText(), Question);
|
||||
Reply := LibraryVariableStorage.DequeueBoolean();
|
||||
end;
|
||||
|
||||
[MessageHandler]
|
||||
procedure PostMessageHandler(Message: Text[1024])
|
||||
begin
|
||||
Assert.ExpectedMessage(LibraryVariableStorage.DequeueText(), Message);
|
||||
CapturedCustomerNo := CustomerCard."No.".Value();
|
||||
end;
|
||||
|
||||
var
|
||||
Assert: Codeunit "Library Assert";
|
||||
LibraryVariableStorage: Codeunit "Library - Variable Storage";
|
||||
CapturedCustomerNo: Code[20];
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,28 +1,28 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: testing
|
||||
keywords: [handler, handlerfunctions, confirm, message, strmenu, variable-storage, enqueue, unhandled-ui]
|
||||
keywords: [handler, handlerfunctions, confirm, message, strmenu, variable-storage, enqueue, capture, runmodal, unhandled-ui]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Wire and verify UI handlers with enqueue-driven expectations
|
||||
# Wire UI handlers and verify meaningful outcomes
|
||||
|
||||
## Description
|
||||
|
||||
A test runs headless: there is no interactive user to answer a dialog. Every UI call the executed path raises — `Confirm`, `Message`, error dialogs, `Page.Run`/`RunModal`, `Report.Run`/`RunModal`, request pages, `StrMenu`, `Notification.Send` — must be intercepted by a handler carrying the matching attribute (`[ConfirmHandler]`, `[MessageHandler]`, `[StrMenuHandler]`, `[ModalPageHandler]`, …) and named in the method's `[HandlerFunctions(...)]`. The list is a two-sided contract: raise a UI call with no listed handler and the platform aborts with an *unhandled UI* error; list a handler the path never hits and it fails with *"handler function was not executed"*. Both are runtime failures — the test never reaches its verdict, so a reviewer sees an infrastructure error instead of a result on the behavior under test.
|
||||
A test runs headless, so every UI call on the executed path must be intercepted by a matching handler named in `[HandlerFunctions(...)]`. The list is a two-sided contract: an unhandled UI call aborts the test, while Microsoft documents that [every listed handler must execute at least once](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/attributes/devenv-handlerfunctions-attribute#remarks) or the test fails.
|
||||
|
||||
Getting the handler *present* is only half the job; the handler must also verify the *right* dialog fired the *right* number of times. Do that by driving handlers from the test, not by hardcoding answers inside them.
|
||||
Beyond that wiring guarantee, the test must verify the behavior it cares about. The appropriate pattern depends on the contract: a handler can capture concrete page state or a result and the test can assert that semantic postcondition after `RunModal`; assertions inside a handler are also supported. Queue/enqueue/dequeue and `LibraryVariableStorage.AssertEmpty` are useful when interaction order, count, text, replies, or a scripted sequence is itself part of the contract, but they are not mandatory for every handler.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Make the test own the expectations and the handlers consume them. Before acting, the test `Enqueue`s — in interaction order — the expected text (a stable substring) and any reply each handler must return. The handler `Dequeue`s the expected text, verifies it with the purpose-built asserts (`Assert.ExpectedMessage`, `Assert.ExpectedConfirm`, `Assert.ExpectedStrMenu` — which match on a fragment, not the full localized caption), then `Dequeue`s and returns its reply. Finish the test body with `LibraryVariableStorage.AssertEmpty` to prove every enqueued interaction fired exactly once, and start each test with an `Initialize` that calls `LibraryVariableStorage.Clear` so a value leaked by an earlier test cannot cascade. List in `[HandlerFunctions]` precisely the handlers the scenario triggers — no superset "just in case", no subset that happens to work today.
|
||||
List precisely the handlers the scenario triggers and make each handler contribute meaningful evidence. For a single modal page, reset a capture variable before the action, capture a concrete value from the page in the handler, and assert the expected value after `RunModal`. For ordered or repeated interactions, let the test enqueue expectations, let handlers dequeue and verify them, clear storage during initialization, and finish with `AssertEmpty`.
|
||||
|
||||
See sample: `ui-handlers-in-tests.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Omitting a handler for a UI call the path raises (unhandled-UI abort), padding the list with a handler the path never reaches ("handler function was not executed"), or writing handlers that hardcode their answer and assert inline with no enqueue/dequeue. The last is the subtle one: nothing proves the correct dialog fired the expected number of times, and an inline assertion that fails inside a handler can be swallowed by the calling UI operation, leaving the suite green while the behavior is broken. Skipping `Initialize`/`AssertEmpty` hides both a leaked queue and a missing or extra dialog.
|
||||
Omitting a handler for a UI call, listing a handler the path never reaches, or claiming action success from a Boolean set before the action runs. A handler that only closes a page can also leave the test without a semantic assertion. Do not flag the absence of queue storage by itself; require it only when the test needs to prove interaction order, count, text, replies, or a scripted sequence.
|
||||
|
||||
See sample: `ui-handlers-in-tests.bad.al`.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue