Enable cloud-migration ReplicateData test (CU 135160 TestReplicateDataProperty)#9423
Enable cloud-migration ReplicateData test (CU 135160 TestReplicateDataProperty)#9423aholstrup1 wants to merge 12 commits into
Conversation
Remove Tests-NewObjects-Internal.DisabledTest.json to re-enable the previously disabled tests.
|
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' |
Copilot PR ReviewIteration 5 · Outcome: completed Knowledge source: https://github.com/microsoft/BCQuality@186d8a131465475c79244d994acb872cd5c0d4bf Orchestrator pre-filter (2 file(s) excluded)
Findings produced by the AL review agent v1.7.3. Reply 👎 on any inline comment to flag false positives. |
…crosoft/BCApps into aholstrup1-automatic-disco # Conflicts: # build/projects/Apps GB/.AL-Go/settings.json
github-actions
Bot
added
the
Build: scripts & configs
Convert the 7 permission-set checks in CU 132532 from fail-fast to accumulate-and-report so a single test run surfaces the complete list of violations instead of only the first one. Reuses the existing ArrayList + String.Join pattern already used by AllAppTablesAreInPermissionSet.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| begin | ||
| // If this test fails, it means that you added a local Table but forgot to add it to the local permission set | ||
| // If the table uses inherent permissions, it is excluded from this check by adding to GetTablesWithInherentEntitlements | ||
| Errors := Errors.ArrayList(); |
There was a problem hiding this comment.
AllLocalAppObjectsAreInLocalPermissionSet hand-rolls error aggregation with a DotNet ArrayList and String.Join instead of using the platform's ErrorBehavior::Collect feature.
This reimplements collection, loses each violation's ErrorInfo structure, and skips telemetry classification. The pattern is copied identically into 4 other test methods in this same codeunit (AllLocalAppObjectsAreInO365FullPermissionSet, AllLocalAppObjectsAreInO365BusFullPermissionSet, AllPermissionsAreObjects, VerifyTempPSinPS), each expanding this anti-pattern's footprint.
Recommendation:
- mark the per-item validation as a separate procedure attributed [ErrorBehavior(ErrorBehavior::Collect)] invoked via Codeunit.Run, then call HasCollectedErrors()/GetCollectedErrors(true) to build the final aggregate error.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| // If this test fails, it means that you added a local Table but forgot to add it to the O365 Full Access permission set | ||
| // To do this, open COD101982 and add the Object here. | ||
| // If the table uses inherent permissions, it is excluded from this check by adding to GetTablesWithInherentEntitlements | ||
| Errors := Errors.ArrayList(); |
There was a problem hiding this comment.
AllLocalAppObjectsAreInO365FullPermissionSet hand-rolls error aggregation with a DotNet ArrayList and String.Join instead of the platform's ErrorBehavior::Collect feature, reimplementing collection and losing per-violation ErrorInfo structure.
See the sibling finding on AllLocalAppObjectsAreInLocalPermissionSet in this same file for the recommended fix (per-item procedure attributed [ErrorBehavior(ErrorBehavior::Collect)], then HasCollectedErrors()/GetCollectedErrors(true)).
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| // If this test fails, it means that you added a local Table but forgot to add it to the O365 Bus Full Access permission set | ||
| // To do this, open COD101982 and add the Object here. | ||
| // If the table uses inherent permissions, it is excluded from this check by adding to GetTablesWithInherentEntitlements | ||
| Errors := Errors.ArrayList(); |
There was a problem hiding this comment.
AllLocalAppObjectsAreInO365BusFullPermissionSet hand-rolls error aggregation with a DotNet ArrayList and String.Join instead of the platform's ErrorBehavior::Collect feature, reimplementing collection and losing per-violation ErrorInfo structure.
See the sibling finding on AllLocalAppObjectsAreInLocalPermissionSet in this same file for the recommended fix.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| begin | ||
|
|
||
| // CopyAllAppTableObjectsToTempBuffer and CopyAllTablePermissionsToTempBuffer to contain correct ranges | ||
| Errors := Errors.ArrayList(); |
There was a problem hiding this comment.
AllPermissionsAreObjects hand-rolls error aggregation with a DotNet ArrayList and String.Join instead of the platform's ErrorBehavior::Collect feature, reimplementing collection and losing per-violation ErrorInfo structure.
See the sibling finding on AllLocalAppObjectsAreInLocalPermissionSet in this same file for the recommended fix.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| Errors: DotNet ArrayList; | ||
| String: DotNet String; | ||
| begin | ||
| Errors := Errors.ArrayList(); |
There was a problem hiding this comment.
VerifyTempPSinPS hand-rolls error aggregation with a DotNet ArrayList and String.Join instead of the platform's ErrorBehavior::Collect feature, reimplementing collection and losing per-violation ErrorInfo structure.
See the sibling finding on AllLocalAppObjectsAreInLocalPermissionSet in this same file for the recommended fix.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
Mirror the CU 132532 permission-set accumulate-and-report change into the CZ layer copy to satisfy MicroApp propagation validation. CZ's intentionally commented-out local permission-set check is left unchanged.
Multiple test methods in this codeunit now hand-roll error accumulation with a DotNet ArrayList (Errors.Add(...) inside a loop, then Error(String.Join(...)) at the end) instead of using the platform's ErrorBehavior::Collect feature.The referenced guidance calls this exact shape an anti-pattern: it reimplements the collection feature, loses each failure's ErrorInfo structure, and skips telemetry classification. The change is functionally reasonable (report every missing permission/table instead of stopping at the first), but the same outcome is achievable by marking a per-item check procedure ErrorBehavior::Collect, invoking it via Codeunit.Run per item, and aggregating with HasCollectedErrors()/GetCollectedErrors(true) as the good sample shows. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why |
Re-scope the PR to enable just the cloud-migration ReplicateData check (CU 135160 TestReplicateDataProperty) and keep the 10 Test Granules permission-set checks disabled for now. - Tests-NewObjects-Internal.DisabledTest.json: restore the 10 Test Granules entries, remove only TestReplicateDataProperty so it runs in CI. - CloudMigrationPropertyTest.Codeunit.al: add GetAppsPendingCloudMigrationProperty / IsTablePendingCloudMigrationProperty to temporarily exempt the 78 src/Apps apps that still ship tables with incorrect ReplicateData, so new regressions in other apps are still caught. List must burn down to zero. - TestMediaCleanup / RecordReferenceTest (System Application test tables): set ReplicateData = false directly instead of grandfathering, since they are outside src/Apps. - Revert the accumulate-and-report changes to Test Granules (W1 + CZ) since those tests stay disabled in this scope. - AL-Go-Settings.json: add src/DisabledTests/* to fullBuildPatterns so disabled-test edits trigger a full test run.
aholstrup1
changed the title
github-actions
Bot
added
Build: Automation
3 participants
What & why
Codeunit 135160 "Cloud Migration Property Test" has a single test,
TestReplicateDataProperty, which asserts that no table is included in cloud migration unless it is on the approved list — i.e. developers must setReplicateData = falseon tables that should not be migrated from on-prem. That test was on the CI disabled-test skip list, so it never ran, and a large backlog of tables shipping with the wrongReplicateDataproperty accumulated undetected.This PR re-enables only that test and introduces a mechanism to temporarily exclude the apps that currently still ship offending tables, so the check can be turned back on now while the debt is burned down per-app. The 10 Test Granules permission-set checks in the same test app stay disabled for now (separate follow-up).
What changed
src/DisabledTests/Tests-NewObjects-Internal/Tests-NewObjects-Internal.DisabledTest.json— restored with the 10 Test Granules methods still disabled; only theTestReplicateDataPropertyentry is removed, re-enabling that single test in CI.src/Layers/W1/Tests/NewObjectTests-Internal/CloudMigrationPropertyTest.Codeunit.al— adds an app-id exclusion mechanism (mirrors PR Re-enable Data Classification test (CU 135153 TestDataSensitivities) in CI #9114):GetAppsPendingCloudMigrationProperty— a temporary allow-list of app ids (78 apps) that still ship tables with incorrectReplicateData.IsTablePendingCloudMigrationProperty— resolves an offending table to its owning app viaAllObj."App Package ID"->NAV App Installed App."App ID"and skips it if the app is grandfathered.src/System Application/Test/.../TestMediaCleanup.Table.alandRecordReferenceTest.Table.al— setReplicateData = falsedirectly. These two System Application test tables are the only offenders outsidesrc/Apps, so they are fixed rather than grandfathered..github/AL-Go-Settings.json— addssrc/DisabledTests/*tofullBuildPatternsso edits to disabled-test lists trigger a full CI test run.Linked work
Fixes #
How I validated this
What I tested and the outcome
The grandfather app-id list was derived empirically from the actual
TestReplicateDataPropertyfailures across all 22 country builds of a prior CI run (543 distinct offending tables mapped to their owning apps). CI on this PR is the validation that the test now passes with the 78 apps grandfathered and the 2 System App tables fixed.Risk & compatibility
The exclusion list is a temporary bridge and must burn down to zero as each app sets
ReplicateData = falseon its offending tables. New tables in apps that are not on the list are still enforced, so this does not create a blind spot for other apps. No product runtime behavior changes.