Repository navigation
[Expense tests] Re-enable reviewed APIs and consolidate Expense fixes - #12325
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
The historical workflow patch relocated the original core checks. Remove that duplicate while retaining the behavioral workflow tests and downstream AL adoption metadata assertions. 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
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
Remove the wrapper generator and its generator-specific tests. Keep the shared finalizer and behavioral tests that execute every project wrapper. 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
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
0ed4683 to
304e5b1
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session:3952f078-a881-4da8-ad96-13b727e48a91
Good Sense Reviewer - Round 1Recommendation: AcceptWhat this PR doesThis change adds shared credentials and cleanup for API tests, enables clean-codeunit execution, and moves the reviewed Expense API suites to the shared authentication provider. It also adds focused approval, permission, status, and amount regressions. The credential lifecycle, disabled-test selection, tenant restore, retry, and result-merging paths are covered by targeted tests, and the Expense API changes keep the existing request and assertion behavior. Problem-solution fitFit: Strong The change addresses the missing credential path that kept the authenticated API suites disabled. It re-enables the intended Expense API coverage while preserving the unrelated non-API exclusions. SuggestionsNone. Risk assessment and necessityRisk: The shared test infrastructure affects country projects and includes credential lifetime, clean-tenant restore, retry, and result-merging logic. The added Pester coverage exercises success and failure paths. The current System Application job failed because AL-Go could not retrieve a baseline artifact for warning comparison (HTTP 404), which is unrelated to the changed code; the remaining country jobs were still running. No report layout, public event surface, or BaseApp publisher dependency changed. Necessity: The API suites could not authenticate with the pipeline-generated credential, so the affected coverage remained disabled. The shared provider and cleanup hooks are required to restore that coverage safely, and the Expense rollout is appropriately scoped.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session:3952f078-a881-4da8-ad96-13b727e48a91
Temporarily sequence193 existing methods through DisabledTests selectors:135 APIV1 and58 APIV2. Removed in#11860; no product quarantine or selection logic. AB#646383 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session:3952f078-a881-4da8-ad96-13b727e48a91
Relocate the same193 temporary method exclusions without changing selection, authentication or tests. Preserve existing manifest order and remove separate stage files. AB#646383 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. |
Good Sense Reviewer - Round 2Recommendation: AcceptWhat this PR doesThe new commits keep 193 not-yet-authenticated general API test methods out of clean-codeunit execution by placing their method-specific selectors in the established APIV1 and APIV2 exclusion manifests. The final selectors exactly match the temporary staging set: 135 APIV1 methods and 58 APIV2 methods. Existing entries remain unchanged, no new duplicate keys were introduced, and every referenced codeunit and method exists. Status of previous suggestionsNone. New observations (commits since round 1)None. Risk assessment and necessityRisk: The main regression risk is excluding the wrong tests or changing existing selection. The added sets, entry order, existing manifest prefixes, duplicate keys, and source targets were checked and match the intended 193 methods. The completed failures are link-validation metadata checks rather than implementation failures; most code jobs are still running. Necessity: The method-specific exclusions are needed to enable the authenticated Expense API scope without also starting general API methods before their shared-authentication uptake. Reusing the existing manifests keeps the selection mechanism unchanged and preserves the staged rollout.
|
Fixes [AB#653393](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653393) <!-- Thanks for contributing to BCApps! A few things before you hit "Create pull request": - Your PR must link to an approved issue. New here? See CONTRIBUTING.md. - You must have built and run your change yourself. CI is a safety net, not a substitute. - If you used AI or an agent to write this PR, you are still the author. Read the diff, build it, and try it before requesting review. Contributing guide: https://github.com/microsoft/BCApps/blob/main/CONTRIBUTING.md Local dev environment: https://github.com/microsoft/BCApps/blob/main/LOCAL_DEV_ENV.md --> ## What & why <!-- A few sentences: what does this change do, and what problem does it solve? --> Revert commit 3f357d6, merged through #12325, as requested following the failures in [workflow run 37598783939](https://github.com/microsoft/BCApps/actions/runs/37598783939). This is the full 21-file revert, not just a test-exclusion change. It removes `enableCleanTestCodeunitExecution: true`, restores the prior Expense API authentication helper and 51 Expense API exclusions, removes the 193 temporary general API exclusions introduced by that commit, and rolls back its consolidated production/test changes. Subsequent AL-Go settings updates are preserved. The run has 19 integration-test jobs with Expense API HTTP 500 failures, two runner-disconnection failures, and one downstream status-check failure. Italy also has ten Avalara E2E test failures caused by shipment number-series date validation. The API errors report `Object reference not set to an instance of an object`; the available test call stacks do not identify the server-side null reference. This rollback mitigates the related enablement change, but does not prove that the underlying runtime defect is fixed. <details> <summary>Failure breakdown for all 22 jobs</summary> - **Users** = `EmployeesAPICanFilterExpenseUsers` in `Expense Users API Test` (148315). - **Capabilities** = `CapabilitiesProjectsEnabledViaAPI` in `Expense Capabilities API Test` (148318). - **Activity** = `ActivityLogEntryIsExposedThroughReadOnlyAPI` in `Expense Activity Log API Test` (148343). Each failed because its GET returned HTTP 500 with a null-reference error instead of HTTP 200. Verified against complete test-job logs, not just the generic `There are test failures!` wrapper or truncated annotations. | Job | Failure | | --- | --- | | [DK IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755266403) | Users | | [CH IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755266566) | Capabilities | | [ES IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755266678) | Users; Activity | | [CA IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755266862) | Users | | [CZ IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755267113) | Capabilities | | [AT IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755267195) | Self-hosted runner lost communication with GitHub; no failed test step recorded. | | [DE IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755267270) | Capabilities | | [BE IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755267653) | Users | | [GB UncategorizedTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755268475) | Self-hosted runner lost communication with GitHub; no failed test step recorded. | | [FI IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755268500) | Users | | [GB IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755270830) | Capabilities; Activity | | [AU IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755270887) | Users | | [SE IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755275791) | Capabilities | | [MX IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755275856) | Users | | [IT IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755276311) | Capabilities; ten Avalara E2E tests failed during invoice posting: `You cannot assign new numbers from the number series S-SHPT on a date before 10/07/26.` | | [NZ IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755277138) | Users | | [W1 IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755277205) | Users; Activity | | [US IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755277297) | Users | | [NL IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755278020) | Capabilities | | [NO IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755278941) | Capabilities | | [IS IntegrationTests](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112755279380) | Capabilities | | [Pull Request Status Check](https://github.com/microsoft/BCApps/actions/runs/37598783939/job/112823047509) | Failed because the preceding 21 jobs failed; not an additional build defect. | Italy's additional failures are in `End-to-End Tests` (133625), app `E-Document Connector - Avalara Tests`: `E2E_OutboundInvoice_Error_ManualResend_Complete`, `E2E_MultipleDocuments_IndependentLifecycles`, `E2E_ServiceDown_ThenRecover`, `E2E_ActivationThenSubmit_Complete`, `E2E_ConnectionSetup_SelectCompany_ThenSubmit`, `E2E_SendModeTest_SubmitSucceeds`, `E2E_BlockedMandate_Unblock_Resend_Complete`, `E2E_StatusCheck_HttpError_RetrySucceeds`, `E2E_PageVerification_AllStatusTransitions`, and `E2E_MessageResponseHeader_PopulatedDuringGetResponse`. Their call stacks reach `Library - E-Document.PostInvoice` -> `Sales-Post.UpdateShippingNo` -> `No. Series.GetNextNo`, before the intended connector lifecycle is exercised. The runner annotations establish lost communication, but do not distinguish machine termination, resource exhaustion, or network failure. </details> ## Linked work Tracking bug: [AB#653393](https://dev.azure.com/dynamicssmb2/_workitems/edit/653393) - intermittent Expense API HTTP 500 / SQL-pool null-reference failures after clean-tenant resets. Related to [AB#646383](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/646383). This is a non-closing link: the rollback mitigates CI blocking by reducing coverage, but does not establish that the underlying runtime defect is fixed. <!-- Required: link an approved GitHub issue using "Fixes #<number>". Microsoft contributors: also link the ADO work item with "AB#<number>" if you have one. --> Reverts #12325. Related failure run: https://github.com/microsoft/BCApps/actions/runs/37598783939. No approved GitHub issue was supplied for this rollback; the original PR has no closing GitHub issue reference. An approved issue link remains to be added before this draft is ready for review. No issue or work item is claimed fixed. ## How I validated this - [ ] I read the full diff and it contains only changes I intended. - [ ] I built the affected app(s) locally with no new analyzer warnings. - [ ] I ran the change in Business Central and confirmed it behaves as expected. - [x] I added or updated tests for the new behavior, or explained below why none are needed. **What I tested and the outcome** *(required — be specific: scenarios, commands, screenshots for UI changes)* <!-- Example: - Ran the new "Post and Send" action on a sales invoice in a fresh container; document posted and email queued (see screenshot). - New unit tests in MyFeatureTest.Codeunit.al pass locally; full module test suite green. - No tests added because change is comment-only / refactor with existing coverage. --> - `Invoke-Pester -Path '.\build\scripts\tests\ParallelTestExecution.Test.ps1' -PassThru` — **passed: 87 tests, 0 failed, 0 skipped**. Covers existing clean-codeunit selection, ordinary execution, and rerun infrastructure; not an AL integration run. - Compared `git ls-tree 3f357d6^ -- <path>` with `git ls-files --stage -- <path>` for every affected non-settings file — **passed: all 20 exactly match the original parent blobs**, including the restored helper and removed permission set. Verified exactly the original 21 changed paths. Reviewed the AL source diff and verified the settings diff only removes the enablement switch. - `Get-Content -LiteralPath <path> -Raw | ConvertFrom-Json` for each changed JSON file — **passed**. - `git diff --cached --check` — **failed only on the restored helper's original blank line at EOF**. Preserved that line for an exact revert. `git -c core.whitespace=-blank-at-eof diff --cached --check` — **passed**. - No new tests added: this restores previous source, fixtures, and exclusions rather than introducing replacement behavior. The original commit's added regression tests are also reverted. - Consulted `LOCAL_DEV_ENV.md` and `build/scripts/tests/runTests.ps1`. **AL compilation and Business Central runtime tests were not run locally**; no dedicated BC development container was provisioned for this rollback. Human full-diff review and runtime checkboxes remain open. CI results for this revert are pending. ## Risk & compatibility <!-- Anything reviewers should watch for: breaking changes, upgrade/data impact, permissions, telemetry, feature flags, follow-up work. Write "None" if there's nothing to call out. --> This full rollback includes production travel-request approver filtering and the associated permission/filter/date/test-fixture fixes. Restoring `SelectionFilterManagement.GetSelectionFilter` removes the exact-user filter fix and can reintroduce the range-gap behavior it addressed. Coverage is reduced by restoring Expense API exclusions and removing the clean-codeunit opt-in. Reintroduce the intended fixes and API coverage in a follow-up once their failures are understood. This change does not repair the two disconnected runners or Italy's independent Avalara number-series date failure. It also does not establish that the server-side HTTP 500 null reference is resolved. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Picks up the revert of #12325, which broke the Expense Agent API IntegrationTests for all PRs targeting main. AB#647832 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Scope
Related authentication and API enablement: AB#646383 (link only; this PR does not resolve the umbrella item).
Fixes AB#653119
Separate test defect: 653119 - Policy snapshot API URL composition and response isolation. This tracks the policy-snapshot test corrections, not production access permissions or unrelated runtime failures.
Expense-first adoption directly against main after the external squash merge of workflow #11891; remaining general APIs follow in #11860.
enableCleanTestCodeunitExecutionboolean. Use existing DisabledTests selectors throughout discovery, ordinary execution and clean-codeunit execution/reruns. No app-name allowlist, new selection setting or runner implementation/test change.This focused review diff has21 paths, using the two existing API exclusion manifests. No NAV selectors, local NST, BC-ExpenseAgent changes, unrelated new test scope or production permission changes.
Exact head:
bd3bbc04e13965f967a26eef435bdbf0cbc09ad7; tree:2b8c47b138846f63bd49d47188f54ea7e86c1f24.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 correction in 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. Historical baseline run37215976231 at head69df41756d918a516a3c868ca66f45cbfd320428 independently corroborates this: zero of the exact193 methods appear across five inspected W1 result artifacts containing37,626 testcases (Integration, both Legacy buckets, default/unit and Uncategorized). This evidence is limited to that W1 baseline, not every country or historical run. The completed alternate-path audit found no other ordinary configured baseline BCApps lane: APIV1/APIV2 are outside Legacy buckets, Disabled-unit fallback retains UnitTest filtering, discovery skips test procedures, and ordinary PR/CI/CD/rerun routes do not bypass those constraints. Manual or explicitly untyped execution remains possible; no universal historical or NAV absence is claimed.
**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.
AB#646383 (link only). 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:
bd3bbc04e13965f967a26eef435bdbf0cbc09ad7; source tree:2b8c47b138846f63bd49d47188f54ea7e86c1f24.