Fix #1443: UnitParser.TryParse doesn't try with FallbackCulture (v6) #1466

lipchev · 2024-12-19T12:30:48Z

UnitParser: extracting a FindMatchingUnits function (used by both Parse and TryParse for a given abbreviation and culture)
UnitAbbreviationsCache: just a cosmetic change (flipping the order of the items in the tuple)
UnitParserTests: added the missing tests for the FallbackCulture
UnitTestBaseClassGenerator: improving the code-coverage of the ParseUnit and TryParseUnit tests (parsing without specifying a culture, which should always work with abbreviations for the FallbackCulture)

…ture - UnitParser: extracting a FindMatchingUnits function (used by both Parse and TryParse for a given abbreviation and culture) - UnitAbbreviationsCache: just a cosmetic change (flipping the order of the items in the tuple) - UnitParserTests: added the missing tests for the FallbackCulture

lipchev · 2024-12-19T12:32:31Z

UnitsNet.Tests/UnitParserTests.cs

        [InlineData("г", "ru-RU", MassUnit.Gram)]
        [InlineData("kg", "en-US", MassUnit.Kilogram)]
        [InlineData("кг", "ru-RU", MassUnit.Kilogram)]
+        [InlineData("kg", "ru-RU", MassUnit.Kilogram)] // should work with the "FallbackCulture"


This was passing before (but was never tested).

lipchev · 2024-12-19T12:33:28Z

UnitsNet.Tests/UnitParserTests.cs

+        [InlineData("г", "ru-RU", MassUnit.Gram)]
+        [InlineData("kg", "en-US", MassUnit.Kilogram)]
+        [InlineData("кг", "ru-RU", MassUnit.Kilogram)]
+        [InlineData("kg", "ru-RU", MassUnit.Kilogram)] // should work with the "FallbackCulture"


