Cleanup CultureInfo tests by hughbe · Pull Request #6683 · dotnet/corefx

hughbe · 2016-03-05T12:50:10Z

Removed/merged duplicate tests
Renamed tests from "PostTest1" etc.
Fixes some todos for Windows 10 tests

Contributes to #2103

stephentoub · 2016-03-18T20:04:39Z

src/System.Globalization/tests/CultureInfo/CultureInfoClone.cs

-            CultureInfo myClone = myTestCulture.Clone() as CultureInfo;
-            Assert.True(myClone.Equals(myTestCulture));
-            Assert.NotSame(myClone, myTestCulture);
+            CultureInfo culture = new CultureInfo(name);


I realize that "" represents an invariant culture, but it'd be good not to lose the test for CultureInfo.InvariantCulture explicitly.

it would be if we explicitly use CultureInfo.InvariantCulture.Name for the sake of code readbility

stephentoub · 2016-03-18T20:11:51Z

A few comments/questions, otherwise LGTM.

hughbe · 2016-03-18T20:21:43Z

@stephentoub thanks for the comments, pushed a PR feedback commit

tarekgh · 2016-03-18T21:44:51Z

src/System.Globalization/tests/CultureInfo/CultureInfoCompareInfo.cs

-            CultureInfo myCultureInfo = new CultureInfo(expectedName);
-            CompareInfo myCompareInfo = myCultureInfo.CompareInfo;
-            Assert.True(myCompareInfo.Name.Equals(expectedName, StringComparison.OrdinalIgnoreCase));
+            // TOOD: Once #5463 is fixed, combine this into the InlineData for CompareInfo_Compare


I am not sure if Linux support es-es_tradnl. it is good we have issue to track the investigation.

stephentoub · 2016-03-22T14:55:43Z

@hughbe, could you rebase this to address the conflicts? Thanks.

hughbe · 2016-03-22T16:35:55Z

@stephentoub done :)

hughbe · 2016-03-22T17:27:07Z

Also, don't mean to sound pushy, but can we get this one merged before #7139 as that's gonna present more merge conflicts

stephentoub · 2016-03-22T17:32:10Z

Thanks, @hughbe. That's up to @tarekgh... while we do appreciate this test cleanup, that PR is fixing breaks that are preventing our Win7 runs from completing, so it takes priority. But if @tarekgh doesn't mind, he can merge this first and then address any conflicts.

hughbe · 2016-03-22T17:38:00Z

Makes sense, I'll fix the merge conflict once that ones merged. don't want to create extra work!

tarekgh · 2016-03-22T17:38:01Z

Yes I can take of the conflicts and Win7 failures after we merge this one. @hughbe could you please squash your commits into 1 commit?

@hughbe could you do that quickly as we need to get the changes as soon as possible?

hughbe · 2016-03-22T18:04:41Z

done, thanks a bunch

tarekgh · 2016-03-22T21:37:25Z

@hughbe because we are running out of time and I need to get my changes in soon, so I am going to merge my changes first and then we can merge your changes later after resolving any conflict. I can help in any of the issues that may pop up

tarekgh · 2016-03-22T21:43:24Z

@hughbe I have merged my changes so you may pull the changes and update yours. the network is very slow today and looks everything take forever to do it :-(

hughbe · 2016-03-22T21:44:18Z

Ah that's fine! I completely understand - your changes are defo more important :)

hughbe · 2016-03-23T00:07:32Z

@tarekgh fixed the merge conflict. It was actually incredibly easy as git resolved the merge conflict automatically. Sorry to delay your work when time may not be on your side ;)

tarekgh · 2016-03-23T00:25:26Z

@hughbe thanks a lot for following up. I am glad the merge was simple.

tarekgh · 2016-03-23T00:39:11Z

@hughbe to let you know, I have cloned your branch, built it and then ran the globalization test on Windows 7 machine and it passed. so we should be good.

hughbe · 2016-03-23T11:13:33Z

@tarekgh good to know

Just wondering why there are so many differences between Windows 7 and later versions of Windows? I'm surprised these were allowed as it seems the .NET management are extremely reticent to have breaking changes!

tarekgh · 2016-03-23T16:03:12Z

Just wondering why there are so many differences between Windows 7 and later versions of Windows?

Globalization in general depends on the running the OS to get the data (like culture data, Idn, Normalization...etc.). so in Windows 7 you can expect many of such data differ with later Windows versions and that cause the result we return will be different. do you remember when I said, I don't like to test against hardcoded data? that exactly the reason, this will cause to have our tests written with a lot of if (Platform.xxxx) and need to maintain that. let me know if you have any more question

hughbe · 2016-03-23T22:48:29Z

Thanks for the answer. I just have one follow-up? Are there any solutions to hardcoding data? Thanks

stephentoub · 2016-03-24T12:43:37Z

Failure in the unix legs:

20:30:34    System.Globalization.Tests.CultureInfoConstructor.Ctor_String_Invalid [FAIL]
20:30:34       Assert.Throws() Failure
20:30:34       Expected: typeof(System.Globalization.CultureNotFoundException)
20:30:34       Actual:   (No exception was thrown)
20:30:34       Stack Trace:
20:30:34             at System.Globalization.Tests.CultureInfoConstructor.Ctor_String_Invalid()

