Repository navigation
[API tests] Improve APIV1 fixture and verification reliability - #12684
Prangshuman Das (t-prda) wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
Team: Integrations
github-actions
Bot
added
Build: Automation
3345866 to
27005fe
Compare
27005fe to
45a2ff0
Compare
b5c8253 to
89bbf4c
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
The removed disabled-test entries re-enable tests that execute explicit COMMIT calls without a TransactionModel attribute. Test methods default to AutoRollback, so these tests can fail at the first commit instead of producing a business assertion; assign AutoCommit to the commit-driven tests and run them under an isolation-enabled test runner. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6 |
5a94dbb to
647f910
Compare
|
The newly enabled automation package tests execute explicit COMMIT calls without a TransactionModel attribute, so they use the default AutoRollback transaction model and can fail at runtime before producing a business assertion. Set the affected tests to AutoCommit and run them under a test runner with database isolation. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6 |
|
The newly enabled G/L entries test executes a COMMIT during initialization without a TransactionModel attribute, so it uses the default AutoRollback transaction model and can fail at runtime before producing a business assertion. Set the test to AutoCommit and run it under a test runner with database isolation. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6 |
|
The newly enabled discount percentage invoice test executes COMMIT without a TransactionModel attribute, so it uses the default AutoRollback transaction model and can fail at runtime before producing a business assertion. Set the test to AutoCommit and run it under a test runner with database isolation. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.50.6 |
647f910 to
1fc5deb
Compare
Good Sense Reviewer - Round 2Recommendation: AcceptWhat this PR doesThis PR makes ten APIV1 tests reliable enough to re-enable. It regenerates RapidStart fixtures, refreshes asynchronous package status, stabilizes journal handler state, selects created lines exactly, and enables the required application area. The full current diff matches established APIV2 patterns and changes no production code. Status of previous suggestionsNone from round 1. New observations (commits since round 1)None. Risk assessment and necessityRisk: Low because only test code and disabled-test metadata change. An incomplete fix could restore CI instability, but the updated patterns match APIV2 and the current checks show no failures. Necessity: Re-enabling these tests restores APIV1 regression coverage that would otherwise remain disabled. The scope is limited to the reliability fixes needed for that coverage.
|
1fc5deb to
46994a0
Compare
Find created lines by unique description, refresh polled configuration packages, fix assertion order and journal template handling, and re-enable the affected APIV1 tests. AB#646383 AB#653393 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
46994a0 to
de737ec
Compare
Good Sense Reviewer - Round 3Recommendation: AcceptWhat this PR doesThis PR makes ten APIV1 tests reliable enough to re-enable. It regenerates RapidStart fixtures for each test, refreshes asynchronous package state, keeps the journal handler selection stable, selects the exact API-created document line, and enables the required application area. The full current diff changes only test code and disabled-test metadata. Status of previous suggestionsNone from round 2. New observations (commits since round 2)None. The branch history was rewritten, so the full net PR diff was rechecked. The reliability changes still match the corresponding APIV2 patterns, and the removed exclusions map to the corrected test methods. Risk assessment and necessityRisk: Low because the changes affect only tests and disabled-test metadata. The main risk is renewed CI instability if an asynchronous or fixture fix is incomplete; the analogous APIV2 implementations support the selected patterns, and the current checks pass. Necessity: Re-enabling these tests restores APIV1 regression coverage that would otherwise remain disabled. The scope is limited to the reliability fixes needed for that coverage.
|
Review focus: ``MicrosoftTestAuthProvider`` (about 60 lines) and the enum default; the ``build/`` changes are deletions plus the anti-SSRF line in ``NewBcContainer.ps1``. Defaults API test authentication (`Library - Graph Mgt`) to the current user's web service key. API tests no longer need per-app auth uptake or a container password. - `API Test Authentication` enum: `DefaultImplementation` = `Microsoft Test Auth Provider`. Removes `No API Test Auth Provider` and the explicit `Microsoft Test Environment` value. - `Microsoft Test Auth Provider`: on-premises with NavUserPassword, sends Basic auth with `UserId()` and the web service key. When the key is missing or expired, a background session (`API Test Web Service Key`, CU 131025) creates it with a 24h expiry and commits it, because the API request runs in a separate server session. The caller's transaction is never committed; the provider waits up to 60 s for the session and then re-reads the key. - `Library - Graph Mgt`: adds the `SetLicenseSafeWorkDate` and `AddFieldToIgnoreIfExists` helpers used by later layers. - Build: removes the container password and Key Vault credential plumbing (`ApiTestCredential.psm1`, `Remove-ApiTestPassword.ps1`, `PipelineFinalize` hooks and their tests). Keeps the clean-tenant execution logic. Sets `NavHttpClientAntiSSRFEnabled=false` in the test container, as NAV `RunALTests.ps1` does: since microsoft#12323, API tests call the container's own endpoint through AL `HttpClient`, which anti-SSRF blocks. - CU 139494 `API Test Auth Provider Tests`: covers key creation and renewal. Declares `TestHttpRequestPolicy = AllowAllOutboundRequests` (NAV blocks outbound requests by default; microsoft#12694 covered the other API tests). Related: [AB#646383](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/646383), [AB#653393](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653393) ### Stack (each PR shows only its own layer; merge bottom first) 1. microsoft#12674 A: web service key auth (base: `main`) 2. microsoft#12675 B: Expense API tests + clean test codeunit execution 3. microsoft#12676 C: re-enable APIV1/APIV2 API tests 4. microsoft#12890 Review fixes for A: simplify web service key handling 5. Area fixes: microsoft#12677 D1 VAT fixtures → microsoft#12678 D2 cancellation reasons → microsoft#12679 D3 localized discounts → microsoft#12680 D4 quote dates → microsoft#12681 D5 journal numbering → microsoft#12682 D6 field comparison → microsoft#12683 D7 PDF documents → microsoft#12684 D8 APIV1 reliability Experiments (on D8, do not merge): microsoft#12685 E single tenant, microsoft#12686 F no clean execution. Combined validation (do not merge): microsoft#12687, same tree as D8. Platform: `main` already pins BCPlatform 30.0.56112.0, which includes the SQL connection-pool fix ([AB#653457](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653457)). Note for CI: AL-Go's new-warning check needs a CI/CD baseline from the base branch, and `features/*` bases have none, so a PR build there fails with a 404. The current CI runs were triggered while these PRs targeted `main`, so their results are valid for the cumulative change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Review focus: the Expense test changes and the auth-helper deletion; the ``enableCleanTestCodeunitExecution`` line. The large JSON diffs are temporary disables, undone in C. Re-enables the Expense Agent API tests on top of the default web service key auth. No Key Vault password and no auth subscriber. - Deletes `Expense API Test Auth Helper` and its usages. - Re-applies the Expense fixes from microsoft#12325 (reverted in microsoft#12514): `TravelRequestApproval`, Expense permission and spend-request tests, the `ExpDetailTest` permission set, and `SpendRequestTests`. - Turns on `enableCleanTestCodeunitExecution`, as microsoft#12325 did, so `RequiredTestIsolation = Disabled` codeunits run in clean-tenant lanes. - Expense disabled tests: keeps only the entries NAV master also disables (12). - Temporarily disables the unrelated APIV1/APIV2 tests that the clean lanes start executing, the same entries microsoft#12325 added. They are re-enabled in the next layers. Related: [AB#646383](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/646383), [AB#653393](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653393) ### Stack (each PR shows only its own layer; merge bottom first) 1. microsoft#12674 A: web service key auth (base: `main`) 2. microsoft#12675 B: Expense API tests + clean test codeunit execution 3. microsoft#12676 C: re-enable APIV1/APIV2 API tests 4. microsoft#12890 Review fixes for A: simplify web service key handling 5. Area fixes: microsoft#12677 D1 VAT fixtures → microsoft#12678 D2 cancellation reasons → microsoft#12679 D3 localized discounts → microsoft#12680 D4 quote dates → microsoft#12681 D5 journal numbering → microsoft#12682 D6 field comparison → microsoft#12683 D7 PDF documents → microsoft#12684 D8 APIV1 reliability Experiments (on D8, do not merge): microsoft#12685 E single tenant, microsoft#12686 F no clean execution. Combined validation (do not merge): microsoft#12687, same tree as D8. Platform: `main` already pins BCPlatform 30.0.56112.0, which includes the SQL connection-pool fix ([AB#653457](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653457)). Note for CI: AL-Go's new-warning check needs a CI/CD baseline from the base branch, and `features/*` bases have none, so a PR build there fails with a 404. The current CI runs were triggered while these PRs targeted `main`, so their results are valid for the cumulative change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Review focus: mostly mechanical. ``Initialize`` sets a license-safe work date in each API test, and the JSON deletions re-enable tests. A few files have small fixture or isolation tweaks. Re-enables the APIV1, APIV2, IRS 1099 and E-Document API tests that pass without area-specific fixes. Aligned with NAV master: only tests that NAV master also runs are re-enabled. - API test `Initialize` sets a license-safe work date. Adds minimal fixture and isolation adjustments. - Removes the APIV1/APIV2 exclusions, keeping only the entries fixed in the following area PRs and the two `139806` methods NAV master disables. Removes E-Doc `135575`. Related: [AB#646383](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/646383), [AB#653393](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653393) ### Stack (each PR shows only its own layer; merge bottom first) 1. microsoft#12674 A: web service key auth (base: `main`) 2. microsoft#12675 B: Expense API tests + clean test codeunit execution 3. microsoft#12676 C: re-enable APIV1/APIV2 API tests 4. microsoft#12890 Review fixes for A: simplify web service key handling 5. Area fixes: microsoft#12677 D1 VAT fixtures → microsoft#12678 D2 cancellation reasons → microsoft#12679 D3 localized discounts → microsoft#12680 D4 quote dates → microsoft#12681 D5 journal numbering → microsoft#12682 D6 field comparison → microsoft#12683 D7 PDF documents → microsoft#12684 D8 APIV1 reliability Experiments (on D8, do not merge): microsoft#12685 E single tenant, microsoft#12686 F no clean execution. Combined validation (do not merge): microsoft#12687, same tree as D8. Platform: `main` already pins BCPlatform 30.0.56112.0, which includes the SQL connection-pool fix ([AB#653457](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/653457)). Note for CI: AL-Go's new-warning check needs a CI/CD baseline from the base branch, and `features/*` bases have none, so a PR build there fails with a 404. The current CI runs were triggered while these PRs targeted `main`, so their results are valid for the cumulative change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Review focus: APIV1 test reliability changes; the APIV1 exclusion file is deleted.
Area fix (previously #11322): APIV1 fixture and verification reliability. Re-enables the last APIV1 tests and deletes the APIV1 exclusion file.
Related: AB#646383, AB#653393
Stack (each PR shows only its own layer; merge bottom first)
main)Experiments (on D8, do not merge): #12685 E single tenant, #12686 F no clean execution. Combined validation (do not merge): #12687, same tree as D8.
Platform:
mainalready pins BCPlatform 30.0.56112.0, which includes the SQL connection-pool fix (AB#653457).Note for CI: AL-Go's new-warning check needs a CI/CD baseline from the base branch, and
features/*bases have none, so a PR build there fails with a 404. The current CI runs were triggered while these PRs targetedmain, so their results are valid for the cumulative change.