New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.Globalization.Tests.DateTimeFormatInfoFirstDayOfWeek.FirstDayOfWeek fails #30074

Closed
laurentkempe opened this Issue Jun 2, 2018 · 19 comments

Comments

Projects
None yet
5 participants
@laurentkempe
Copy link
Collaborator

laurentkempe commented Jun 2, 2018

During .NET Core Community online Hackathon - 6/2 I cloned the repo with latest commit then I run build.cmd and build-tests.cmd which failed with following error:

System.Globalization.Tests.DateTimeFormatInfoFirstDayOfWeek.FirstDayOfWeek fails with Expected: Sunday Actual: Monday

My operating system version: Windows 10 Pro 1803 OS Build 17134.81
Windows 10 Region is set to United States, Languages to English (United States) and I changed the first day of the week from Sunday to Monday.

When I changed the first day of the week to Sunday, the test passed!

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Jun 2, 2018

@ViktorHofer ViktorHofer removed their assignment Jun 2, 2018

@tarekgh

This comment has been minimized.

Copy link
Member

tarekgh commented Jun 2, 2018

I'll try to take a look on Monday but I am not expecting this test to fail if it is used to succeed before. what is exactly the /p:EnableDumpling=true we enabled here?

@tarekgh

This comment has been minimized.

Copy link
Member

tarekgh commented Jun 2, 2018

@ahsonkhan

This comment has been minimized.

Copy link
Member

ahsonkhan commented Jun 2, 2018

what is exactly the /p:EnableDumpling=true we enabled here?

I would be really surprised if that has any impact on the test results. That is mainly an infrastructure change to get dumps to get uploaded using the dumpling service in case of test hangs and crashes. We should look at the set of commits since it was last succeeding to now.

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Jun 2, 2018

I believe this is failing because of this and never worked with it:

I changed the first day of the week from Sunday to Monday.

@tarekgh

This comment has been minimized.

Copy link
Member

tarekgh commented Jun 2, 2018

Oh, I missed this phrase. then yes it is expected to fail if you changed the first day of the week. why do you need to change the first day of the week for this test?

@tarekgh

This comment has been minimized.

Copy link
Member

tarekgh commented Jun 2, 2018

I am closing this issue because it is not really a bug either in the product nor in the test. if you change the test, you'll need to change the expected results.

Also, you may want to look at the issue #28933 for supporting the ISO week if what you are trying to do is related to that.

@tarekgh tarekgh closed this Jun 2, 2018

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Jun 2, 2018

Shouldn't we support all possible machine configurations? We usually try to avoid adding tests that don't run on all possible environments.

@tarekgh

This comment has been minimized.

Copy link
Member

tarekgh commented Jun 2, 2018

This test is specific testing Invariant, en-US and fr-FR. so the environment shouldn't affect the test as these locales using the Gregorian calendar.

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Jun 2, 2018

The failing test isn't about Invariant but about CultureInfo("en-US"): http://source.dot.net/#System.Globalization.Tests/DateTimeFormatInfo/DateTimeFormatInfoFirstDayOfWeek.cs,15

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Jun 2, 2018

Oh sorry I misread your answer.

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Jun 4, 2018

@tarekgh we want any contributor to be able pull down the repo and all the tests will pass no matter what their locale is. But ideally even if they have non default locale settings, like en-US with a different day of the week. Is that feasible with our existing tests do you think? For this test, for example, it could just skip if the start day is non default.

@tarekgh

This comment has been minimized.

Copy link
Member

tarekgh commented Jun 4, 2018

@danmosemsft

This test is scoped to Invariant, en-US, and fr-FR. it is very unlikely anyone will change the first day of the week in their machine settings when having one of these cultures set as default locale. if you set the default locale to any other locale will not affect this test at all.

Now, if we want to be immune to anyone changing the first day of the week on their machine while having en-us or fr-FR, we can change the test to:

            yield return new object[] { new CultureInfo("en-US", false).DateTimeFormat, DayOfWeek.Sunday };
            yield return new object[] { new CultureInfo("fr-FR", false).DateTimeFormat, DayOfWeek.Monday };

which passing false to the culture constructor which forcing not to read any user override for that cultures.

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Jun 4, 2018

It isn't that unlikely thought is it since that's how this customer intentionally set it? @ViktorHofer maybe you could make that change.

@tarekgh

This comment has been minimized.

Copy link
Member

tarekgh commented Jun 4, 2018

It isn't that unlikely thought is it since that's how this customer intentionally set it?

It would be good to understand why he did that. why need to set the first day of the week to Monday for en-US? I am pretty sure this may cause some other issues for any apps assume or expect Sunday as the first day of the week. it will not hurt to update the test though.

@tarekgh tarekgh reopened this Jun 4, 2018

@tarekgh tarekgh added this to the 3.0 milestone Jun 4, 2018

@ViktorHofer

This comment has been minimized.

Copy link
Member

ViktorHofer commented Jun 4, 2018

It would be good to understand why he did that. why need to set the first day of the week to Monday for en-US?

I guess the simple answer is because you can on Windows. As long as this setting exists we should take it into consideration IMO.

@ViktorHofer maybe you could make that change.

sure.

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Jun 4, 2018

I use en-US on my machine so that date formats match the US but being from the UK I consider Monday the first day of the week - I admit I never bothered to change the OS setting. Outlook has its own setting.

@tarekgh

This comment has been minimized.

Copy link
Member

tarekgh commented Jun 4, 2018

I use en-US on my machine so that date formats match the US but being from the UK I consider Monday the first day of the week - I admit I never bothered to change the OS setting. Outlook has its own setting.

People want to get UK settings usually change their locale to en-GB :-) and not changing one setting from many different settings. anyway, I am fine with fixing the test.

@laurentkempe

This comment has been minimized.

Copy link
Collaborator Author

laurentkempe commented Jun 5, 2018

It would be good to understand why he did that. why need to set the first day of the week to Monday for en-US?

The reason is that I always install en-US machines and as a French the start of the week is Monday, so I change it this way because I want to see the Windows calendar starting on Mondays. Nothing really special at the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment