Split the legacy test stage into four buckets - #9802
Open
Groenbech96 wants to merge 17 commits into
Open
Conversation
Tests-SCM is a single test app (~8.6k tests, ~113 min) and the long pole in the BCApps test run, since ParallelTestExecution dispatches each test app as its own parallel job on a separate tenant. This carves the Warehouse/WMS sub-domain (39 codeunits, ~2187 tests) into a new Tests-SCM-Warehouse app so it runs concurrently with the Tests-SCM remainder, shortening the critical path. - New app src/Layers/W1/Tests/SCM-Warehouse (app.json + 39 moved codeunits) - Register in build/projects.json, build/groups.json and TestConfiguration.json (LegacyTests-Bucket2, next to Tests-SCM) - Add the folder to testFolders in all 23 Apps <country> AL-Go settings - Move 362 matching disabled-test entries into DisabledTests/Tests-SCM-Warehouse and prune them from the Tests-SCM files No code coupling between the two apps (0 cross-references verified); object IDs are unchanged and remain globally unique, so no renumbering is needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds Tests-SCM-Warehouse to Base Application internalsVisibleTo, resolving AL0161 failures in the country builds. [bcapps-fix-loop attempt 1] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 093dac30-6dd7-42da-9378-2e096ccc6fd5
The Tests-SCM split moved warehouse test codeunits from Layers/W1/Tests/SCM to Layers/W1/Tests/SCM-Warehouse, but left the country-layer localization overrides in Layers/<CC>/Tests/SCM. Because country layers overlay onto the W1 app by folder name, those overrides kept shipping in Tests-SCM while their W1 base now ships in Tests-SCM-Warehouse, causing duplicate object errors at install time (e.g. codeunit 137048 'SCM Warehouse II' in NZ). Move the 11 stray overrides (CH, CZ, GB, NA, NO, NZ, RU) into a Tests/SCM-Warehouse folder in each country layer, matching the base relocation and the existing SCM-Assembly/Service overlay pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8ae9b321-45bb-411e-9b9d-e8cde28c467e
Follow-up to the Tests-SCM-Warehouse split, this time driven by *measured* per-codeunit runtimes pulled from PR 9680's LegacyTestsBucket2 test-result artifact rather than by test counts. Measured baseline (W1, Bucket2): Tests-SCM was still 86.7 min of test time across 166 codeunits and remained the critical path for the whole job -- the other 20 apps in the bucket finished long before it. A single codeunit, 139491 "WFW Item Journal Batch", accounted for 25.9 min (30% of Tests-SCM) by itself. Extracts four sub-domain apps (93 codeunits): Tests-SCM-Workflow 3 codeunits 26.2 min (WFW*, Transfer Order Events) Tests-SCM-Planning 26 codeunits 14.4 min Tests-SCM-Costing 43 codeunits 11.4 min Tests-SCM-Reservation 21 codeunits 10.2 min Tests-SCM (remainder) 77 codeunits 24.5 min Also rebalances the legacy buckets from the same measurements. Bucket2 held 210.7 min of test work against Bucket1's 192.3 min, and each bucket runs in a container with only 3 tenants, so a bucket can never finish faster than total/3. Moving Tests-SCM-Costing to Bucket1 evens the two out, and both bucket lists are now ordered longest-first, which is what the FIFO dispatch loop in ParallelTestExecution.psm1 wants (LPT scheduling): before Bucket1 64.1 min floor Bucket2 86.7 min floor (Tests-SCM bound) after Bucket1 67.9 min floor Bucket2 66.4 min floor (total bound) Safety: - 0 cross-app codeunit references between the five resulting apps, so they compile independently with no new inter-app dependency. - Object IDs unchanged, so no renumbering. - Country-layer overrides moved alongside their W1 base (the overlay binds by folder name; leaving them behind caused duplicate-object install errors in the Warehouse split). - Disabled-test entries conserved exactly: 1,142 in Tests-SCM before, 312 + 419 + 106 + 305 + 0 = 1,142 after. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Transient platform races (OpenForm 130455) kill a test job within about a
minute of dispatch, so the victim is almost always one of the first apps
dispatched -- which, with the longest-first bucket order, is the longest app.
Re-queueing it at the back meant it restarted only after every other app had
been dispatched, adding its entire duration to the tail of the run.
Both observed Bucket2 runs hit exactly this:
PR 9680, W1 Bucket2 Tests-SCM raced at 09:39, re-dispatched 10:33 (+54 min)
job's test phase 151 min instead of ~98 min
main, W1 Bucket2 Tests-SCM raced at 23:19, re-dispatched 00:00 (+41 min)
Re-queueing at the front lets the retry take the next tenant that frees up,
which is what the one-retry-cap already assumes. Adds a regression test that
fails on the old ordering.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Each legacy bucket runs its test apps in parallel across the container's
tenants, so the bucket can never finish faster than total_work / tenants.
With Tests-SCM now split into six sub-domain apps, three tenants is the
binding constraint rather than any single app:
Bucket2 199.3 min of test work, largest app 32.7 min
3 tenants -> max(66.4, 32.7) = 66.4 min (tenant-bound)
6 tenants -> max(33.2, 32.7) = 33.2 min (now app-bound)
Measured cost and headroom from the Bucket2 run:
- Tenant creation is ~50 s per extra tenant (copy + mount + app sync), so
6 tenants adds ~4 min to container setup against a ~33 min saving.
- Free memory inside the 15.9 GB container is flat with respect to how many
tenants are busy -- median 1.6 GB at 1 busy tenant, 2.2 GB at 3, minimum
1.2 GB in every bucket. The footprint is dominated by the shared service
tier and SQL buffer cache, which gives back memory under pressure, not by
per-tenant test sessions, so tenant count is not the memory driver.
Bucket1 becomes the new long pole at 57.4 min because Tests-ERM alone is
57.4 min; splitting it is the next lever.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The generator used PowerShell -replace, which substitutes every match, so the new app's id overwrote the id of each entry in the dependencies array as well. Each of Tests-SCM-{Costing,Planning,Reservation,Workflow} therefore declared four dependencies on itself.
Sort-AppFoldersByDependencies recursed forever on the self-edge and the Initialization job died with 'The script failed due to call depth overflow' before any app was built.
Dependency ids now match the Tests-SCM template they were copied from; verified no self-dependency and no cycle anywhere in the 844-app graph.
[bcapps-fix-loop attempt 1]
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
main added src/Apps/DK/EDocumentFormatOIOUBL/test ('E-Document Format for OIOUBL Tests') but left Get-AffectedApps asserting a fan-out of 50, so the test now sees 51. This fails on main itself (run 30251214379) and on every PR that touches a .ps1, not just this one.
Bumped to 51 and noted that the number tracks the E-Document app count, so the next person to add one knows why it moves.
[bcapps-fix-loop attempt 2]
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tSettings Resolves 'Verify App Changes' / ValidateCountryProjects: 'Country project settings are out of date. Run build/scripts/Update-CountryProjectSettings.ps1 and commit the changes.' (all 23 countries + W1 flagged). The new SCM test folders were added to testFolders by hand, appended after SCM-Warehouse, but the generator emits the list alphabetically. Regenerated with the authoritative script instead of hand-editing. The generator also normalises 'Apps GB' from the PowerShell-style indentation it carries on main to the standard formatting; that drift is pre-existing and unrelated to this PR. Verified every one of the 23 files is semantically identical before/after - same keys, same appFolders (130) and testFolders (134) sets - so the change is ordering and formatting only. Update-CountryProjectSettings.ps1 -Validate now reports PASSED. [bcapps-fix-loop attempt 3] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests-ERM was the binding constraint of LegacyTestsBucket1: at 52.2 min of measured test time in a single app it set the bucket floor regardless of tenant count, since ParallelTestExecution dispatches one app per tenant and tests inside an app run serially. Bucket1 therefore gained only ~3% goodput from the 3 -> 6 tenant increase while paying a ~1.6x contention tax. Split by sub-domain, sized on measured per-codeunit runtimes: Tests-ERM-Application 31 codeunits 14.3 min (apply/unapply, tolerance) Tests-ERM (remainder) 144 codeunits 12.4 min (pricing, O365, banking, IC) Tests-ERM-Purchase 32 codeunits 9.7 min Tests-ERM-Sales 25 codeunits 8.5 min Tests-ERM-Finance 62 codeunits 8.0 min Bucket1 floor at 6 tenants drops 52.2 -> 32.0 min and the bucket becomes tenant-bound rather than app-bound. The legacy critical path across both buckets drops 52.2 -> 34.6 min. Safety: - Zero cross-app references. Only 15 intra-ERM reference edges existed, forming 10 clusters; a union-find keeps every cluster inside one app. - Object IDs unchanged; no <kind,id> is owned by two ERM apps in a layer. - 182 country-layer overrides moved alongside their W1 base across 20 layers, since the overlay binds by folder name. - Disabled tests conserved exactly: 3,705 before -> 1,233 + 265 + 469 + 781 + 957 = 3,705 after, none left in an app that lost its codeunit. - No app changed bucket, preserving the additionalDemoDataTypes (Standard, Evaluation) that only LegacyTestsBucket1 containers get. - Country settings regenerated via Update-CountryProjectSettings.ps1; -Validate passes. build/scripts Pester suite: 118/118 pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # build/projects/Apps CA/.AL-Go/settings.json # build/projects/Apps CZ/.AL-Go/settings.json # build/projects/Apps DK/.AL-Go/settings.json # build/projects/Apps FR/.AL-Go/settings.json # build/projects/Apps GB/.AL-Go/settings.json # build/projects/Apps US/.AL-Go/settings.json
…ders
src/Layers/<layer>/.layer/excluded_view_files.json lists W1 files that are
excluded from that layer's generated view. The entries are layer-relative
paths that embed the owning app folder, e.g.
Tests\ERM\ERMVATSettlementwithApply.Codeunit.al
Splitting a test app moves the file, so every exclusion entry for it goes
stale and the file starts compiling in countries that had deliberately
excluded it. W1 compiled 294 files of Tests-ERM while CA compiled 268 - the
26 difference is exactly this mechanism.
That is what broke run 30347236203: Apps IT/ES/MX/CA/BE failed in
Compile Apps with errors that are all "this W1 test cannot compile in this
country", e.g.
AL0132 '"Tax Type"' does not contain a definition for 'Sales Tax'
(NA-derived countries use 'Sales and Use Tax')
AL0135 no argument for required parameter 'ShowVATEntries'
(IT/BE change the VAT settlement report signature)
AL0132 'Report Check' has no 'FormatNoText' / 'InitTextVariable'
AL0132 'TestPage Posted Sales Invoice' has no 'Statistics'
Repoints 1017 entries across 25 layers, derived from git's exact rename
records for src/Layers/W1/Tests between the merge base and HEAD, so the
mapping is old path -> new path with no name guessing. Entry counts and the
4-space formatting are unchanged; the diff is exactly 1017 lines each way.
This also fixes the same latent breakage from the earlier Tests-SCM split,
which silently un-excluded its moved files - those tests have been compiling
and running in countries that had opted out of them.
Verified: 0 exclusion entries that resolved at the merge base fail to
resolve now.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Buckets are separate jobs on separate runners with separate containers, so they run fully in parallel - run 30354787707 used 44 distinct runners with Bucket1 and Bucket2 starting in the same minute. Splitting the work across more buckets therefore adds real, contention-free capacity, unlike raising the tenant count inside one container. Measured on run 30354787707 (W1), Bucket1 carries 388.4 min of test work against Bucket2's 313.1, so the stage is bound by Bucket1. The constraint that shapes the split: only LegacyTestsBucket1 containers get additionalDemoDataTypes ["Standard","Evaluation"], so an app cannot move between the two classes without changing the data it tests against. Keeping every app in its current class: split Bucket2 in two 388.4 / 156.5 / 156.5 -> critical 388.4 (no gain) split Bucket1 in two 194.4 / 194.0 / 313.1 -> critical 313.1 (-19%) split both (this PR) 194.4 / 194.0 / 156.5 / 156.5 -> critical 194.4 (-50%) The intuitive "add a third bucket" is worthless unless it splits Bucket1, and the unconstrained ideal 3-way (233.8 min) is unreachable across the demo-data boundary - which is why this goes to four rather than three. Bucket3 is a sibling of Bucket1 and inherits its additionalDemoDataTypes; Bucket4 is a sibling of Bucket2. No app changes demo-data class, and no app changes the dataset it runs against. Assignment is LPT over each class using measured per-app times from run 30354787707, and each list is ordered longest-first for the FIFO dispatch loop. No code changes were needed: Get-AppNamesForBucket already resolves "LegacyTests-Bucket$N" generically and the Uncategorized path discovers every LegacyTests-* key. Adds six Pester tests over the bucket configuration, because the failure modes here are silent: an app listed twice runs its tests twice, and an app listed nowhere falls through to the Uncategorized build mode with a different testType and company rather than erroring. Both new guards were negative-tested by injecting a duplicate and a non-contiguous bucket. build/scripts Pester suite: 124/124 pass. Note: with four buckets each bucket becomes app-bound rather than tenant-bound (work/6 is 26-32 min against a largest app of 45.6-51.7), so numberOfTenantsForTesting should be re-tuned downward in a follow-up, and splitting Tests-Workflow / AlCosting becomes worthwhile for the first time. Left out here to keep this change to a single variable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Could not find a linked ADO work item. Please link one by using the pattern 'AB#' followed by the relevant work item number. You may use the 'Fixes' keyword to automatically resolve the work item when the pull request is merged. E.g. 'Fixes AB#1234' |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Buckets are separate jobs on separate runners with separate containers, so they run fully in parallel — run
30354787707used 44 distinct runners, with Bucket1 and Bucket2 starting in the same minute. Adding a bucket therefore buys real, contention-free capacity, which raising the tenant count inside one container does not.That distinction matters because the legacy stage has hit its contention ceiling. Measured in
30354787707, both buckets are already tenant-bound, and #9736 showed that adding parallelism inside a container now costs almost as much in per-test slowdown as it gains: splittingTests-ERMraised Bucket1's concurrency and made every other app in that bucket 10–19% slower, inflating bucket work +27% for a net −5%.The constraint that shapes the split
Only
LegacyTestsBucket1containers getadditionalDemoDataTypes: ["Standard","Evaluation"]. An app cannot move between the two classes without changing the dataset it tests against, so the classes cannot be freely rebalanced.Keeping every app in its current class, using measured per-app times from run
30354787707(W1):Two non-obvious results:
What
Bucket3is a sibling of Bucket1 and inherits itsadditionalDemoDataTypes;Bucket4is a sibling ofBucket2. No app changes demo-data class, so no app changes the data it runs against.Get-AppNamesForBucketalready resolves"LegacyTests-Bucket$N"generically, and the Uncategorized path discovers everyLegacyTests-*key.Tests-Workflow51.7Tests-SCM50.9AlCosting45.6Tests-SCM-Warehouse46.0Cost
Legacy jobs go from 44 → 88 (22 countries × 4). Roughly +87 runner-hours per build against the 129.2 measured today, because ~97 min of container setup is paid per job. This is the trade to weigh: it is the only lever that adds real capacity, and the only one that costs materially more compute.
Tests
Adds six Pester tests over the bucket configuration. These failure modes are silent, which is why they are worth guarding:
testTypeandcompanyName.Both new guards were negative-tested by injecting a duplicate app and a non-contiguous bucket number, and both failed as intended.
Get-AppNamesForBucketpreviously had no test coverage at all.build/scriptsPester suite: 124/124 pass.Follow-ups deliberately left out
Kept to a single variable so the next run measures the bucket change cleanly:
numberOfTenantsForTestingdownward. With four buckets each bucket becomes app-bound rather than tenant-bound —work/6is 26–32 min against a largest app of 45.6–51.7 — so tenants 5 and 6 are provably idle. The model puts 4–5 tenants at the same floor with materially less contention.Tests-Workflow(51.7 min) andAlCosting(45.6 min). In the current 2-bucket layout these have 30 and 38 min of slack and splitting them is worth nothing. Once buckets are this small they become the binding constraint, and splitting them pays for the first time.AlCostingis a single codeunit, so it needs a source-level split.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com