Skip to content

[API tests] Ignore localized fields in API document comparisons - #12682

Open
Prangshuman Das (t-prda) wants to merge 1 commit into
features/646383-stack-d5-journal-numberingfrom
features/646383-stack-d6-field-comparison
Open

Prangshuman Das (t-prda) wants to merge 1 commit into
features/646383-stack-d5-journal-numberingfrom
features/646383-stack-d6-field-comparison

Conversation

@t-prda

@t-prda Prangshuman Das (t-prda) commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Review focus: AddFieldToIgnoreIfExists call sites.

Area fix (previously #11229): ignores localized fields in API document comparisons, then re-enables the affected tests.

Related: AB#646383, AB#653393

Stack (each PR shows only its own layer; merge bottom first)

  1. [API tests] Default to web service key authentication #12674 A: web service key auth (base: main)
  2. [Expense tests] Re-enable Expense Agent API tests #12675 B: Expense API tests + clean test codeunit execution
  3. [API tests] Re-enable APIV1 and APIV2 API tests #12676 C: re-enable APIV1/APIV2 API tests
  4. Area fixes: [API tests] Ensure VAT posting setup in API document fixtures #12677 D1 VAT fixtures → [API tests] Ensure cancellation reason codes in API sales document tests #12678 D2 cancellation reasons → [API tests] Handle localized purchase invoice payment discounts in APIV1 tests #12679 D3 localized discounts → [API tests] Set sales quote document and posting dates in API tests #12680 D4 quote dates → [API tests] Assign a number series to API vendor payment journals #12681 D5 journal numbering → [API tests] Ignore localized fields in API document comparisons #12682 D6 field comparison → [API tests] Return shipment PDF selection and pdfDocument test isolation #12683 D7 PDF documents → [API tests] Improve APIV1 fixture and verification reliability #12684 D8 APIV1 reliability

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: main already 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 targeted main, so their results are valid for the cumulative change.

@github-actions github-actions Bot added the AL: Apps (W1) Add-on apps for W1 label Oct 12, 2026
@github-actions github-actions Bot added the Team: Integrations GitHub request for Integrations area label Oct 12, 2026
@t-prda
Prangshuman Das (t-prda) changed the base branch from features/646383-stack-d5-journal-numbering to main October 12, 2026 11:13
@github-actions github-actions Bot added Build: Automation Workflows and other setup in .github folder Build: scripts & configs Build scripts and configuration files Team: Finance GitHub request for Finance area and removed Team: Integrations GitHub request for Integrations area labels Oct 12, 2026
@github-actions github-actions Bot added this to the Version 30.0 milestone Oct 12, 2026
@t-prda
Prangshuman Das (t-prda) force-pushed the features/646383-stack-d6-field-comparison branch from ab249f3 to f853da9 Compare October 12, 2026 11:28
@t-prda
Prangshuman Das (t-prda) changed the base branch from main to features/646383-stack-d5-journal-numbering October 12, 2026 12:29
@t-prda
Prangshuman Das (t-prda) added this pull request to stack #12697 October 12, 2026 15:38
@t-prda
Prangshuman Das (t-prda) marked this pull request as ready for review October 13, 2026 08:58
@t-prda
Prangshuman Das (t-prda) requested a review from a team October 13, 2026 08:58
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Accept with Suggestions

What this PR does

This change re-enables ten API document comparison tests and ignores localized date fields when they exist. The dynamic field lookup is safe across country builds, and the APIV1 and APIV2 changes are consistent, but two changes remove more date validation than the scenario establishes.

Problem-solution fit

Fit: Partial

The change restores the excluded tests and handles country-specific fields without compile-time dependencies. It does not establish why every ignored date may differ, and the purchase-order change broadens an existing time-zone exception to all runs.

Suggestions

S1 (🟠 Moderate): Keep coverage for ignored localized dates
These calls remove all checks for Operation Occurred Date and, for quotes, VAT Reporting Date, but the scenario does not show why the API and UI values may differ. Add focused assertions for their expected values, or limit each ignore to the country and setup that needs it, so the re-enabled tests do not hide a wrong posting or VAT date.

S2 (🟠 Moderate): Keep Order Date comparison time-bound
This moves Order Date into the always-ignored set, although the existing comment limits the time-zone exception to the 00:00-02:00 window. Keep it inside that condition, or assert the expected order date separately, so an API regression cannot pass for the rest of the day.

Risk assessment and necessity

Risk: The change is test-only and does not alter runtime or public surfaces. The risk is false-green coverage in countries that use operation or VAT dates, especially because these fields can affect posting and VAT reporting.

Necessity: Restoring the excluded API tests is worthwhile, and the country-aware helper is a suitable mechanism. The scope should keep date assertions wherever the values are expected to match.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=12682 round=1 by=alexei-dobriansky at=2026-10-13T22:21:55.5729808Z lastSha=f853da94a8df86710421e2acd82f42a1a0f4f8cd reviewKey=b1912cf7b5ddc1ad3a94d9064c08068c6fe7825486df7e837d557714ba128d41 suggestions=S1@2cffaafa,S2@784bad11

@t-prda
Prangshuman Das (t-prda) requested a review from a team as a code owner October 14, 2026 09:27
@t-prda
Prangshuman Das (t-prda) requested a review from a team October 14, 2026 09:27
@t-prda
Prangshuman Das (t-prda) force-pushed the features/646383-stack-d6-field-comparison branch from f853da9 to 4ada5f6 Compare October 14, 2026 09:27
@github-actions github-actions Bot removed Build: Automation Workflows and other setup in .github folder Build: scripts & configs Build scripts and configuration files labels Oct 14, 2026
@t-prda Prangshuman Das (t-prda) added the Team: Finance GitHub request for Finance area label Oct 14, 2026
@github-actions github-actions Bot removed the Team: Finance GitHub request for Finance area label Oct 14, 2026
@t-prda
Prangshuman Das (t-prda) force-pushed the features/646383-stack-d6-field-comparison branch from 4ada5f6 to 7ceaecc Compare October 14, 2026 09:48
Comment thread src/Apps/W1/APIV2/test/src/APIV2PurchaseOrdersE2E.Codeunit.al
@t-prda
Prangshuman Das (t-prda) force-pushed the features/646383-stack-d6-field-comparison branch from 7ceaecc to 12194aa Compare October 14, 2026 10:30
Comment thread src/Apps/W1/APIV2/test/src/APIV2PurchaseOrdersE2E.Codeunit.al
@t-prda

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Both suggestions come from the same cause: these fields are not exposed by the API, so the two creation paths default them in different sessions with different WorkDates.

S2 (Order Date): in APIV2 purchaseOrders, orderDate maps to Rec."Document Date" (field(orderDate; Rec."Document Date")), not to Order Date. The API never sets Purchase Header."Order Date", so the API session defaults it from its own WorkDate, which is today. The page path runs in the test session, whose WorkDate is pinned by LibraryGraphMgt.SetLicenseSafeWorkDate() (Nov 15). So Order Date differs on every run, not only between 00:00 and 02:00. The API's orderDate mapping is still covered: the test locates the API document with SetRange("Document Date", OrderDate) and compares Document Date.

S1 (Operation Occurred Date / VAT Reporting Date): neither field is exposed by any APIV1/APIV2 page. Both are derived in the session that creates the document (VAT Reporting Date := GLSetup.GetVATDate("Posting Date", "Document Date"), and the localized Operation Occurred Date similarly), so they follow that session's dates. AddFieldToIgnoreIfExists only ignores them in the countries where the field exists. This comparison test cannot assert an expected API value for a field the API does not write. Their correctness belongs to the posting and VAT tests that own those fields.

No code change for these two; the AL review thread on the Order Date line has the same explanation.

Skip localized header fields that only exist in some countries when comparing page and
API documents and re-enable the through-page-and-API tests.

AB#646383
AB#653393

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3952f078-a881-4da8-ad96-13b727e48a91
@t-prda
Prangshuman Das (t-prda) force-pushed the features/646383-stack-d6-field-comparison branch from 12194aa to 2a25095 Compare October 14, 2026 14:04
Comment thread src/Apps/W1/APIV2/test/src/APIV2PurchaseOrdersE2E.Codeunit.al
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 2

Recommendation: Accept

What this PR does

This change re-enables ten page-versus-API document comparison tests and excludes fields that are not part of the API contract or only exist in some countries. The exposed document and posting dates remain checked, and the dynamic field lookup limits each localized exclusion to tables where that field exists.

Status of previous suggestions
ID Title Status Author response
S1 Keep coverage for ignored localized dates Disputed These fields are not exposed by the APIs and are derived in each creating session. Their behavior belongs to the localized posting and VAT tests that own them.
S2 Keep Order Date comparison time-bound Disputed orderDate maps to Document Date, which remains checked. Order Date is not exposed and defaults in the separate API session, so it cannot reliably match the page-created record.
New observations (commits since round 1)

None.

Risk assessment and necessity

Risk: The change is test-only. The remaining false-green risk is limited because API-exposed dates are still compared, while only localized or separate-session defaults are excluded.

Necessity: Restoring these ten tests improves API coverage. The narrow exclusions are required so the comparisons validate the API contract instead of unrelated country fields and session-local defaults.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=12682 round=2 by=alexei-dobriansky at=2026-10-14T22:53:08.0405399Z lastSha=2a25095b351818a82380ea4bca11bd432df22925 reviewKey=450002b48f40d92b34e1785833a2474cd9a27f9627b4572465bbb87156f1fd0a suggestions=S1@2cffaafa:disputed,S2@784bad11:disputed parentRound=1

This branch was successfully deployed

1 active (outdated) deployment
triage — 4ada5f65 Deployed Oct 14, 2026 by t-prda via Classify team ownership #7801
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: Apps (W1) Add-on apps for W1 Team: Integrations GitHub request for Integrations area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants