Skip to content

Conversation

@Jagailo
Copy link
Contributor

@Jagailo Jagailo commented Jun 10, 2023

I fixed all the tests that didn't pass due to a non-English operating system.
I didn't change the test logic.

I didn't make any changes to the core code of the library, so this PR doesn't need a separate version.

Also, I can't guarantee that I've fixed all cases of missing culture.
For example, the test Assert.False("5 m" != "6 м") could return a false result not because 5 != 6, but because meter is not a метр.

Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, just a few suggestions for you to consider.

@angularsen angularsen merged commit 4670911 into angularsen:master Jun 14, 2023
@angularsen
Copy link
Owner

angularsen commented Jun 14, 2023

Thanks!

I see your point that providing English in a test case, either via CurrentCulture or as method argument, does not help distinguish from the "default" culture it would fall back to if the machine is running say Norwegian culture that has no unit abbreviation translations. Only the number formatting would differ, which kind of gets tested for certain numbers, but it is not as obvious as seeing the unit abbreviations in the correct language.

Russian is more or less the only other language with translations, so it would be an improvement to use that instead. This probably goes for many of our tests, so I'd make that a separate PR since this was primarily about fixing tests on machines with. If you are interested in taking a look at that, let me know.

@Jagailo Jagailo deleted the tests-without-culture branch June 14, 2023 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants