Update testing to only test .NET Standard and LTS versions #614
Conversation
…and .NET Core 2.1/3.1; refactor logo handling in tests
📝 WalkthroughWalkthroughUpdates the CI workflow test matrix; refactors tests to embed and load assets via new helper methods; standardizes culture handling and removes legacy NETCOREAPP1_1 conditionals; revises test project target frameworks and package references; adjusts project references; and trims Solution Items in the solution file. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
Once merged, #611 can be modified to use |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
QRCoderTests/QRCoderTests.csproj (1)
21-40: Package references look reasonable.The unified package reference approach simplifies the configuration. A few observations:
Microsoft.NET.Test.Sdk 17.3.3is from September 2022; newer versions are available.xunit 2.9.2is current.xunit.runner.visualstudio 2.4.3is older (from 2020); version 2.5.x or 2.8.x may be available.shouldly 4.2.1is from 2023.coverlet.collectorandcoverlet.msbuild 6.0.4are recent.System.Drawing.Common 6.0.0is appropriate for net6.0.System.ManagementandSystem.CodeDomwithExcludeAssetsbuildTransitive is a good practice to avoid transitive dependencies.Consider updating to the latest compatible versions of test packages for improved features and bug fixes:
#!/bin/bash # Check latest versions of test-related packages echo "Checking latest versions of test packages..." for package in "Microsoft.NET.Test.Sdk" "xunit.runner.visualstudio" "shouldly" "coverlet.collector" "coverlet.msbuild"; do echo "Package: $package" curl -s "https://api.nuget.org/v3-flatcontainer/$package/index.json" | jq -r '.versions[-1]' doneQRCoderTests/Helpers/HelperFunctions.cs (2)
92-105: Consider creating a defensive Bitmap copy to avoid potential stream disposal issues.While the PNG format typically loads all data into memory, creating a new Bitmap from the stream-loaded one ensures no references to the disposed stream remain. This pattern is already used in
BitmapSourceToBitmap(line 40).Apply this diff:
public static Bitmap GetIconBitmap() { var assembly = Assembly.GetExecutingAssembly(); var resourceName = "QRCoderTests.assets.noun_software engineer_2909346.png"; using (var stream = assembly.GetManifestResourceStream(resourceName)) { if (stream == null) throw new InvalidOperationException($"Embedded resource '{resourceName}' not found."); - return new Bitmap(stream); + using (var bmp = new Bitmap(stream)) + return new Bitmap(bmp); } }
98-98: Consider extracting resource names as constants.The resource names are hardcoded in multiple methods. Extracting them as
private constfields would improve maintainability if these resources are referenced elsewhere or if the names need to change.Example:
private const string IconPngResourceName = "QRCoderTests.assets.noun_software engineer_2909346.png"; private const string IconSvgResourceName = "QRCoderTests.assets.noun_Scientist_2909361.svg";Also applies to: 113-113, 132-132
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/wf-build-test.yml(1 hunks)QRCoder.sln(0 hunks)QRCoderTests/ArtQRCodeRendererTests.cs(1 hunks)QRCoderTests/Base64QRCodeRendererTests.cs(0 hunks)QRCoderTests/Helpers/HelperFunctions.cs(1 hunks)QRCoderTests/PayloadGeneratorTests/BitcoinAddressTests.cs(0 hunks)QRCoderTests/QRCodeRendererTests.cs(5 hunks)QRCoderTests/QRCoderTests.csproj(1 hunks)QRCoderTests/SvgQRCodeRendererTests.cs(6 hunks)
💤 Files with no reviewable changes (3)
- QRCoderTests/PayloadGeneratorTests/BitcoinAddressTests.cs
- QRCoderTests/Base64QRCodeRendererTests.cs
- QRCoder.sln
🧰 Additional context used
🧬 Code graph analysis (4)
QRCoderTests/SvgQRCodeRendererTests.cs (1)
QRCoderTests/Helpers/HelperFunctions.cs (3)
HelperFunctions(16-143)GetIconBytes(110-124)GetIconSvg(129-142)
QRCoderTests/QRCodeRendererTests.cs (2)
QRCoder/QRCode.cs (3)
QRCode(15-170)QRCode(21-21)QRCode(27-27)QRCoderTests/Helpers/HelperFunctions.cs (1)
HelperFunctions(16-143)
QRCoderTests/ArtQRCodeRendererTests.cs (2)
QRCoder/ArtQRCode.cs (3)
ArtQRCode(18-251)ArtQRCode(24-24)ArtQRCode(30-30)QRCoderTests/Helpers/HelperFunctions.cs (1)
HelperFunctions(16-143)
QRCoderTests/Helpers/HelperFunctions.cs (3)
QRCoder/ArtQRCode.cs (6)
Bitmap(37-38)Bitmap(45-46)Bitmap(62-129)Bitmap(138-162)Bitmap(208-232)Bitmap(281-290)QRCoder/QRCode.cs (5)
Bitmap(34-35)Bitmap(45-46)Bitmap(56-87)Bitmap(101-148)Bitmap(197-203)QRCoder/Extensions/StreamExtensions.cs (1)
CopyTo(9-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (20)
QRCoderTests/QRCoderTests.csproj (5)
3-3: LGTM! Target frameworks updated to match LTS and current versions.The framework list now includes .NET Framework 4.6.2, .NET Core 2.1, 3.1, and .NET 5.0/6.0 variants, aligning with the PR objectives and the workflow changes.
13-15: Good addition of build warning suppressions for EOL frameworks.The
SuppressTfmSupportBuildErrorsandCheckEolTargetFrameworkproperties appropriately suppress warnings about targeting end-of-life frameworks, which is necessary when maintaining backward compatibility.
17-19: LGTM! Pinning .NET Core 2.1 to the latest patch version.Setting
RuntimeFrameworkVersionto 2.1.30 ensures tests run against the final servicing release of .NET Core 2.1.
41-44: LGTM! Appropriate assembly references for .NET Framework 4.6.2.The explicit references to
PresentationCoreandWindowsBaseare necessary for WPF functionality on .NET Framework 4.6.2.
52-54: Excellent change to embedded resources!Switching from
CopyToOutputDirectorytoEmbeddedResourceeliminates file I/O dependencies and makes tests more portable and reliable. This aligns with the test code changes that now useHelperFunctions.GetIcon*()methods.QRCoderTests/QRCodeRendererTests.cs (5)
56-56: LGTM! Icon loading refactored to use embedded resources.The switch from
Image.FromFiletoHelperFunctions.GetIconBitmap()eliminates file I/O dependencies and makes tests more portable. The bitmap is still passed toGetGraphicwith the same expected behavior and hash validation.
68-68: LGTM! Consistent use of helper for icon loading.
82-82: LGTM! Consistent use of helper for icon loading.
96-96: LGTM! Consistent use of helper for icon loading.
110-110: LGTM! Consistent use of helper for icon loading.QRCoderTests/ArtQRCodeRendererTests.cs (1)
56-56: LGTM! Background image loading refactored to use embedded resources.The change from file-based loading to
HelperFunctions.GetIconBitmap()is consistent with the test suite refactoring. The bitmap is correctly passed as thebackgroundImageparameter toGetGraphic, maintaining the same test behavior.QRCoderTests/SvgQRCodeRendererTests.cs (6)
95-95: LGTM! PNG logo loading refactored to use embedded resources.The bitmap is correctly loaded via
HelperFunctions.GetIconBitmap()and passed toSvgLogoconstructor, maintaining the same test behavior while eliminating file I/O.
113-113: LGTM! Consistent use of helper for PNG logo loading.
131-131: LGTM! Consistent use of helper for PNG logo loading.
150-150: LGTM! PNG byte array loading refactored to use embedded resources.The change from
File.ReadAllBytestoHelperFunctions.GetIconBytes()eliminates file I/O while maintaining the same byte array input toSvgLogoconstructor.
168-168: LGTM! SVG logo loading refactored to use embedded resources.The switch from
File.ReadAllTexttoHelperFunctions.GetIconSvg()provides the SVG content from embedded resources, eliminating file I/O dependency.
186-186: LGTM! Consistent use of helper for SVG logo loading.QRCoderTests/Helpers/HelperFunctions.cs (3)
44-49: LGTM! Simplified preprocessor directives.The removal of NETFRAMEWORK and NETCOREAPP1_1-specific branches aligns with the PR objectives to drop legacy framework testing.
107-124: LGTM! Proper resource disposal and stream handling.The nested
usingblocks ensure both streams are properly disposed, and the pattern correctly loads the embedded resource into a byte array.
126-142: LGTM! Appropriate pattern for loading text-based SVG resource.The
StreamReaderapproach is correct for SVG content, and resource disposal is properly handled.
Shane32
changed the title
|
@gfoidl I'd like your input here. This PR should simplify testing libraries - now we can use any .NET Standard 2.0 libraries within the test project - but does mean that we can't test the .NET Framework 3.5 and .NET Standard 1.3 builds. Thoughts? My thought is that these are so far outdated anyway it doesn't matter. If someone runs into a bug on one of these frameworks, they can report it. And I'd drop these frameworks for the next major version. Users can continue to use QR Coder 1.x if they need .NET Framework 3.5 or .NET Core 1.x support |
gfoidl
left a comment
gfoidl
left a comment
There was a problem hiding this comment.
I'm always in favor of simplification.
.NET Standard 2.0 as baseline is sufficient. So I'm fine dropping the older test-targets.
drop these frameworks for the next major version.
👍🏻
For the current changes incl. changes in tests, we should increase at least the minor-version, to have some clear(er) distinction.
Agree. Typically I follow this pattern for other open source projects:
Generally I also allow bug fixes that could be considered a breaking change from an atypical viewpoint, but depending on the scenario, I may trigger a minor version bump in that scenario. Hard to give an example; it's pretty rare. In that scenario I'd point it out in the PR for you to review. Any code path that will trigger an exception (especially where it would always trigger an exception) is typically not a breaking change either. For instance, these scenarios are never considered breaking:
I try never to add dependencies within a major version, but we can discuss if there's a reasonable reason to do so. I try to only bump dependencies across minor versions where it is clear that the updated dependency does not place additional constraints on the developer -- and never bump dependencies across revisions. For example, if we target .NET 8, perhaps we bump System.Drawing.Common to the .NET 8 version - but only for the .NET 8 target, keeping the dependency as it was for other targets. We can discuss for anything unclear. |
Nothing unclear. This matches quite well how I'd like to do versioning. Maybe extract your last comment into a |
Changes:
This means:
Summary by CodeRabbit
New Features
Tests
Chores