Update GitHub Actions workflow for testing code#31
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughCI workflows are modernized by bumping several action versions and switching the test runner from a pinned ChangesCI Workflow Updates
Test Suite: Drawing/Image Linux Skips and Test Adjustment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Identify all of the failed tests. Alter those tests so that they are skipped on Linux. (The tests should still run on Windows.) |
Agent-Logs-Url: https://github.com/Shane32/EPPlus/sessions/82b21e3a-2032-449f-bbcd-44030b17918b Co-authored-by: Shane32 <6377684+Shane32@users.noreply.github.com>
Done in commit 5d1aafb. I identified all six failing Linux tests from the workflow logs and updated them to call Assert.Inconclusive on Linux so they are skipped there while still executing on Windows. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/publish.yml (1)
43-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
actions/upload-artifactversion inconsistency — line 43 was not bumped.Within this same workflow, the NuGet packages upload step (line 43) still uses
actions/upload-artifact@v4, while the documentation upload step (line 66) was upgraded tov7. This leaves the workflow in a mixed-version state.🛠️ Proposed fix
- uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v7🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yml at line 43, The workflow has a mixed version for the upload-artifact action: the NuGet packages upload step still uses actions/upload-artifact@v4 while the docs upload step was bumped to v7; update the NuGet upload step (the uses: actions/upload-artifact entry) to actions/upload-artifact@v7 to match the other step, ensuring any inputs/flags used by the NuGet upload in that step remain compatible with v7 and adjust them if the action's interface changed.
🧹 Nitpick comments (1)
EPPlusTest/WorkSheetTests.cs (1)
43-46: ⚡ Quick winBlanket Linux skip in
RunWorksheetTests()over-skips non-drawing tests.The early return on lines 43–46 causes the entire
RunWorksheetTestsintegration suite — including unrelated tests likeInsertDeleteTestRows,LoadData,StyleFill,RichTextCells,CreatePivotTable, and ~25 others — to be skipped on Linux. Only the three drawing-specific calls (PictureURL,AutoFitColumns,SetHeaderFooterImage) actually require GDI+.The guard was likely added to prevent
AssertInconclusiveExceptionfrom propagating whenSetHeaderFooterImage()(which now has its own Linux guard) is called at line 91. The targeted fix is to remove the blanket guard fromRunWorksheetTests()and instead wrap only the drawing calls inline:♻️ Proposed targeted fix for `RunWorksheetTests()`
- if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) - { - Assert.Inconclusive("Skipping worksheet integration tests on Linux."); - } - InsertDeleteTestRows(); ... `#if` !NoDrawing - PictureURL(); + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + PictureURL(); `#endif` ... `#if` !NoDrawing - AutoFitColumns(); + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + AutoFitColumns(); `#endif` ... `#if` !NoDrawing - SetHeaderFooterImage(); + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + SetHeaderFooterImage(); `#endif`The individual Linux guards on
SetBackground()(line 2283) andSetHeaderFooterImage()(line 2298) remain necessary for direct[TestMethod]invocations and should be kept regardless.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@EPPlusTest/WorkSheetTests.cs` around lines 43 - 46, The RunWorksheetTests() method currently returns early on Linux, over-skipping many non-drawing tests; remove the blanket RuntimeInformation.IsOSPlatform(OSPlatform.Linux) guard in RunWorksheetTests() and instead wrap only the drawing-specific calls (PictureURL, AutoFitColumns, SetHeaderFooterImage) with individual Linux guards that call Assert.Inconclusive when on Linux; leave existing Linux guards on SetBackground() and the SetHeaderFooterImage() test method intact so direct [TestMethod] invocations still short-circuit on Linux.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/publish.yml:
- Line 43: The workflow has a mixed version for the upload-artifact action: the
NuGet packages upload step still uses actions/upload-artifact@v4 while the docs
upload step was bumped to v7; update the NuGet upload step (the uses:
actions/upload-artifact entry) to actions/upload-artifact@v7 to match the other
step, ensuring any inputs/flags used by the NuGet upload in that step remain
compatible with v7 and adjust them if the action's interface changed.
---
Nitpick comments:
In `@EPPlusTest/WorkSheetTests.cs`:
- Around line 43-46: The RunWorksheetTests() method currently returns early on
Linux, over-skipping many non-drawing tests; remove the blanket
RuntimeInformation.IsOSPlatform(OSPlatform.Linux) guard in RunWorksheetTests()
and instead wrap only the drawing-specific calls (PictureURL, AutoFitColumns,
SetHeaderFooterImage) with individual Linux guards that call Assert.Inconclusive
when on Linux; leave existing Linux guards on SetBackground() and the
SetHeaderFooterImage() test method intact so direct [TestMethod] invocations
still short-circuit on Linux.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0373f993-db9b-4a42-b829-3c1c6627c0ef
📒 Files selected for processing (6)
.github/workflows/build.yml.github/workflows/publish.yml.github/workflows/test-documentation.ymlEPPlusTest/DTS_FailingTests.csEPPlusTest/DrawingTest.csEPPlusTest/WorkSheetTests.cs
|
@copilot Adjust the tests to not run .NET Core 2.1 or .NET Core 3.1 tests while running on Linux. Adjust the workflow that runs tests to not install .NET Core 2.1 or 3.1 on the linux runner. |
Agent-Logs-Url: https://github.com/Shane32/EPPlus/sessions/f9c69196-04f3-4815-b2e1-ef557965f6cf Co-authored-by: Shane32 <6377684+Shane32@users.noreply.github.com>
Implemented in commit 2a84289. Linux now targets only net6.0/net8.0 in EPPlusTest.Core.csproj, and the test workflow on ubuntu-latest installs only .NET 6/8 (2.1/3.1 setup runs only on non-Linux runners). |
Co-authored-by: Shane Krueger <shane@acdmail.com>
|
@copilot fix build errors for the .net framework build: Error: D:\a\EPPlus\EPPlus\EPPlusTest\WorkSheetTests.cs(43,17): error CS0103: The name 'RuntimeInformation' does not exist in the current context [D:\a\EPPlus\EPPlus\EPPlusTest\EPPlusTest.Core.csproj::TargetFramework=net462] (check the csproj/workflow to see which platforms it's supposed to be running on) |
Agent-Logs-Url: https://github.com/Shane32/EPPlus/sessions/e37315fd-7014-4323-9307-31dcb58bbbc3 Co-authored-by: Shane32 <6377684+Shane32@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Shane32/EPPlus/sessions/e37315fd-7014-4323-9307-31dcb58bbbc3 Co-authored-by: Shane32 <6377684+Shane32@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Shane32/EPPlus/sessions/e37315fd-7014-4323-9307-31dcb58bbbc3 Co-authored-by: Shane32 <6377684+Shane32@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Shane32/EPPlus/sessions/e37315fd-7014-4323-9307-31dcb58bbbc3 Co-authored-by: Shane32 <6377684+Shane32@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Shane32/EPPlus/sessions/e37315fd-7014-4323-9307-31dcb58bbbc3 Co-authored-by: Shane32 <6377684+Shane32@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Shane32/EPPlus/sessions/e37315fd-7014-4323-9307-31dcb58bbbc3 Co-authored-by: Shane32 <6377684+Shane32@users.noreply.github.com>
... Fixed in commit 7cc2079. I removed direct RuntimeInformation/OSPlatform usage from the affected tests and switched those Linux skip guards to a shared TestPlatform helper that compiles on net462 while preserving Linux-only skip behavior on modern targets. |
Summary by CodeRabbit