tarekgh · 2016-03-24T17:38:14Z

@hughbe

Are there any solutions to hardcoding data?

My idea was manually to read the data from the OS and compare the results with what the framework return. we don't have to compare data to data, but we can compare results to results. for example we can read the default short date format from the OS, then format some date with what we read and then compare the result with the framework short date formatted date. and so on.
we had some discussion before and there is of course pros and cons for any approach so my ask for now is reduce depending on the hardcoded data.

hughbe · 2016-03-24T19:53:43Z

Thanks for the answer!
I've pushed a commit hopefully fixing the broken Unix build

stephentoub · 2016-03-25T23:17:04Z

src/System.Globalization/tests/CultureInfo/CultureInfoCtor.cs

+
+            if (PlatformDetection.IsWindows && PlatformDetection.WindowsVersion < 10)
+            {
+            	Assert.Throws<CultureNotFoundException>(() => new CultureInfo("no-such-culture"));


Nit: indentation. Not worth fixing and restaring CI for if this is the only issue... can be addressed later.

stephentoub · 2016-03-28T19:14:44Z

@hughbe, this has conflicts now, I'm assuming from some of your other changes. Could you rebase again?

Some duplication was present between tests, so I deleted some test files Also contributes to #2103 Also fixes some todos to enabled some tests for Windows 10

hughbe · 2016-03-29T09:00:12Z

@stephentoub with pleasure, done

stephentoub · 2016-03-29T12:38:39Z

Test Innerloop OSX Debug Build and Test please
Test Innerloop OSX Release Build and Test please

hughbe · 2016-03-29T12:45:36Z

Test Innerloop OSX Debug Build and Test please
Test Innerloop OSX Release Build and Test please

I'm getting the impression the CI hates me! Maybe this is revenge for stressing it with 500K tests the other day!

stephentoub · 2016-03-29T12:46:58Z

It does seem to be holding a grudge ;)

…-culture-info Cleanup CultureInfo tests Commit migrated from dotnet/corefx@1d35de1

dnfclas added the cla-already-signed label Mar 5, 2016

stephentoub reviewed Mar 18, 2016
View reviewed changes

tarekgh reviewed Mar 18, 2016
View reviewed changes

stephentoub closed this Mar 23, 2016

stephentoub reopened this Mar 23, 2016

dnfclas added the cla-already-signed label Mar 23, 2016

stephentoub added the 2 - In Progress label Mar 23, 2016

stephentoub reviewed Mar 25, 2016
View reviewed changes

Cleanup CultureInfo tests

0cc9c88

Some duplication was present between tests, so I deleted some test files Also contributes to #2103 Also fixes some todos to enabled some tests for Windows 10

stephentoub closed this Mar 29, 2016

stephentoub removed the 2 - In Progress label Mar 29, 2016

stephentoub reopened this Mar 29, 2016

dnfclas added the cla-already-signed label Mar 29, 2016

stephentoub added the 2 - In Progress label Mar 29, 2016

stephentoub merged commit 1d35de1 into dotnet:master Mar 29, 2016

stephentoub removed the 2 - In Progress label Mar 29, 2016

hughbe deleted the globalization-tests-culture-info branch March 29, 2016 15:59

karelz modified the milestone: 1.0.0-rtm Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#6683 from hughbe/globalization-tests…

4a1a692

…-culture-info Cleanup CultureInfo tests Commit migrated from dotnet/corefx@1d35de1

Conversation

hughbe commented Mar 5, 2016

Uh oh!

stephentoub Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

tarekgh Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Mar 18, 2016

Uh oh!

hughbe commented Mar 18, 2016

Uh oh!

tarekgh Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Mar 22, 2016

Uh oh!

hughbe commented Mar 22, 2016

Uh oh!

hughbe commented Mar 22, 2016

Uh oh!

stephentoub commented Mar 22, 2016

Uh oh!

hughbe commented Mar 22, 2016

Uh oh!

tarekgh commented Mar 22, 2016

Uh oh!

hughbe commented Mar 22, 2016

Uh oh!

tarekgh commented Mar 22, 2016

Uh oh!

tarekgh commented Mar 22, 2016

Uh oh!

hughbe commented Mar 22, 2016

Uh oh!

hughbe commented Mar 23, 2016

Uh oh!

tarekgh commented Mar 23, 2016

Uh oh!

tarekgh commented Mar 23, 2016

Uh oh!

hughbe commented Mar 23, 2016

Uh oh!

tarekgh commented Mar 23, 2016

Uh oh!

hughbe commented Mar 23, 2016

Uh oh!

stephentoub commented Mar 24, 2016

Uh oh!

tarekgh commented Mar 24, 2016

Uh oh!

hughbe commented Mar 24, 2016

Uh oh!

stephentoub Mar 25, 2016

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Mar 28, 2016

Uh oh!

hughbe commented Mar 29, 2016

Uh oh!

stephentoub commented Mar 29, 2016

Uh oh!

hughbe commented Mar 29, 2016

Uh oh!

stephentoub commented Mar 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants