Skip to content

Some DateTimeTest format tests fail on the upcoming Fedora 39 #84763

Closed
#85102
#84963
Closed
Some DateTimeTest format tests fail on the upcoming Fedora 39#84763
#85102
#84963
Assignees
stephentoubtarekgh
Labels
area-System.Globalization
Milestone
8.0.0

Description

@tmds
tmds
opened on Apr 13, 2023
System.Tests.DateTimeTests.TryFormat_MatchesToString(format: \"f\")
System.Tests.DateTimeTests.TryFormat_MatchesToString(format: \"F\")
System.Tests.DateTimeTests.TryFormat_MatchesToString(format: \"g\")
System.Tests.DateTimeTests.TryFormat_MatchesToString(format: \"G\")
System.Tests.DateTimeTests.TryFormat_MatchesToString(format: \"t\")
System.Tests.DateTimeTests.TryFormat_MatchesToString(format: \"T\")
System.Tests.DateTimeTests.TryFormat_MatchesToString(format: \"U\")
System.Tests.DateTimeTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"f\")
System.Tests.DateTimeTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"F\")
System.Tests.DateTimeTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"g\")
System.Tests.DateTimeTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"G\")
System.Tests.DateTimeTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"t\")
System.Tests.DateTimeTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"T\")
System.Tests.DateTimeTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"U\")
System.Tests.DateTimeOffsetTests.TryFormat_ToString_EqualResults
System.Tests.DateTimeOffsetTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"f\")
System.Tests.DateTimeOffsetTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"F\")
System.Tests.DateTimeOffsetTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"g\")
System.Tests.DateTimeOffsetTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"G\")
System.Tests.DateTimeOffsetTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"t\")
System.Tests.DateTimeOffsetTests.ParseExact_ToStringThenParseExactRoundtrip_Success(standardFormat: \"T\")

It's likely there was a regression in #83589 which was not caught in CI.

The test failures are from our CI machine. I will reproduce the issue locally and share more information.

cc @tarekgh @omajid

Activity

  1. ghost added
    needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners
    on Apr 13, 2023
  2. ghost added
    untriagedNew issue has not been triaged by the area owner
    on Apr 13, 2023
  3. jeffschwMSFT
    added
    area-System.Globalization
    on Apr 13, 2023
  5. SingleAccretion
    removed
    needs-area-labelAn area label is needed to ensure this gets routed to the appropriate area owners
    on Apr 13, 2023
  6. tarekgh
    added this to the 8.0.0 milestone on Apr 18, 2023
  7. tarekgh
    removed
    untriagedNew issue has not been triaged by the area owner
    on Apr 18, 2023
  8. tarekgh
    self-assigned this
    on Apr 18, 2023

  9. tarekgh commented on Apr 18, 2023

    @tarekgh
    tarekgh
    on Apr 18, 2023
    Member

    The problem is in DateTime.TryFormat with Utf-8. This assumes all date/time pattern characters are ascii which is not the case as CLDR started to use U+202F as the space character. The following line should convert the character to Utf-8 and store it to the buffer instead of just casting it.

    runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeFormat.cs

    Line 730 in 9aefa9d

    result.Append(TChar.CastFrom(ch));

  10. stephentoub
    self-assigned this
    on Apr 18, 2023

  11. tmds commented on Apr 18, 2023

    @tmds
    tmds
    on Apr 18, 2023
    MemberAuthor

    I had tried to reproduce the issue in a Fedora 38, and a Fedora 39 container yesterday and to my surprise it didn't reproduce.

    The problem is in DateTime.TryFormat with Utf-8. This assumes all date/time pattern characters are ascii which is not the case as CLDR started to use U+202F as the space character. The following line should convert the character to Utf-8 and store it to the buffer instead of just casting it.

    Thanks for pointing me to the problem.
    This matches with the failures: they show / (0x2F) which is the lower byte of U+202F:

    Expected: ···ril 18, 2023 5:10:51 AM
Actual:   ···ril 18, 2023 5:10:51/AM

  12. tarekgh commented on Apr 18, 2023

    @tarekgh
    tarekgh
    on Apr 18, 2023
    Member

    I had tried to reproduce the issue in a Fedora 38, and a Fedora 39 container yesterday and to my surprise it didn't reproduce.

    I am able to produce the issue on edora 39. I just used Fedora:39 docker image

  13. tmds commented on Apr 18, 2023

    @tmds
    tmds
    on Apr 18, 2023
    MemberAuthor

    I am able to produce the issue on Fedora 39. I just used Fedora:39 docker image

    Let me try again.

    I'm using the image from registry.fedoraproject.org/fedora , which has a sha ae8f1aaa5e2b for Fedora 39.
    Are we using the same one?

  14. tarekgh commented on Apr 18, 2023

    @tarekgh
    tarekgh
    on Apr 18, 2023
    Member

    I just did docker pull fedora:39 and then manually added all needed dependencies to build the runtime repo.

  15. tmds commented on Apr 18, 2023

    @tmds
    tmds
    on Apr 18, 2023
    MemberAuthor

    We're using a different image:

    registry.fedoraproject.org/fedora            39     ae8f1aaa5e2b  33 hours ago  197 MB
docker.io/library/fedora                     39     9526ca27af97  5 weeks ago   196 MB

    I'm going to try building in the Docker Hub image, and see if that reproduces the issue for me as well.

  16. 2 remaining items

  17. tmds commented on Apr 18, 2023

    @tmds
    tmds
    on Apr 18, 2023
    MemberAuthor

    Aha, this does it:

    export LANG=en_US.UTF-8

  18. tarekgh commented on Apr 18, 2023

    @tarekgh
    tarekgh
    on Apr 18, 2023
    Member

    right, don't keep C locale as default. We use Invariant culture at that time which will not have U+202F in the time formats.

  19. tmds
    closed this as completedon Apr 18, 2023
  20. tmds
    reopened this on Apr 18, 2023

  21. tmds commented on Apr 19, 2023

    @tmds
    tmds
    on Apr 19, 2023
    MemberAuthor

    @tarekgh the parser issues occur when a non-breaking space occurs in the format string.

    The tests fail for AllowWhiteSpaces because the parser expects skipped whitespace to match with regular spaces only.
    We can allow it to match it also with the non-breaking spaces.

    What is probably not covered by any tests: what should happen (without AllowWhiteSpaces) when the format string has a non-breaking space, and the string being parsed has a regular, or a different type of non-breaking space at that position?

    (edit:) or is this fully covered by:

    runtime/src/libraries/System.Runtime/tests/System/DateTimeTests.cs

    Lines 2021 to 2023 in 33b1869

    [Theory]
    [MemberData(nameof(FormatAndParse_DifferentUnicodeSpaces_Succeeds_MemberData))]
    public void FormatAndParse_DifferentUnicodeSpaces_Succeeds(char formatSpaceChar, char parseSpaceChar)

  22. tarekgh commented on Apr 19, 2023

    @tarekgh
    tarekgh
    on Apr 19, 2023
    Member

    @tmds are you willing to open a PR for it? If not, I'll take care with it.

  23. tmds commented on Apr 19, 2023

    @tmds
    tmds
    on Apr 19, 2023
    MemberAuthor

    I was working on this patch: main...tmds:runtime:parse_nbsp.

    This fixes the FormatExceptions I had mentioned in #84963 (comment).

    If this looks like the way to go, I'll open up a PR.

  24. tarekgh commented on Apr 19, 2023

    @tarekgh
    tarekgh
    on Apr 19, 2023
    Member

    You may use Char.IsWhiteSpace instead of checking every character manually. And if you confirm this will fix the FormatException, then it should be good and go ahead open the PR. Thanks for the help with this issue.

  25. stephentoub
    closed this as completedin #84963on Apr 19, 2023
  26. tmds
    reopened this on Apr 20, 2023

  27. tmds commented on Apr 20, 2023

    @tmds
    tmds
    on Apr 20, 2023
    MemberAuthor

    Reopen to track fixing the FormatExceptions.

  28. ghost added
    in-prThere is an active PR which will close this issue when it is merged
    on Apr 20, 2023
  29. tarekgh
    closed this as completedin #85102on Apr 22, 2023
  30. ghost removed
    in-prThere is an active PR which will close this issue when it is merged
    on Apr 22, 2023
  31. ghost locked as resolved and limited conversation to collaborators on May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area-System.Globalization

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions