Skip to content

[Main]-Re-enable BC tests disabled due to BCApps submodule migration-IT - #11234

Open
DeepsShukla wants to merge 1 commit into
mainfrom
bugs/Re-enableBCtests-disabled-IT
Open

DeepsShukla wants to merge 1 commit into
mainfrom
bugs/Re-enableBCtests-disabled-IT

Conversation

@DeepsShukla

@DeepsShukla DeepsShukla commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Workitem Bug 641038: [all-e]Re-enable BC tests disabled due to BCApps submodule migration (hardcoded GDL/DemoTool paths)

Fixes AB#641038

@github-actions github-actions Bot added the Team: Finance GitHub request for Finance area label Sep 9, 2026
@github-actions github-actions Bot added this to the Version 30.0 milestone Sep 9, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Stale Status Check Deleted

The Pull Request Build workflow run for this PR was older than 72 hours and has been deleted.

📋 Why was it deleted?

Status checks that are too old may no longer reflect the current state of the target branch. To ensure this PR is validated against the latest code and passes up-to-date checks, a fresh build is required.


🔄 How to trigger a new status check:

  1. 📤 Push a new commit to the PR branch, or
  2. 🔁 Close and reopen the PR

This will automatically trigger a new Pull Request Build workflow run.

@DeepsShukla
DeepsShukla marked this pull request as ready for review September 15, 2026 08:44
@DeepsShukla
DeepsShukla requested a review from a team as a code owner September 15, 2026 08:44
local procedure VerifyDatiFatturaAttributes(FileName: Text)
var
XMLDoc: DotNet XmlDocument;
XmlDocument: XmlDocument;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Testing}$

This refactor drops the schema-validation assertion from VerifyDatiFatturaAttributes: the helper now loads the XML and checks four root attributes via native XmlDocument/XmlElement APIs, but it no longer calls ValidateXmlAgainstXsdSchema as the old DotNet-based implementation did, and no other call site validates the Datifattura export against its XSD on this test path. In ExportedVATReportConsistsOfLinesFromOneVATReport_Datifattura, the flow is ExportFile_Datifattura -> VerifyDatiFatturaInvoiceNoAndDate -> VerifyDatiFatturaAttributes, so malformed-but-attribute-correct XML would now pass this test. Restore XSD validation in this helper (or an equivalent step earlier in the same verification flow) so the test still proves the generated document is schema-valid.

Agent judgement — not directly backed by a BCQuality knowledge article.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.41.6

@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Good Sense Reviewer - Round 1

Recommendation: Request Changes

What this PR does

This PR updates Italian test code so XML schema test assets can be found after the test asset layout changed. The intent is good, and one changed helper uses the right probe-and-fallback shape, but two other changed paths now hardcode the migrated layout and no longer preserve the standalone BCApps layout.

Problem-solution fit

Fit: Partial

The bug requires the same tests to pass in both layouts. The diff only does that in one file; two changed test areas can still fail when the migrated asset path is not present.

Suggestions

S1 (🔴 High): Probe before changing the schema root
This now builds only the migrated layout path. Standalone BCApps still needs the legacy schema location, so use a ServerFileExists probe and fall back to the old path when the new path is not present.

S2 (🔴 High): Keep the legacy asset path fallback
This helper always returns the migrated layout path. In standalone BCApps the schema files are still under the legacy path, so check the new path first and return GetInetRoot() + RelativePath when it is missing.

Risk assessment and necessity

Risk: The regression surface is limited to test code, but it is the exact test code this change is trying to re-enable. If the hardcoded paths stay, some tests can still fail in one supported checkout layout.

Necessity: The change is necessary because the schema assets must be found after the layout move. The scope is right, but each changed schema lookup must use the same layout-agnostic pattern before the fix is complete.


[AI-PR-REVIEW] version=1 promptVersion=4 system=github pr=11234 round=1 by=alexei-dobriansky at=2026-09-15T13:20:33Z lastSha=0ee0e53d7d82f1b9f4dc79c49395acd06bac44ae reviewKey=cfcd23c63c9b77028aa94f6b1ff56bed41345411ebee7bfb89e71a05c76f1dfc suggestions=S1@c003775f,S2@40ebab5c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team: Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants