Skip to content
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

MNT Fix test that didn't account for i18n spaces #611

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

GuySartorelli
Copy link
Member

Fixes https://github.com/silverstripe/silverstripe-graphql/actions/runs/11061549418/job/30734504257?pr=610

  1. SilverStripe\GraphQL\Tests\Schema\IntegrationTest::testDBFieldArgs
    Failed asserting that two strings are equal.
    --- Expected
    +++ Actual
    @@ @@
    -'2/29/20, 5:00 PM'
    +'2/29/20, 5:00 PM'

The date formatter used is IntlDateFormatter which sometimes uses narrow non-breaking spaces. These are valid, given the intention of using IntlDateFormatter is to give a format that is correct for the locale used on the front-end, and in some locales the narrow non-breaking space is correct.

This test doesn't have to care what specific spaces are used - just that the correct date is there in the correct format.

Issue

@GuySartorelli
Copy link
Member Author

lint failure is unrelated and will be fixed by #610

@@ -1434,8 +1434,8 @@ public function testDBFieldArgs()
$node = $result['data']['readOneDataObjectFake'] ?? null;
$this->assertEquals('This is a varchar field', $node['myField']);
$this->assertEquals('Saturday', $node['date1']);
$this->assertEquals('2/29/20, 5:00 PM', $node['date2']);
$this->assertEquals('5:00:00 PM', $node['date3']);
$this->assertMatchesRegularExpression('#2/29/20,\h5:00\hPM#u', $node['date2']);
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong, SapphireTest should be setting the i18n default locale to en_US.

Copy link
Member

Choose a reason for hiding this comment

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

Actually it might be html5 overriding it - https://github.com/silverstripe/silverstripe-framework/blob/5/src/Forms/DateField.php#L450

However there's a comment that this should force a particular standard of date formats, so even then we still shouldn't get thin spaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is draft for a reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on php/php-src#11600 it seems that there was an intentional change in "ICU" which seems to be what's used under-the-hood for the i18n date formatting. This change includes the narrow non-breaking spaces which seem like they're here to stay.

We'll need to update a bunch of tests to account for it, including this one.

@GuySartorelli GuySartorelli marked this pull request as ready for review September 30, 2024 00:56
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Linting failure

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Sep 30, 2024

#611 (comment)

lint failure is unrelated and will be fixed by #610

@emteknetnz emteknetnz merged commit cac855b into silverstripe:5 Sep 30, 2024
12 of 13 checks passed
@emteknetnz emteknetnz deleted the pulls/5/fix-test branch September 30, 2024 20:37
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