[API tests] Complete missing VAT posting fixtures - #11224
Prangshuman Das (t-prda) wants to merge 5 commits into
Conversation
GitHub Actions
Inactive
github-actions
Bot
added
AL: Apps (W1)
|
github-actions
Bot
added
Build: Automation
github-actions
Bot
removed
Build: Automation
|
Issue #11561 is not valid. Please make sure you link an issue that exists, is open and is approved. |
Good Sense Reviewer - Round 1Recommendation: AcceptWhat this PR doesThis change creates missing VAT posting-group combinations before API test fixtures create document lines, while retaining combinations that already exist. The guard checks the exact key, runs before line creation or API calls, and is applied consistently across the affected sales, purchase, item-variant, API v1, and API v2 scenarios. The corresponding test exclusions are removed. Problem-solution fitFit: Strong The changes directly address fixture failures caused by missing VAT posting setup. The scope covers the reported failing methods without changing production behavior or unrelated exclusions. SuggestionsNone. Risk assessment and necessityRisk: The change is limited to test setup and disabled-test metadata. Existing VAT setup is preserved, sales and purchase patterns are aligned, and no production event publisher, subscriber, or BaseApp event timing is changed. Current-head checks have no failures or pending jobs. Necessity: The setup guard is needed so these API tests do not depend on country-specific pre-existing VAT combinations. The change is narrowly scoped to the affected fixtures and tests.
|
## Purpose [AB#646383](https://dynamicssmb2.visualstudio.com/Dynamics%20SMB/_workitems/edit/646383) Add opt-in authentication to `Library - Graph Mgt.` without API re-enablement or pipeline changes. ## Design - Default `None` preserves existing behavior. The public enum/interface and public `API Test Auth Context` retain external provider extensibility. - The Microsoft provider stays `Internal`; context `Apply` stays internal. Internal visibility is API encapsulation, not authorization. - Credential precedence remains container file, then existing Key Vault only if the file is absent. Invalid configured credentials fail; successful passwords remain instance-scoped `SecretText`. - File credentials now use a private file-mapping provider directly, without replacing or clearing the session-wide Azure Key Vault provider/cache. No extra AL plaintext copy is introduced. - `OnAfterInitializeWebRequestWithURL` remains last. The pre-existing empty Graph `OnRun` is restored. The trust boundary is admitted OnPrem test code and environment credential access, not `Internal` visibility or a destination-URL restriction. ## Tests and scope All six AL contracts remain: default None, event ordering, provider reuse, instance scoping, same-provider reselection and deselection. The dedicated recorder is removed. The non-SingleInstance internal mock publishes the **test-only** `OnAfterConfigureAuthentication` event; the same manually bound test-codeunit instance owns `Library - Variable Storage` for both recording and verification. Initialization clears that instance's queue after prior failures; each contract drains it and calls `AssertEmpty`. The six source-pattern PowerShell checks are removed, not replaced by other AL-text assertions. The real 401/200/401 HTTP scenario is owned by uptake microsoft#11860 after credential provisioning; core has no dependency on that future workflow. This is not a claim of a complete provider-branch matrix. No caller migration, URL repair, credential provisioning, scheduling, exclusion or work-date rollout belongs to this core. Native stack #11893: **microsoft#10085 auth core -> microsoft#11862 URL/fixture prerequisites -> microsoft#11891 workflow infrastructure -> microsoft#11860 AL uptake -> microsoft#11224–microsoft#11230 -> microsoft#11322 -> microsoft#11451–microsoft#11454**. ## Current checkpoint Head `4a76bb71851c158083dbe4d22e84bf40a0577842`, tree `12739039e502a3feeea9e98694fb360ea959c606`. Merged main baseline remains `0a602a2481c93ebfe7b1d27d6016fb9926c42111` (permission cleanup from PR 11561). No additional main merge or code changes were made during finalization. Old `b195c7a`/`4eef8ac` tree-equality claims remain obsolete after the intentional baseline/auth changes. [Verified AL compile/publish/test run 36131334134](https://github.com/microsoft/BCApps/actions/runs/36131334134) **succeeded, attempt 2**, at exact validation head `4a76bb71851c158083dbe4d22e84bf40a0577842`. **113/113 test jobs and 113/113 cleanup steps succeeded.** Core has no credential-file provisioning/removal step (expected). W1 artifacts verify all **6 auth contracts**. Local Pester at the applicable checkpoint: **28 passed, zero failed/skipped**. Core local Pester: **28 passed**; no unsupported claim about a separate root PowerShell workflow. ## Validation limits For uptake/full, CU139496 `MicrosoftAuthenticationRespectsServerAuthMode` is verified in the actual UserPassword fixture (**401/200/401**). The Windows **200/200/200** expectations are implemented but **Windows runtime remains unverified**; no foreign local NST was used. Successful GitHub runs do not establish universal native NAV coverage. Excluded PDF cases, country-specific absent/excluded cases and tolerated-native distinctions are not claimed passing. No PR has been merged or auto-merged; validation drafts remain Do Not Merge outside stack #11893. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// <summary>Creates VAT posting setup when the specified posting-group combination does not exist.</summary> | ||
| /// <param name="VATBusPostingGroup">VAT business posting group.</param> | ||
| /// <param name="VATProdPostingGroup">VAT product posting group.</param> | ||
| procedure EnsureVATPostingSetupExists(VATBusPostingGroup: Code[20]; VATProdPostingGroup: Code[20]) |
There was a problem hiding this comment.
The new EnsureVATPostingSetupExists helper has no access modifier or Scope attribute, so it becomes a public API available beyond the test-library boundary even though it is an implementation helper. Restrict it to the existing OnPrem-scoped test-library surface, or otherwise use the narrowest access that supports its callers.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
[Scope('OnPrem')]
procedure EnsureVATPostingSetupExists(VATBusPostingGroup: Code[20]; VATProdPostingGroup: Code[20])Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.46.6
|
This newly enabled negative test uses asserterror without checking the expected error message or code, so an unrelated setup or permission error can make the test pass without proving that deletion of an in-use item variant is rejected. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.47.6 |
1 similar comment
|
This newly enabled negative test uses asserterror without checking the expected error message or code, so an unrelated setup or permission error can make the test pass without proving that deletion of an in-use item variant is rejected. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.47.6 |
Good Sense Reviewer - Round 2Recommendation: AcceptWhat this PR doesThe branch reconciliation since round 1 does not change the net PR patch. The guarded VAT posting setup still creates only missing combinations before the affected API test fixtures create lines, and the related tests are re-enabled consistently. Status of previous suggestionsNone. New observations (commits since round 1)None - the new commits only propagate parent updates. The exact net patch matches round 1, and no new PR-owned non-merge span remains after incremental attribution. Risk assessment and necessityRisk: The change remains limited to test fixtures and disabled-test metadata. It changes no production API, event, report layout, or BaseApp behavior. Fresh exact-head runtime checks are still pending, but the completed checks show no failure. Necessity: The setup guard remains needed to make the re-enabled API tests independent of country-specific pre-existing VAT combinations. The scope remains targeted.
|
Good Sense Reviewer - Round 3Recommendation: AcceptWhat this PR doesThe reviewed fixture patch is unchanged since round 2; the new branch commits only propagate parent updates. The guarded setup still creates only missing VAT posting-group combinations before the affected API test fixtures create or change document lines, retains existing setup, and re-enables the matching tests. Status of previous suggestionsNone. New observations (commits since round 2)None - the reviewed file contents are unchanged. After excluding the current base branch, there are no new PR-owned non-merge spans to assess. Risk assessment and necessityRisk: The change remains limited to test fixtures and disabled-test metadata. It changes no production API, event, report layout, or BaseApp behavior. Fresh exact-head validation is still incomplete; the current failures are infrastructure-related or outside the changed tests. Necessity: The setup guard is needed so the re-enabled API tests do not depend on country-specific pre-existing VAT combinations. The scope remains limited to the affected fixtures and exclusions.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session:3952f078-a881-4da8-ad96-13b727e48a91
157037a to
c13bb37
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session:3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session:3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session:3952f078-a881-4da8-ad96-13b727e48a91
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session:3952f078-a881-4da8-ad96-13b727e48a91
|
Issue #11224 is not valid. Please make sure you link an issue that exists, is open and is approved. |
| begin | ||
| if (VATBusPostingGroup = '') and (VATProdPostingGroup = '') then | ||
| exit; | ||
| if VATPostingSetup.Get(VATBusPostingGroup, VATProdPostingGroup) then |
There was a problem hiding this comment.
This helper must continue to create the VAT posting setup when the combination is missing, but the bare Record.Get raises a runtime error instead of returning false. Consume the Boolean result with an existence check so the CreateVATPostingSetup call can run when no setup exists.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
if not VATPostingSetup.Get(VATBusPostingGroup, VATProdPostingGroup) then
LibraryERM.CreateVATPostingSetup(VATPostingSetup, VATBusPostingGroup, VATProdPostingGroup);Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.48.6
Good Sense Reviewer - Round 4Recommendation: AcceptWhat this PR doesThe guarded test setup creates only missing VAT posting-group combinations before the affected API test fixtures create or change document lines. Existing setup is retained, and the matching tests are re-enabled. Status of previous suggestionsNone. New observations (commits since round 3)None - the force-pushed branch has the same reviewed net patch. The previous and current three-dot diffs have the same stable patch ID, and the contents of all ten reviewed files are unchanged. Risk assessment and necessityRisk: The change remains limited to test fixtures and disabled-test metadata. It changes no production API, event, report layout, or BaseApp behavior. Current-head validation is still running; the completed failure is the unrelated issue-link check. Necessity: The setup guard is needed so the re-enabled API tests do not depend on country-specific pre-existing VAT combinations. The scope remains limited to the affected fixtures and exclusions.
|
…cture (microsoft#11891) ## Scope [AB#646383](https://dynamicssmb2.visualstudio.com/Dynamics%20SMB/_workitems/edit/646383) Workflow-only prerequisite above microsoft#11862 and below microsoft#11860: 33 `.github`/`build` files including the committed project finalizer wrappers. - Atomic protected credential-file materialization and cleanup, with cleared temporary buffers and consumers stopped before deletion. - Secondary-tenant Disabled-isolation discovery/restoration and clean-codeunit worker resets. - Clean-codeunit execution is opt-in via `enableCleanTestCodeunitExecution`. This infrastructure stage leaves it absent/off, preserving ordinary typed/Legacy execution and the existing Unit Disabled pass, including warmup and retries. Uptake microsoft#11860 activates it alongside AL authentication adoption. - Existing Legacy lanes, selectors, result aggregation and retry policy remain; Task Scheduler stays limited to the existing Uncategorized profile. - Behavioral PowerShell/ACL coverage of credential lifetime, discovery, scheduling, retries and result merging. All AL source and exclusions equal the integrated main baseline `bb7111877ff786951b86a1a0f80d8b39b8f5dacd`, including merged auth/prerequisites. No provider adoption or API re-enablement belongs here. The old Expense helper remains until uptake. The six relocated source-pattern auth checks are removed here too; the three additional uptake AL source-inspection assertions were also removed. No replacement auth source-pattern checks are added. The previous workflow-only NZ Integration run newly dispatched API Disabled-isolation codeunits before authentication adoption: CU139700 has 22 failures in its JUnit, while unchanged-core NZ has no CU139700 suite and no failures. These are real runtime regressions, not infra failures or tolerated-native evidence. The default-off gate restores the stage boundary; enabled uptake/full behavior remains equivalent to the previous combined implementation. The earlier workflow run succeeded; that evidence is historical after this baseline refresh. Native stack #11893: **microsoft#10085 auth core -> microsoft#11862 URL/fixture prerequisites -> microsoft#11891 workflow infrastructure -> microsoft#11860 AL uptake -> microsoft#11224–microsoft#11230 -> microsoft#11322 -> microsoft#11451–microsoft#11454**. Main-targeted draft microsoft#11892 stays outside grouping. ## Validation limits Historical uptake/full runs verified CU139496 `MicrosoftAuthenticationRespectsServerAuthMode` in the actual UserPassword fixture (**401/200/401**). The Windows **200/200/200** expectations are implemented but **Windows runtime remains unverified**; no foreign local NST was used. Successful GitHub runs do not establish universal native NAV coverage. Excluded PDF cases, country-specific absent/excluded cases and tolerated-native distinctions are not claimed passing. No additional PR was merged or auto-merged during this refresh; validation drafts remain Do Not Merge outside stack #11893. ## Historical targeted country evidence Final workflow NZ Integration artifact at `9269e3df582704881a898e19029308e9296cbbf1` verifies **2,751 test cases, zero failures, and CU139700 absent**. This confirms the default-off gate no longer prematurely runs the API Items codeunit that previously had 22 failures. [Workflow run36157073629](https://github.com/microsoft/BCApps/actions/runs/36157073629) succeeded with all113 test jobs. ## Current checkpoint Head `69df41756d918a516a3c868ca66f45cbfd320428`; captured main `eaddd6b3c518414f372154ea6cedd6c12b287359`. **Validation BLOCKED; no whole-run or all-country green claim.** Current exact-head AL runs: [workflow](https://github.com/microsoft/BCApps/actions/runs/37215976231), [workflow validation](https://github.com/microsoft/BCApps/actions/runs/37215975121), [uptake](https://github.com/microsoft/BCApps/actions/runs/37215975884), [uptake validation](https://github.com/microsoft/BCApps/actions/runs/37215973922), [full](https://github.com/microsoft/BCApps/actions/runs/37215975278). Remaining jobs are not cancelled. Platform `30.0.55429.0` reproducibly throws `AcquireSqlConnectionFromPool` NullReference on the first API GET after tenant reset: uptake IS Integration job111493435528 (`CapabilitiesProjectsEnabledViaAPI`) and full MX Default job111493443618 (`TestGetCurrencyExchangeRates`). Both occur56–64 seconds after tenant3 reset; later independent requests pass, but the failed methods do not recover. No scheduler reset/worker overlap was found. Async runtime/pool lifecycle coupling remains possible, unproven—not infrastructure-only. Exact implementation is unavailable in the exposed NAV tree; runtime-owner source/PDB analysis of pool lifetime across dismount/copy/remount is required. Separately, uptake-validation BE job111494278233 fails ordinary SLS installation with a duplicate datasearch sequence before clean execution. Runner/network outages are separate failures. No AL retries, arbitrary waits or classifier broadening will mask these failures. **Verified partial evidence:** direct uptake CU148343 passes **153/198 cases across17 countries**, including all9 methods and the repaired policy snapshot method. CA/CZ/ES/NL/NO artifacts were missing at2026-10-04T19:58Z; absence is not failure or success. Original8 methods, all assertions, four response clears and five query-safe URLs are preserved alongside upstream's ninth method. Local Pester remains117/117 at workflow/uptake/full. All4 exact-head PowerShell runs passed; workflow-validation37215974877 required attempt2 after one analyzer-tool crash; the other3 passed attempt1. Uptake PowerShell provenance is validation37215973669 at the same uptake SHA, not a separate direct-branch run. The supported finalizer ran after normal teardown in direct workflow BE Default job111495104917 at2026-10-04T19:30:13.2943924Z and direct uptake DE Integration job111493433096 at2026-10-04T19:37:30.6075547Z. These markers prove hook invocation, not explicit deletion when teardown already removed the file. The23 committed wrappers/shared finalizer and absent generator remain unchanged. **Accepted limitation:** explicit cleanup is success-only; failed/cancelled runs rely on normal container teardown, with no hard-runner-loss guarantee. Credential ACL protections remain; per-run disposable CI credentials limit risk, not eliminate it. No CU139496 runtime evidence yet; Legacy1 remains unverified and W1 Default does not cover it. Windows authentication, excluded CompanyInfo (`TestGetCompanyAndEnvironmentDescriptions`, PR11741), Travel, PDF/native coverage remain unverified/excluded as applicable. Existing review fixes/resolutions, native stack #11893, merged prefix, approval/queue/draft states and upstream exclusions remain unchanged. Permission cleanup from PR11561 is retained. No new source change, baseline update, push, merge or AL retry accompanies this checkpoint. Validation drafts remain Do Not Merge. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
This branch was successfully deployed
2 participants
Fixes AB#653121
Scope
Create only missing VAT posting-group combinations for item, item-charge, resource and G/L fixtures. Retain existing setup when present. This includes the shared fixture helper and seven API test callers.
Related umbrella646383.
Evidence and limits
The saved BCApps all-country results demonstrate 12 distinct failing methods in seven codeunits (46 country/method occurrences in each of result sets 2 and 4). This is not a universal NAV failure: the reviewed NAV uptake artifact passed CU139739.TestDeleteInUse; its other 11 methods were absent.
Only this layer's original fix/exclusion patch is reviewed here (10 paths); the patch is unchanged by Expense-first restructuring.
Exact head:
ef78428c5eeb3035af36aa4fe137f5222cb66a6e; tree:c85159c983db0a407a0e33649592274f66d07d80.Expense-first sequencing using existing exclusions
The same 193 method-specific temporary exclusions (135 APIV1 +58 APIV2 across12 codeunits) now live in the existing
src/DisabledTests/_Exclude_APIV1__Tests/_Exclude_APIV1__Tests.DisabledTest.jsonandsrc/DisabledTests/_Exclude_APIV2__Tests/_Exclude_APIV2__Tests.DisabledTest.json. No separate sequencing files remain. Original entries retain their order/style/content; no new duplicate or wildcard/codeunit-wide exclusion is introduced. Unrelated pre-existing APIV2 duplicates are preserved rather than mixed with this change.#11860 removes the temporary additions from these same existing manifests alongside its already-reviewed authentication uptake. All193 temporary stage entries are removed in #11860, but it makes 192/193 target methods eligible: the independent
139739::TestDeleteInUseexclusion remains until the VAT fixture fix PR #11224 removes it. From #11224 onward all193 are eligible; eligibility is not runtime success. All general/downstream cumulative trees are exactly byte-identical to the preceding checkpoint; the full checkpoint has none of these193 methods excluded. No app-name allowlist, selection setting, runner/authentication/test/production change or Logiq exclusion is added.Why these methods started executing: they already required Disabled isolation and were IntegrationTest codeunits. Ordinary typed selection in TestSuiteMgt332–352 selects None|Codeunit; the old extra Disabled pass in RunTestsInBcContainer was UnitTest-only. The clean-execution switch newly reaches Disabled IntegrationTest codeunits and still honors the existing JSON exclusions. These193 methods were not listed in those exclusions. This is a pre-existing selection gap, not a newly introduced product auth bug or proof they never ran in any historical configuration. A complete67-codeunit audit preserves existing UnitTest/Legacy behavior and the ten independently handled Logiq integration tests.
**Expense scope is unchanged:**51 API reenables, nine non-API exclusions, baseline six cases and all consolidated regressions. The two legacy Spend Requests methods remain conditional on not CLEAN30. Focused #12325 review:21 files, including the two existing exclusion manifests. General #11860 review:159 files.
The official NAV
Disable-NAVALTesthelper was inspected: it has no destination/app-file parameter, writes NAV's App/DisabledTests using per-codeunit filenames and sorts entries. It cannot safely preserve these BCApps files. A bounded JSON relocation preserved original prefixes/order and checked exact identities, then exercised the unchanged real loader. No new shared helper/framework or NAV selector change was made.Validation and presentation evidence
Pester117/117 passed independently at the new Expense/general/full heads. Actual existing-loader checks confirm identical effective193-method selection after relocation and preserved51/9 Expense scope. All nine downstream Git tree hashes remain exactly unchanged. Fresh exact-head CI is pending; old-head successes are not substituted for current runtime evidence.
Historical run37330893173 at head88881be reached193 general methods:171 genuine401 failures and22 nominal passes (17 bare-ASSERTERROR cases can accept the wrong error; five local fixtures). Separately, all51 Expense methods passed in13 inspected countries (663 results), and all63 touched API methods yielded819 results; Activity coverage was117/198 at that checkpoint. These are bounded historical results, not a full/current matrix pass. The preceding stage-fix runs hit50 hosted-runner acquisition cancellations; other jobs were active, so this was not a claimed global outage. New pushes schedule fresh CI, not an AL retry. The general SQL-pool NRE and missing warning-reference artifacts remain separate unresolved limitations.
Durable session presentation notes: api-test-enablement-presentation-notes.md, with source-pinned selection proof, fix/coverage inventory, helper limitations and historical-vs-current evidence boundaries. Fresh runtime proof must still establish the51 Expense cases and actual193-case suppression at Expense stage.
Related umbrella646383. Native stack #12327 remains #12325 → #11860 → #11224 → #11225 → #11226 → #11227 → #11228 → #11229 → #11230 → #11322. Protected merged #10085/#11862/#11891 and validation #11892 are untouched; validation-only #11861/#11455 remain Do Not Merge. No new main integration, native-group/base change, PR merge, queue operation, Actions cancellation or manual retry was made. All prior heads remain ancestors; source publication used backups and an atomic forward-only push with explicit leases.
Exact current head:
ef78428c5eeb3035af36aa4fe137f5222cb66a6e; source tree:c85159c983db0a407a0e33649592274f66d07d80.