Optimize CI by categorizing tests and parallelizing builds even more - #5715
Conversation
|
One thing I want to raise before this merges: backgrounding the build changes when we provision Azure resources. On master the build runs before any setup step, so a broken commit fails the job before If the runner is killed or times out, that teardown does not happen at all. This is exactly the case we deliberately avoided. We would rather create no resources for a build that cannot run tests than create them and clean them up afterwards. The fix is small. Move - name: Setup WSL
# ...SQL, PostgreSQL, RabbitMQ, IBM MQ, azure-cli install stay here, still overlapping the build...
- name: Wait for build
wait: build
- name: Azure login
if: matrix.test-category == 'AzureServiceBus' || matrix.test-category == 'AzureStorageQueues'
# ...Setup ASB / Setup ASQ...
- name: Run tests A failed build then fails the wait step and the job stops before login. The container categories keep the full overlap, since their infrastructure is local and never touches Azure. Only ASB and ASQ (4 jobs) lose it, roughly a minute each by my estimate, which puts it back around 9.5m instead of 8.6m. I think that is a fair price for keeping the invariant. |
38a27a7 to
de708ab
Compare
@danielmarbach this is now also fixed, and it seems we are still under 9mins |
c46f8d4 to
8a5b21b
Compare
| public async Task<NotificationsSettings> LoadSettings() | ||
| { | ||
| using var aggressivelyCacheFor = await Session.Advanced.DocumentStore.AggressivelyCacheForAsync(CacheTimeout); | ||
| // Deliberately not aggressively cached. These settings are read rarely and edited by hand, |
There was a problem hiding this comment.
This seems to be a change unrelated to this PR, should this be lifted to it's own change?
There was a problem hiding this comment.
It can be. This was found as part of this change, though
|
FYI #5722 |
This refactors the CI workflow to improve efficiency and reduce execution time. Test projects are now assigned `TestCategory` properties, enabling matrix jobs to dynamically select, build, and run only the relevant projects for their category. This significantly reduces build times and resource consumption. A new `compile` job ensures the entire `src` directory remains buildable. The build step for test jobs is now backgrounded to overlap with infrastructure setup, further improving parallelization. Also corrects inconsistent `PostgreSQL` casing to `PostgreSql` in the workflow.
Refines the CI workflow by breaking down the broad `Default` and `RabbitMQ` test categories into more granular ones. This addresses instances where these categories were running several slow assemblies in a single job. - `Default` is split into `DefaultCore`, `DefaultAudit`, and `DefaultMonitoring`. - `RabbitMQ` is split into `RabbitMQClassicConventional`, `RabbitMQClassicDirect`, `RabbitMQQuorumConventional`, and `RabbitMQQuorumDirect`. This change enables finer-grained parallelization of tests in the CI workflow, which reduces overall execution time and improves efficiency. It also standardizes test category declarations across projects using the `<TestCategory>` property and `IncludeInTestCategory` attribute, and updates CI conditions accordingly.
RabbitMQ now uses Particular/setup-rabbitmq-action@v2.0.0, which gained WSL support upstream, so no local action is needed for it.
The `ServiceControl.MultiInstance.AcceptanceTests` now explicitly creates both the primary and audit event sources on Windows. This ensures the necessary EventLog sources are available for the tests. Previously, the audit event source was implicitly created by other acceptance tests, which is no longer guaranteed due to CI changes like parallelization.
Notifications settings are rarely read and primarily edited by hand. Aggressive caching, which invalidates asynchronously via the Changes API, could lead to stale reads immediately following a save. This change ensures the latest settings are always retrieved, preventing potential consistency issues.
Improves the reliability of GitHub Actions for IBM MQ and RabbitMQ. IBM MQ's health check now directly probes the listener port, as Docker's internal check was insufficient for detecting connectivity issues, especially on Windows/WSL. RabbitMQ setup is made more robust by automatically restarting the container once if it hangs during boot within 90 seconds. Additionally, WSL memory allocation is increased to 8GB to provide more headroom and prevent resource-related failures during concurrent builds.
Moves the Azure Service Bus and Storage Queues setup steps to occur only after the build is successful. This prevents the creation of real cloud resources if the build fails, reducing unnecessary provisioning and potential orphaned resources.
Removes redundant `TestsFilter.cs` files and specialized `IncludeIn...TestsAttribute.cs` attributes. Test categories are now declared once via the `` MSBuild property in each test project, which `Directory.Build.props` uses to automatically generate the corresponding `IncludeInTestCategoryAttribute`. This ensures consistency between CI test filtering, which uses the MSBuild property, and local test runs filtered by `ServiceControl_TESTS_FILTER`, which relies on the assembly attribute. It also simplifies test project configuration by removing boilerplate files.
Introduces a build-time check in `select-test-projects.ps1` to prevent test projects from being silently ignored by CI if they reference the test SDK but do not declare a ``. Updates `README.md` with enhanced documentation on adding test projects and categories, utilizing GitHub Admonitions for improved clarity and visibility of important configuration details.
This dedicated action is no longer required, indicating a consolidation or replacement of messaging service setup within CI workflows.
Moves the `FakeTimeProvider` to a common test context and introduces a `StorableUtcNow` method. This ensures the `FakeTimeProvider` is initialized with a timestamp that respects database precision limits (e.g., PostgreSQL's microsecond precision), preventing mismatches and flakiness in persistence tests when comparing stored timestamps.
826b6ae to
bb6f0a1
Compare
| $connectString = "AccessKeyId=${{ secrets.AWS_ACCESS_KEY_ID }};SecretAccessKey=${{ secrets.AWS_SECRET_ACCESS_KEY }};Region=${{ secrets.AWS_REGION }};QueueNamePrefix=GHA-${{ github.run_id }}" | ||
| echo "ServiceControl_TransportTests_SQS_ConnectionString=$connectString" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf-8 -Append | ||
| # Everything above provisions local containers, so it overlaps the build freely. The Azure steps | ||
| # below create real cloud resources, so they sit behind the wait: a background step only fails the |
There was a problem hiding this comment.
This is so annoying with many LLMS. They go very verbose for little value
Why
CI took ~14.3 minutes wall clock. The assumption was that Windows runners are just slow, so pre-compiling on Linux would fix it. Measuring run 31189671891 showed the real cause was redundant work:
dotnet build srcbuilt all 76 projects in all 25 jobs: 180-281s on WindowsAssert.Ignore, so irrelevant ones still spin up. Windows-AzureServiceBus burned 73s of its 372s on thisServiceControlInstaller.Packagingneeds itBut the dependency closures are tiny: a transport test project needs 6 of 76 projects.
A cross-OS binary artifact was considered and rejected. It makes the build a serial stage (est. ~12.4m, barely better) and risks the
net10.0-windowsWPF/WinUI projects, Linux ELF apphosts, and absolute Linux NuGet paths inruntimeconfig.dev.json.Result
Windows-AzureServiceBus, the old bottleneck, went 14.3m → 7.0m.
What changed
Scoped builds. Each test project declares
<TestCategory>.tools/select-test-projects.ps1scans for it and generates atests.projtraversal file (usingMicrosoft.Build.NoTargets, already pinned inglobal.json), so a job builds only its category's closure.tools/run-tests.ps1then runs only that category's assemblies.Overlapped build and infrastructure, using the new parallel steps feature. The build is
background: trueand the job joins it atwait: buildafter the infra chain.backgroundrather than aparallel:group because the infra steps are an ordered chain (WSL before the database actions) that must run concurrently with the build, which a group can't express.Split the two fat categories.
Default→DefaultCore/DefaultAudit/DefaultMonitoring, andRabbitMQ→ one per routing topology. Both were single jobs running several slow assemblies back to back.Containerised the Windows infrastructure. Local
setup-rabbitmqandsetup-ibmmqactions run their Linux containers in WSL2 on Windows, replacing Azure Container Instances for RabbitMQ and enabling IBM MQ on Windows for the first time.Setup WSLis now gated to the categories that need it, andAzure logindropped to just the two categories that actually provision cloud resources.Added a
compilejob building all 76 projects on both OSes. Scoped builds mean projects outside every test closure (HealthCheckApp,Particular.PlatformSample.ServiceControl,LegacyArtifacts) would otherwise stop being compiled at all.Bugs found along the way
PostgreSQL tests had never run. The matrix used
PostgreSQL/PostgreSQLPersistence; the attributes declarePostgreSql/PostgreSqlPersistence; the comparison is ordinal. 310 persistence + 23 transport tests were silently skipped, and four jobs burned ~21 minutes testing nothing. They now run.Notifications settings could read stale after a write.
NotificationsManager.LoadSettingsread throughAggressivelyCacheForAsync(5 minutes), which invalidates asynchronously via the Changes API, so a read straight after a save could return the pre-save document. This is user-visible: save SMTP settings, re-read, see the old values. Removed.MultiInstance tests depended on assembly ordering. They host a primary and an audit instance but only pre-created the
ServiceControlevent source, relying onAudit.AcceptanceTests.RavenDBrunning earlier in the same job to create the audit one. Now explicit.Six required status checks point at job names this PR renames, so they will never report:
Linux-Default/Windows-Default-DefaultCore,-DefaultAudit,-DefaultMonitoringLinux-PostgreSQL/Windows-PostgreSQL-PostgreSql(casing)Linux-RabbitMQ/Windows-RabbitMQ-RabbitMQ{ClassicConventional,ClassicDirect,QuorumConventional,QuorumDirect}Worth also requiring the new
Linux-Compile/Windows-Compilejobs, since they are now the only thing compiling projects outside the test closures.Notes for reviewers
tools/run-tests.ps1replacesParticular/run-tests-action, which has no way to be told which projects to run. It should fold back into that action once it grows aprojectsinput.setup-rabbitmq/setup-ibmmqactions are deliberately self-contained so they can move to their ownParticular/setup-*-actionrepos. They have no teardown because composite actions can't declarepost:steps; hosted-runner VMs are destroyed anyway.concurrency, so two overlapping runs will queue. Addingcancel-in-progresson PR builds would likely free more capacity than this costs.