This was failing before (the issue mentioned in #1443)

lipchev · 2024-12-19T12:39:43Z

UnitsNet/CustomCode/UnitParser.cs

        /// <returns></returns>
-        public TUnitType Parse<TUnitType>(string unitAbbreviation, IFormatProvider? formatProvider = null) where TUnitType : Enum
+        public TUnitType Parse<TUnitType>(string unitAbbreviation, IFormatProvider? formatProvider = null)
+            where TUnitType : Enum


This is likely going to annoy some people (a little) but I really think we should make all Enum constraints have the struct, Enum - this makes sure that we're only calling this with a concrete type, disabling the potential call:
Enum whatType = UnitParser.Parse<Enum>(abbreviation, null);

I don't see why not, I think it was a mistake to not restrict to struct on all methods. I see only some of them have it.

I'll push that as a separate PR.

…ests

lipchev · 2024-12-19T18:04:55Z

In the "Test Session" try searching for TryParseUnitWithAmbiguousAbbreviation - there aren't that many..

lipchev · 2024-12-20T13:44:56Z

UnitsNet.Tests/GeneratedCode/TestsBase/VolumeTestsBase.g.cs

+        [InlineData("ru-RU", "Американская унция", VolumeUnit.UsOunce)]
+        [InlineData("fr-CA", "pmp", VolumeUnit.BoardFoot)]
+        [InlineData("fr-CA", "pied-planche", VolumeUnit.BoardFoot)]
+        [InlineData("fr-CA", "pied de planche", VolumeUnit.BoardFoot)]
+        public void ParseUnitWithCulture(string culture, string abbreviation, VolumeUnit expectedUnit)


If you want to make it really explicit- we could aslo include the non-localized mappings for (culture, englishAbbreviation, unit):

[InlineData("fr-CA", "ac-ft", VolumeUnit.AcreFoot)] [InlineData("fr-CA", "acre-foot", VolumeUnit.AcreFoot)] [InlineData("fr-CA", "acre-feet", VolumeUnit.AcreFoot)] ....

Change tests to control current culture: - Change `ParseUnit` to `ParseUnit_WithUsEnglishCurrentCulture` - Change `TryParseUnit` to `TryParseUnit_WithUsEnglishCurrentCulture` Add tests for more cases: - ParseUnit_WithUnsupportedCurrentCulture_FallsBackToUsEnglish - ParseUnit_WithCurrentCulture - TryParseUnit_WithUnsupportedCurrentCulture_FallsBackToUsEnglish - TryParseUnit_WithCurrentCulture

angularsen

Superb! I pushed a few minor improvements to the PR.

angularsen · 2024-12-26T17:27:23Z

UnitsNet.Tests/GeneratedCode/TestsBase/AccelerationTestsBase.g.cs

+        [InlineData("mg", AccelerationUnit.MillistandardGravity)]
+        [InlineData("nm/s²", AccelerationUnit.NanometerPerSecondSquared)]
+        [InlineData("g", AccelerationUnit.StandardGravity)]
+        public void TryParseUnit(string abbreviation, AccelerationUnit expectedUnit)


It was not obvious to me why this won't fail if the current culture is ru-RU, but I see from the test generator that it picks units uniquely defined by only the fallback culture.

Maybe add a comment or rename the test method to help point this out.

You can also manipulate the current culture per test, and revert it back, to make the intent more clear and avoid different behavior for different host systems running the tests.

I pushed some improvements 70f1b3e

Change tests to control current culture: - Change `ParseUnit` to `ParseUnit_WithUsEnglishCurrentCulture` - Change `TryParseUnit` to `TryParseUnit_WithUsEnglishCurrentCulture` Add tests for more cases: - ParseUnit_WithUnsupportedCurrentCulture_FallsBackToUsEnglish - ParseUnit_WithCurrentCulture - TryParseUnit_WithUnsupportedCurrentCulture_FallsBackToUsEnglish - TryParseUnit_WithCurrentCulture

You can also manipulate the current culture per test, and revert it back, to make the intent more clear and avoid different behavior for different host systems running the tests.

What, like the ToString method? Are you sure this is thread safe? I didn't want to risk another #438. I believe NUnit has annotations that can be used to set / ensure a given culture per test, but I don't know how this is supposed to be done properly in xUnit. Especially on a per-test level, I don't know if try / finally is the correct way to do it..

XUnit runs each test per test class sequentially, so a try-finally or using-scope pattern to revert changes work well.

However, XUnit will concurrently run test classes by default, unless a test fixture is used to control concurrency. This is not a problem because each test class is run on a different thread, and any changes to CurrentCulture or fields marked with [ThreadStatic] attribute will be isolated per test class.

The only thing I'm not 100% about is whether XUnit creates new threads per test class or uses som kind of pool that may be tainted, but in my experience this has never been a problem.

Oh, that's great- I've been tiptoeing around this issue for a long time..

So in what situation was the original issue occurring? There is no way that we could have been configuring something static on the per-test-class level..

Not sure what you mean, what issue exactly?
Something about CurrentCulture in tests that failed before?

Generally speaking, since tests can run on any developer or CI agent machine, it's good to control things like CurrentCulture to avoid flaky tests.

Nah, when I saw the [Collection(nameof(UnitAbbreviationsCacheFixture))] (and read on the related issues) I thought that this is something that is required every time we need to change something static..

Aha, I didn't remember we had those.

// Apply this collection fixture to classes: // 1. That rely on manipulating CultureInfo. See https://github.com/angularsen/UnitsNet/issues/436 // 2. To avoid accessing static ToString/Parse from multiple tests where UnitAbbreviationsCache.Default is modified

Nr 1 should not be a problem if you revert the value, not sure what happened back in 2018 but likely the order of test execution caused flakiness if we did not revert values.

Nr 2 is still required, since the static Default singleton and other static stuff is not marked [ThreadStatic]. If I recall correctly, a test project runs in the same app domain and share static fields, and most of our tests and test classes are in the same project.

Nr 2 is still required, since the static Default singleton and other static stuff is not marked [ThreadStatic]. If I recall correctly, a test project runs in the same app domain and share static fields, and most of our tests and test classes are in the same project.

Yeah, that's still a bit of a pain in the ass - wish there was a simple annotation that would make a given class run in total isolation, like in BenchmarkDotNet.

…or) (#1595) - Refactored the `try / catch` blocks for the `Parse` tests, similarly to the way this was handled in #1466 - added two tests for the `GetAbbreviation` method: `GetAbbreviationForCulture` and `GetAbbreviationWithDefaultCulture` --------- Co-authored-by: Andreas Gullberg Larsen <[email protected]>

lipchev commented Dec 19, 2024

View reviewed changes

Improve the test coverage for the static ParseUnit and TryParseUnit t…

dcd4fb0

…ests

lipchev commented Dec 20, 2024

View reviewed changes

angularsen approved these changes Dec 26, 2024

View reviewed changes

angularsen merged commit 9fbd6b4 into angularsen:release/v6 Dec 26, 2024
1 check was pending

lipchev mentioned this pull request Aug 3, 2025

Refactored the tests for Parse and TryParse (UnitTestBaseClassGenerator) #1595

Merged

Fix #1443: UnitParser.TryParse doesn't try with FallbackCulture (v6) #1466

Fix #1443: UnitParser.TryParse doesn't try with FallbackCulture (v6) #1466

Uh oh!

Conversation

lipchev commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lipchev commented Dec 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angularsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lipchev Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lipchev commented Dec 19, 2024 •

edited

Loading

lipchev Dec 26, 2024 •

edited

Loading