Skip to content

Conversation

diego-mathis
Copy link
Contributor

Q A
Is bugfix? ✔️/❌
New feature? ✔️/❌
Breaks BC? ✔️/❌
Tests pass? ✔️/❌
Fixed issues comma-separated list of tickets # fixed by the PR, if any

@schmunk42
Copy link
Contributor

1) yiiunit\framework\i18n\FormatterNumberTest::testIntlAsScientific
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-8.76543210987654E16
+8.765432109876540E16

Fails with the same error, maybe we should use 14 as precision value?

@diego-mathis
Copy link
Contributor Author

That is really strange. Seems its more difficult to fix.
With 14, I get this (I actually thought its 14, but only 15 did pass the test for me):

There was 1 failure:

1) yiiunit\framework\i18n\FormatterNumberTest::testIntlAsScientific
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-8.76543210987654E16
+8.7654321098765E16

With 15 it works.

@schmunk42
Copy link
Contributor

I think we should do the test with, i.e. precision 10 and the number formatter has to format to this length/precision.

@schmunk42
Copy link
Contributor

@samdark Maybe there should be an exception, when the precision is higher than the system can handle?

@diego-mathis
Copy link
Contributor Author

There are more problems with this.
Disabling intl extension gives:

1) yiiunit\framework\i18n\FormatterNumberTest::testIntlAsScientific
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-1.23E2
+1.230000E+2

E:\fire\yii2-new\tests\framework\i18n\FormatterNumberTest.php:519
E:\fire\yii2-new\vendor\phpunit\phpunit\phpunit:52

2) yiiunit\framework\i18n\FormatterNumberTest::testAsScientific
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-1.23E+2
+1.2E2

E:\fire\yii2-new\tests\framework\i18n\FormatterNumberTest.php:538

Then I tried (with intl active):

 $this->assertSame('8.765432109876544E16', $this->formatter->asScientific('87654321098765436'));

Notice the extra "4" 8.765432109876544E16

That did pass on my windows. And it would pass travis I guess.
But this doesn't feel satisfying. I believe this should be investigated some more.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants