Skip to content

Bug 635057: [Subcontracting] Enable subcontracting tests that were disabled#8169

Merged
ChethanT merged 2 commits into
mainfrom
bugs/Subcontracting/635057-EnableDisabledTests
May 20, 2026
Merged

Bug 635057: [Subcontracting] Enable subcontracting tests that were disabled#8169
ChethanT merged 2 commits into
mainfrom
bugs/Subcontracting/635057-EnableDisabledTests

Conversation

@ChethanT
Copy link
Copy Markdown
Contributor

@ChethanT ChethanT commented May 15, 2026

What & why

Test were disabled while the root cause were being fixed in BaseApp. Now that they are fixed, it is time to enable the tests.

Linked work

Fixes AB#635057

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.
  • 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)

Risk & compatibility

@ChethanT ChethanT requested a review from a team as a code owner May 15, 2026 14:15
@ChethanT ChethanT requested a review from mirkobiasini May 15, 2026 14:15
@github-actions github-actions Bot added the AL: Apps (W1) Add-on apps for W1 label May 15, 2026
@ChethanT ChethanT added the Subcontracting Subcontracting related activities label May 15, 2026
@github-actions github-actions Bot added this to the Version 29.0 milestone May 15, 2026
@ChethanT ChethanT changed the title Bug 635057: Enable subcontracting tests that were diabled Bug 635057: [Subcontracting] Enable subcontracting tests that were diabled May 15, 2026
@ChethanT ChethanT removed the request for review from mirkobiasini May 15, 2026 15:25
@ChethanT ChethanT enabled auto-merge (squash) May 16, 2026 21:12
@alexei-dobriansky
Copy link
Copy Markdown
Contributor

Analysis

Scenario Validity:

  • The scenario is valid as engineering hygiene: the linked ADO bug says these disabled Subcontracting tests should be re-enabled only once the underlying WIP-item issue is actually fixed, especially on IN builds.
  • The most concrete statement is in the [AI-REPRO] comment: "remove the disabled markers ... run it both on W1 and IN builds ... Expected: All listed test methods run green on every supported build flavour, especially IN".
  • Clarity is only moderate, and the AI-REPRO explicitly scores it 5/10 because this is a tracker rather than a user-facing bug repro. Still, it gives a crisp acceptance criterion: re-enable, then prove green on W1 and IN.

Correctness:

  • The diff deletes src/Apps/W1/Subcontracting/Test/DisabledTests/SubcontractingTests.json outright. Because the runner loads every DisabledTests\*.json and passes them as disabledTests into the test run (build/scripts/RunTestsInBcContainer.ps1:10-22,62), deleting that file unquestionably re-enables every method listed there.
  • That file currently disables four methods, including the three WIP transfer tests in codeunit 149910 and one separate method in codeunit 139989 (src/Apps/W1/Subcontracting/Test/DisabledTests/SubcontractingTests.json:1-26).
  • The PR does satisfy the mechanical part of the ADO repro (remove disabled markers), but it fails the acceptance criterion. In the PR's own CI, the IN job fails on all three re-enabled WIP transfer tests:
    • PostWIPTransferOrder_ShipPartialReceiveFullReceive
    • PostWIPTransferOrder_FullWhseHandling_ShipPartialReceiveFullReceive
    • PostWIPTransferOrder_FullWhseHandling_SerialTrackedItem
  • The failing call stack is consistent across runs: TransferOrder-Post Receipt triggers the India GST subscriber GST Transfer Order Receipt, which errors with "There is no Item Register within the filter. Filters: Source Code: TRANSFER". That is shown directly in the PR build log for Build Apps (W1) (IN). So the exact region the work item called out is still broken.
  • The test code explains why this matters. These three methods explicitly verify WIP transfer receipts create no item ledger / warehouse ledger entries and keep base quantities at zero for WIP transfers (SubcWIPTransferPostTest.Codeunit.al:165-208, 287-341, 452-515). The IN localization still assumes an Item Register exists on transfer receipt, so the runtime contract is not actually fixed yet.
  • There is also a scope problem: the ADO comment enumerates the three codeunit-149910 methods, but the deleted JSON file also removes TestPostItemChargeAssignedToSubcontractingLingValueEntryWithCapacityRelation from codeunit 139989. I do not see evidence in the linked work item that this fourth test was re-validated.

Side Effects:

  • The immediate side effect is a red IN pipeline on the exact tests the PR is trying to re-enable.
  • The PR also broadens scope beyond the linked work item by re-enabling the extra codeunit-139989 test via full-file deletion.
  • There is no production-code blast radius here, but there is review/CI risk: merging this would convert a known-disabled test set into an actively failing localized pipeline without proving the underlying defect is gone.

Risk Assessment:

  • Medium. The diff is tiny and does not alter product behavior directly, so customer-facing blast radius is low. But the evidence says the underlying IN issue is still present, so the change is not actually ready.
  • If this is merged prematurely, the Subcontracting test suite becomes red in IN for a known reason, and the review goal (restoring safe automated coverage) is not achieved.
  • What if we don't make this change? Then those regressions remain hidden because the tests stay disabled. That is not great, but it is a safer state than pretending the issue is fixed when the PR's own IN run shows it is not. The practical workaround is to keep the disables in place until the BaseApp / IN-localization fix is present and the IN pipeline is green, or to re-enable only the methods that are actually proven green.

Test Coverage:

  • No new tests were added, which is fine for a test-enablement PR because the existing tests are the artifact being re-enabled.
  • More importantly, the existing tests provide direct evidence against the PR: the three WIP transfer scenarios fail in IN as soon as they are re-enabled, exactly as the ADO work item warned might happen.
  • I also do not see evidence that the extra codeunit-139989 test removed by the full-file deletion was exercised successfully.

Recommendation: Request Changes

  1. Do not delete the whole disabled-tests file yet — the PR's own Build Apps (W1) (IN) job shows the three re-enabled WIP transfer tests still fail with GST Transfer Order Receipt expecting an Item Register during transfer receipt. Re-enable them only after the underlying IN/BaseApp issue is demonstrably fixed.
  2. Narrow the change to the validated methods only — full-file deletion also re-enables TestPostItemChargeAssignedToSubcontractingLingValueEntryWithCapacityRelation (SubcontractingTests.json:2-7), but the linked work item and AI-REPRO comment focus on the three codeunit-149910 WIP transfer tests. Keep unrelated suppressions unless they are covered by the same evidence.
  3. Link or include the actual underlying fix before re-enabling — if the intended BaseApp / localization fix already exists, link it and rerun the IN pipeline successfully. Right now the diff only removes the suppression; it does not prove the root cause is gone.

@alexei-dobriansky
Copy link
Copy Markdown
Contributor

Check if IN tests can be re-enabled.

@ChethanT ChethanT changed the title Bug 635057: [Subcontracting] Enable subcontracting tests that were diabled Bug 635057: [Subcontracting] Enable subcontracting tests that were disabled May 19, 2026
@ChethanT ChethanT merged commit cbbf3d3 into main May 20, 2026
134 of 148 checks passed
@ChethanT ChethanT deleted the bugs/Subcontracting/635057-EnableDisabledTests branch May 20, 2026 14:30
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 Subcontracting Subcontracting related activities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants