CI: Consolidate GitHub Actions test workflows#38147
Conversation
6a020e9 to
56c19d9
Compare
Merge TestCosmos.yaml and TestSqlServer.yaml into a single Test.yaml workflow with dedicated jobs for Main, Cosmos, SqlServer, Sqlite, and Microsoft.Data.Sqlite. Additional fixes: - Use restore.cmd on Windows (restore.sh doesn't support MINGW/Git Bash) - Use portable shell commands for macOS compatibility - Conditionally exclude net481 TFM on non-Windows for Microsoft.Data.Sqlite tests - Disable assembly signing for F# test project on non-Windows (dotnet/fsharp#17451) - Fix ApiChief DecompilerFactory to resolve runtime assemblies on preview SDKs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
56c19d9 to
cd841bd
Compare
There was a problem hiding this comment.
Pull request overview
Consolidates provider-specific GitHub Actions test workflows into a single workflow, and updates a few test/tooling projects to make the new cross-platform CI matrix viable.
Changes:
- Replace
TestCosmos.yamlandTestSqlServer.yamlwith a unified.github/workflows/Test.yamlcontaining dedicated jobs for Main, Cosmos, SqlServer, Sqlite, and Microsoft.Data.Sqlite. - Adjust Microsoft.Data.Sqlite test projects to only target .NET Framework on Windows, enabling non-Windows CI runs.
- Add an ApiChief decompiler resolver to improve assembly reference resolution; disable F# assembly signing on non-Windows as a workaround.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.Data.Sqlite.Tests/Microsoft.Data.Sqlite.sqlite3mc.Tests.csproj | Make .NET Framework target conditional on Windows only. |
| test/Microsoft.Data.Sqlite.Tests/Microsoft.Data.Sqlite.sqlite3.Tests.csproj | Make .NET Framework target conditional on Windows only. |
| test/Microsoft.Data.Sqlite.Tests/Microsoft.Data.Sqlite.Tests.csproj | Make .NET Framework target conditional on Windows only. |
| test/EFCore.FSharp.FunctionalTests/EFCore.FSharp.FunctionalTests.fsproj | Disable signing on non-Windows (workaround for upstream F# issue). |
| eng/Tools/ApiChief/Format/DecompilerFactory.cs | Use a UniversalAssemblyResolver and add runtime directory to resolve references. |
| EFCore.slnx | Update workflow file entries to the consolidated Test.yaml. |
| .github/workflows/TestSqlServer.yaml | Removed (replaced by consolidated workflow). |
| .github/workflows/TestCosmos.yaml | Removed (replaced by consolidated workflow). |
| .github/workflows/Test.yaml | New consolidated workflow with dedicated jobs/matrices per provider area. |
| push: | ||
| branches: | ||
| - main | ||
| - feature/* | ||
| - release/* | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| - feature/* | ||
| - release/* |
There was a problem hiding this comment.
I'd prefer to run it just on PRs, but on all branches
There was a problem hiding this comment.
but on all branches
I'm not sure, but I think that means if we push branches to the repo in order to create PRs out of them, we get two CI builds for the same thing, which isn't great... Also, a reason to run on push-to-main branch (and not just on PRs) is to have checks associated with the actual commits in main, e.g. when looking at the commit history on github etc. (and there's probably no real downside). On the other hand it's true that we never do feature/*.
Let me know what you think...
There was a problem hiding this comment.
I'm not sure, but I think that means if we push branches to the repo in order to create PRs out of them, we get two CI builds for the same thing, which isn't great...
It would mean that they will run on PR's targeting other branches, so you'll still get them when not using stacked PRs
Also, a reason to run on push-to-main branch (and not just on PRs) is to have checks associated with the actual commits in main, e.g. when looking at the commit history on github etc. (and there's probably no real downside).
The downside is that we waste resources on redundant work. And if they fail, how would we know?
There was a problem hiding this comment.
I'm not sure, but I think that means if we push branches to the repo in order to create PRs out of them, we get two CI builds for the same thing, which isn't great...
It would mean that they will run on PR's targeting other branches, so you'll still get them when not using stacked PRs
That's true, but as I wrote it has the downside of running twice for PRs submitted from branches pushed to the repo (that's truly double work/wasted resources). And I don't remember when we ever last did stacked PRs.
BTW re stacked PRs, there's this github feature in private preview, which I think would solve the problem you're referring to in a better way.
Also, a reason to run on push-to-main branch (and not just on PRs) is to have checks associated with the actual commits in main, e.g. when looking at the commit history on github etc. (and there's probably no real downside).
The downside is that we waste resources on redundant work. And if they fail, how would we know?
I don't think it's redundant - IMHO it's important to be able to look at the git history of the repo and see red vs. green checkmarks next to each commit (and be able to click in and see those builds).
Let's start out with a compromise :) I'll add pull_request for all branches as you want (for stacked PRs), and keep push for main only... We can always change it later.
There was a problem hiding this comment.
That's true, but as I wrote it has the downside of running twice for PRs submitted from branches pushed to the repo (that's truly double work/wasted resources).
Only if you keep the push triggers.
BTW re stacked PRs, there's this github feature in private preview, which I think would solve the problem you're referring to in a better way.
Yes, that's what I am referring to
IMHO it's important to be able to look at the git history of the repo and see red vs. green checkmarks next to each commit (and be able to click in and see those builds).
We already have that with the rolling build on CI
Let's start out with a compromise :) I'll add pull_request for all branches as you want (for stacked PRs), and keep push for main only... We can always change it later.
Sure
e594e07 to
14761c9
Compare
| <File Path=".github/workflows/inter-branch-merge-flow.yml" /> | ||
| <File Path=".github/workflows/TestCosmos.yaml" /> | ||
| <File Path=".github/workflows/TestSqlServer.yaml" /> | ||
| <File Path=".github/workflows/Test.yaml" /> |
There was a problem hiding this comment.
EFCore.slnx now includes .github/workflows/Test.yaml, but that workflow file doesn't exist in this PR (the added workflow is .github/workflows/Build.yml). This leaves the solution item pointing at a missing file; either rename Build.yml to Test.yaml or update the solution entry to the correct path.
| <File Path=".github/workflows/Test.yaml" /> | |
| <File Path=".github/workflows/Build.yml" /> |
| name: Build | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
| pull_request: | ||
|
|
There was a problem hiding this comment.
PR description/title indicate consolidating into a single Test.yaml workflow, but this PR adds .github/workflows/Build.yml (workflow name is also Build). If the intent is to replace the removed TestCosmos.yaml/TestSqlServer.yaml with Test.yaml, please align the workflow filename and/or update the PR description (and the EFCore.slnx entry) so they reference the same workflow file.
Merge
TestCosmos.yamlandTestSqlServer.yamlinto a singleTest.yamlworkflow with dedicated jobs for each test area: