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

Use a consistent format when testing BigDecimal.to_s #48693

Merged
merged 1 commit into from Jul 12, 2023

Conversation

zzak
Copy link
Member

@zzak zzak commented Jul 8, 2023

The previous examples were causing failures in CI, after ruby/bigdecimal#264 was merged.

For example:

Failure:
NumericExtFormattingTest#test_default_to_fs [/rails/activesupport/test/core_ext/numeric_ext_test.rb:446]:
--- expected
+++ actual
@@ -1 +1,3 @@
-"10000 10.0"
+# encoding: US-ASCII
+#    valid: true
+"10 00010.0"

The recommendation was to adjust the test to deal with the upstream change more flexibly.

@zzak
Copy link
Member Author

zzak commented Jul 8, 2023

Ok, it seems like it's due to ruby/bigdecimal#264 👀

@matthewd
Copy link
Member

matthewd commented Jul 8, 2023

We can just switch to a number that formats consistently (0.100001 would probably do?). We don't have an opinion on the actual output string, just that our code ends up hitting upstream's implementation. We definitely should not imply that upstream should reconsider fixing their bug just because we had a test (not real code) that depended on it.

Honestly I'm not sure to_fs should even have this behaviour at all... it was necessary when we were overriding to_s, but I don't see why anyone would want to call our separate method with that argument.

The previous examples were causing failures in CI, after ruby/bigdecimal#264 was merged.

For example:

```
Failure:
NumericExtFormattingTest#test_default_to_fs [/rails/activesupport/test/core_ext/numeric_ext_test.rb:446]:
--- expected
+++ actual
@@ -1 +1,3 @@
-"10000 10.0"
+# encoding: US-ASCII
+#    valid: true
+"10 00010.0"
```

The recommendation was to adjust the test to deal with the upstream change more flexibly.
@zzak zzak changed the title Try to understand this flaky test Use a consistent format when testing BigDecimal.to_s Jul 9, 2023
@byroot byroot merged commit 50e3ae0 into rails:main Jul 12, 2023
9 checks passed
@zzak zzak deleted the bigdecimal-to_s branch July 12, 2023 07:13
zzak pushed a commit to zzak/rails that referenced this pull request Jul 12, 2023
Use a consistent format when testing BigDecimal.to_s
skipkayhil pushed a commit to skipkayhil/rails that referenced this pull request Jan 8, 2024
Use a consistent format when testing BigDecimal.to_s
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.

None yet

3 participants