Repository navigation
[API tests] Re-enable APIV1 and APIV2 API tests - #12676
Prangshuman Das (t-prda) merged 2 commits into
Conversation
92ef499 to
de3cca8
Compare
|
The AL source file name uses a lowercase object-name segment (APIV2itemLedgEntriesE2E), which breaks the PascalCase ..al convention. Rename it to APIV2ItemLedgEntriesE2E.Codeunit.al. 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 |
bd52825 to
169957b
Compare
169957b to
d040033
Compare
Remove the Expense API Test Auth Helper (no Key Vault secret, no subscriber) and apply the Expense app and test fixes. Keep only Expense disablements that NAV master also has. Enable clean test codeunit execution; APIV1/APIV2 tests that this surfaces are temporarily excluded (entries from #12325) and re-enabled by the following layers. AB#646383 AB#653393 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
Re-enable APIV1, APIV2, IRS 1099 and E-Document API tests that pass without area-specific fixture fixes. Only tests fixed by the following area layers and the two Company Info. tests that NAV master disables stay excluded. AB#646383 AB#653393 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
d040033 to
b301b04
Compare
|
The file name uses a lowercase object-name segment ( 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 |
Good Sense Reviewer - Round 2Recommendation: AcceptWhat this PR doesThis revision re-enables the APIV1, APIV2, IRS 1099, and E-Document API tests that are ready to run. It sets a license-safe work date, prepares fixtures before API calls, and keeps the remaining known failures excluded. The full current diff was reviewed because the previous head was replaced. The APIV1 and APIV2 counterparts use consistent setup and isolation patterns, the purchase-order dates now follow the configured work date, and the retained disabled-test entries still map to valid test methods. Status of previous suggestionsNone. New observations (commits since round 1)None. Risk assessment and necessityRisk: Re-enabling 1,150 test exclusions broadens the CI surface, and several API tests disable isolation so external sessions can see committed fixtures. The fixtures are initialized before API activity, the paired API versions follow the same pattern, the remaining exclusions are valid, and all completed checks are green. Necessity: Restoring these tests is needed to recover broad API regression coverage after the shared authentication and runtime prerequisites became available. The scope is appropriate because it re-enables the passing set and retains the known unrelated failures.
|
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.
Initializesets 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.
Initializesets a license-safe work date. Adds minimal fixture and isolation adjustments.139806methods NAV master disables. Removes E-Doc135575.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.