Only run PR checks on Ubuntu by default #3137
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR changes the default operating systems for CI tests from all platforms (Ubuntu, macOS, Windows) to Ubuntu only. The goal is to reduce compute usage and avoid rate limiting by running most tests exclusively on Linux, while preserving cross-platform testing for specific tests where it provides meaningful coverage.
- Removes Ubuntu-only OS specifications from many test files since Ubuntu is now the default
- Updates the default OS list in the sync script from all platforms to Ubuntu only
- Preserves multi-platform testing for specific tests like autobuild and file baseline export where platform differences matter
- Removes Windows-specific conditional logic that's no longer needed
Reviewed Changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pr-checks/sync.py | Changes default OS from all platforms to Ubuntu only and removes exclusion logic |
| pr-checks/checks/*.yml | Removes redundant Ubuntu-only OS specifications from 25+ test files |
| .github/workflows/*.yml | Removes macOS/Windows entries from generated workflow matrices |
| @@ -1,5 +1,6 @@ | |||
| name: "autobuild-action" | |||
| description: "Tests that the C# autobuild action works" | |||
| operatingSystems: ["ubuntu", "macos", "windows"] | |||
There was a problem hiding this comment.
Should the other two autobuild- workflows also run on macos? Or was there a specific reason those only run on linux and windows?
There was a problem hiding this comment.
I think just because it's very unlikely to run Java analysis on macOS.
There was a problem hiding this comment.
If the tests are mainly for direct tracing, would it make sense to switch to a different traced language where we're more likely to run on all platforms (e.g. C#)?
Or alternatively, if the main goal is to test direct tracing, does it matter that it's unlikely to run Java analysis on macOS?
There was a problem hiding this comment.
Can you open an issue about this? We want testing of both indirect and direct tracing, and I'd like to understand better why we disable the CLR tracer in this job.
There was a problem hiding this comment.
Could this test be combined with one of autobuild-direct-tracing*? Seems similar.
Alternatively, maybe we could combine the three build-mode tests in one workflow with a matrix (and conditional steps based on the build-mode) / sequential jobs.
There was a problem hiding this comment.
Could this test be combined with one of
autobuild-direct-tracing*? Seems similar.
I've combined with autobuild-direct-tracing now that we will use direct tracing by default. But with the specific working directory it might still be useful to check build-mode: autobuild respects the working directory.
Alternatively, maybe we could combine the three
build-modetests in one workflow with a matrix (and conditional steps based on the build-mode) / sequential jobs.
We could, but we could no longer generate the workflow, so won't do for now.
| @@ -1,6 +1,5 @@ | |||
| name: "Clean up database cluster directory" | |||
| description: "The database cluster directory is cleaned up if it is not empty." | |||
| operatingSystems: ["ubuntu"] | |||
There was a problem hiding this comment.
This seems like it could be useful across all platforms because of difference in how IO behaves. That said, it feels like this could easily just be tacked on to another test that already runs on all platforms.
There was a problem hiding this comment.
I'll preserve the existing behaviour here. It seems unlikely that this would break.
There was a problem hiding this comment.
Feels like this could be added on to some other test.
There was a problem hiding this comment.
True, but I'll leave this for future work.
There was a problem hiding this comment.
This can probably be removed in favour of unit test coverage (which may already exist).
There was a problem hiding this comment.
I think a test is useful, particularly since default setup relies on language aliases.
There was a problem hiding this comment.
Do we need this specific one for Rust?
There was a problem hiding this comment.
It provides some coverage against specific older versions. I'll keep for now.
Direct tracing is now enabled by default.
There isn't really anything platform-specific at the moment.
henrymercer
force-pushed
the
henrymercer/slim-pr-checks
branch
from
ad33e2f to
4082f8c
Compare
Continue running ones on macOS and Windows too where it's useful, but the default should be Linux only as most functionality is platform independent.
This should reduce unnecessary compute usage and help avoid rate limiting.
Risk assessment
For internal use only. Please select the risk level of this change:
Merge / deployment checklist