Tests: Add 11 missing UnitConverters unit tests#35191
Conversation
Add [Theory] xUnit tests for FahrenheitToCelsius, CelsiusToFahrenheit, CelsiusToKelvin, KelvinToCelsius, MilesToMeters, DegreesPerSecondToHertz, RadiansPerSecondToHertz, HertzToDegreesPerSecond, HertzToRadiansPerSecond, KilopascalsToPascals, and HectopascalsToPascals. Increases test method count from 20 to 31, covering all public methods in UnitConverters.shared.cs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35191Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35191" |
There was a problem hiding this comment.
Pull request overview
Adds missing unit test coverage for several Microsoft.Maui.Media.UnitConverters conversion methods in Essentials, improving confidence in basic numeric conversion helpers.
Changes:
- Added theories for Fahrenheit↔Celsius and Celsius↔Kelvin conversions
- Added theories for Miles→Meters and angular velocity ↔ frequency conversions (deg/s, rad/s, Hz)
- Added theories for Kilopascals/Hectopascals → Pascals conversions
|
/review |
|
✅ Expert Code Review completed successfully! |
There was a problem hiding this comment.
Expert Code Review — 3-Reviewer Adversarial Consensus
Findings
🟡 MODERATE — PR description does not match the actual changes (3/3 reviewers)
The PR description claims tests were added for: CelsiusToKelvin, KelvinToCelsius, MilesToMeters, MetersToMiles, KilometersToMiles, MilesToKilometers, DegreesToRadians, RadiansToDegrees, PoundsToKilograms, KilogramsToPounds, AtmospheresToPascals.
However, comparing against the existing file and the implementation in UnitConverters.shared.cs:
| Claimed in description | Actual status |
|---|---|
MetersToMiles |
❌ Method does not exist in UnitConverters.shared.cs |
KilometersToMiles |
Already had tests |
MilesToKilometers |
Already had tests |
DegreesToRadians |
Already had tests |
RadiansToDegrees |
Already had tests |
PoundsToKilograms |
Already had tests |
KilogramsToPounds |
Already had tests |
AtmospheresToPascals |
Already had tests |
Tests actually added (8 of which are not mentioned in the description):
FahrenheitToCelsius, CelsiusToFahrenheit, CelsiusToKelvin, KelvinToCelsius, MilesToMeters, DegreesPerSecondToHertz, RadiansPerSecondToHertz, HertzToDegreesPerSecond, HertzToRadiansPerSecond, KilopascalsToPascals, HectopascalsToPascals
Recommendation: Update the PR description to accurately list the 11 methods being tested, and remove the reference to the non-existent MetersToMiles.
🟢 MINOR — MilesToMeters missing negative input case (2/3 reviewers)
See inline comment. The existing MilesToKilometers test includes [InlineData(-1, -1.6093)]; the new MilesToMeters test only covers non-negative values. Adding a negative case would maintain consistency.
🟢 MINOR — Frequency conversion tests could use more coverage (2/3 reviewers)
See inline comment. RadiansPerSecondToHertz has only 3 test cases (vs 4–5 for other tests), and the frequency conversion tests don't include negative inputs unlike other converter tests in the file.
Discarded — CelsiusToKelvin/KelvinToCelsius body-temperature case (1/3 reviewers, follow-up disagreed)
One reviewer suggested adding body-temperature anchor cases (37°C ↔ 310.15 K). Two follow-up reviewers independently disagreed: the existing test points (absolute zero, boiling, 0 K, room temp) already validate the linear offset formula adequately, and body-temp is meaningful for the Fahrenheit fractional multiplier but redundant for Kelvin's simple + 273.15.
Test Quality & CI Assessment
- ✅ All
InlineDatavalues verified mathematically correct against the implementation constants - ✅ Precision parameter (
, 4) is consistent with existing tests in the file - ✅ Test patterns (
[Theory]+[InlineData], parameter naming) follow established conventions - ✅ This is a test-only PR — no production code changes to validate
- i️ PR reports 168 tests passed, 0 failed
Methodology
3 independent reviewers with adversarial consensus. Disputed findings (flagged by only 1 reviewer) were evaluated by the other 2 reviewers as follow-up. Only findings with ≥2/3 agreement are included.
Generated by Expert Code Review for issue #35191 · ● 7.9M
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed review feedback:
|
> [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Summary Add unit tests for 11 `UnitConverters` methods that had no test coverage. New test cases cover: `FahrenheitToCelsius`, `CelsiusToFahrenheit`, `CelsiusToKelvin`, `KelvinToCelsius`, `MilesToMeters`, `DegreesPerSecondToHertz`, `RadiansPerSecondToHertz`, `HertzToDegreesPerSecond`, `HertzToRadiansPerSecond`, `KilopascalsToPascals`, and `HectopascalsToPascals`. ✅ `dotnet test src/Essentials/test/UnitTests/Essentials.UnitTests.csproj --filter UnitConverters` — 168 passed, 0 failed. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
> [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Summary Add unit tests for 11 `UnitConverters` methods that had no test coverage. New test cases cover: `FahrenheitToCelsius`, `CelsiusToFahrenheit`, `CelsiusToKelvin`, `KelvinToCelsius`, `MilesToMeters`, `DegreesPerSecondToHertz`, `RadiansPerSecondToHertz`, `HertzToDegreesPerSecond`, `HertzToRadiansPerSecond`, `KilopascalsToPascals`, and `HectopascalsToPascals`. ✅ `dotnet test src/Essentials/test/UnitTests/Essentials.UnitTests.csproj --filter UnitConverters` — 168 passed, 0 failed. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
> [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Summary Add unit tests for 11 `UnitConverters` methods that had no test coverage. New test cases cover: `FahrenheitToCelsius`, `CelsiusToFahrenheit`, `CelsiusToKelvin`, `KelvinToCelsius`, `MilesToMeters`, `DegreesPerSecondToHertz`, `RadiansPerSecondToHertz`, `HertzToDegreesPerSecond`, `HertzToRadiansPerSecond`, `KilopascalsToPascals`, and `HectopascalsToPascals`. ✅ `dotnet test src/Essentials/test/UnitTests/Essentials.UnitTests.csproj --filter UnitConverters` — 168 passed, 0 failed. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Summary
Add unit tests for 11
UnitConvertersmethods that had no test coverage.New test cases cover:
FahrenheitToCelsius,CelsiusToFahrenheit,CelsiusToKelvin,KelvinToCelsius,MilesToMeters,DegreesPerSecondToHertz,RadiansPerSecondToHertz,HertzToDegreesPerSecond,HertzToRadiansPerSecond,KilopascalsToPascals, andHectopascalsToPascals.✅
dotnet test src/Essentials/test/UnitTests/Essentials.UnitTests.csproj --filter UnitConverters— 168 passed, 0 failed.