Add specs for Kernel#sprintf #537

andrykonchin · 2017-10-28T15:45:44Z

Related issue #68

Add specs for Kernel#sprintf and all related methods:

Kernel #printf/.printf/.sprintf
File, StringIO #printf
String#%

Not sure whether we should test ARGF.printf and IO#printf

andrykonchin · 2017-10-31T08:30:29Z

@eregon There are several issues marked with # strange behavior comments. Not sure they are bugs or just documentation isn't full

Looks like TravisCI integration is broken and build failed

andrykonchin · 2017-10-31T15:27:05Z

Build fails on AppVeyor on both 2.3 and 2.4

File#printf other formats c supports Unicode characters FAILED
Expected "\xD4\x86"
 to equal "\u0506"
C:/projects/spec-x948i/core/kernel/shared/sprintf.rb:413:in `block (4 levels) in <top (required)>'
C:/projects/spec-x948i/core/file/printf_spec.rb:4:in `<top (required)>'
Finished in 262.352770 seconds

Specs run successfully on OS X so it's maybe an issue with Windows.

andrykonchin · 2017-11-01T18:32:34Z

On TravisCI some some not relevant tests failed

eregon · 2017-11-02T17:00:09Z

core/file/printf_spec.rb

+require File.expand_path('../../../spec_helper', __FILE__)
+require File.expand_path('../../kernel/shared/sprintf', __FILE__)
+
+describe "File#printf" do


It's IO#printf:

[1] pry(main)> File.new("README.md").method :printf => #<Method: File(IO)#printf>

@eregon Got it

I checked owner for StringIO and have found it is IO::writable in Ruby 2.2 and IO::generic_writable in Ruby 2.3/2.4. It looks like an implementation detail and I wonder if I should test printf for both IO::writable and IO::generic_writable

StringIO.new.method(:printf).owner => IO::writable

Interesting.
Those modules are definitely internal, even though they show up in ancestors:

> StringIO.ancestors => [StringIO, IO::generic_writable, IO::generic_readable, Enumerable, Data, Object, PP::ObjectMixin, Kernel, BasicObject]

So I think it's best to share specs as possible and just have the specs in StringIO#printf, since modules above are internal.

Could you make this a spec for IO#printf?

@eregon do you propose to test only IO#printf or share the specs for ancestors too (File/StringIO)?

@eregon ping

eregon · 2017-11-02T17:05:09Z

That's a big and awesome PR!
It might take me a while to review these 1000 lines of spec, I'll try to review progressively.

Re Travis, it looks like a Travis issue to me.
Did you see it in other builds or is it spurious?

andrykonchin · 2017-11-03T08:40:11Z

@eregon

Did you see it in other builds or is it spurious?

I reproduce it constantly when restart builds. Looks like builds in other new PRs fail too

What do you think about failed build on AppVeyor and issue with file and encoding on Windows?

mjago · 2017-11-03T10:04:50Z

The same travis fail is happening on Ruby trunk too.

mjago · 2017-11-03T12:26:19Z

@eregon Looks like the CI failures are due to localhost resolving to ::1 (IPV6) which travis doesn't like [(such as here)

Ruby CI builds are failing for the same reason. Unfortunately sockets are well outside my comfort zone so I will not be able to resolve) 😊

eregon · 2017-11-04T18:41:26Z

I fixed Travis in 7d76b72, if you rebase there should not be bind(2) failures anymore.

andrykonchin · 2017-11-06T14:46:33Z

Looks like some strange warnings of rubocop failed the build on TravisCI.

eregon · 2017-11-06T21:40:14Z

Re RuboCop, we should disable Lint/FormatParameterMismatch for the file core/kernel/shared/sprintf.rb in .rubocop.yml. There are already examples in that file how to do that.
Here the warnings are not interesting since we want to test edge cases with sprintf & friends.

eregon

That's a great start!
That's smart, passing a lambda to it_behaves_like, well done.
Could you take a look at my comments and address them?

eregon · 2017-11-06T21:42:03Z

core/file/printf_spec.rb

+      end
+      string = File.read(@filename)
+
+      string


The string variable is not needed here.

eregon · 2017-11-06T21:43:41Z

core/kernel/printf_spec.rb

+
+        printf(format, *args)
+
+        $stdout = stdout


This line should be in ensure so that if printf raises we still get back a normal $stdout.

eregon · 2017-11-06T21:45:23Z

core/kernel/shared/sprintf_encoding.rb

+  end
+
+  it "returns a String in the same encoding as the format String if compatible" do
+    string = "%s".force_encoding(Encoding::KOI8_U)


Nice, I didn't know this encoding before :)

eregon · 2017-11-06T21:47:38Z

core/kernel/shared/sprintf_encoding.rb

+
+    result = format(string, argument)
+    result.encoding.should equal(Encoding::UTF_8)
+  end


What happens if format is KOI8_U with non-7-bit characters and the argument UTF-8 with non-7-bit characters?
Could you add a spec for that?
Maybe also one with incompatible encodings (e.g. UTF-16BE and a UTF-8).

eregon · 2017-11-06T21:50:50Z

core/kernel/shared/sprintf.rb

+      def obj.to_i;   0 end
+      def obj.to_int; 1 end
+
+      format("%b", obj).should == "1"


As a note, you could encode this constraint in the first spec with
obj.should_not_receive(:to_i).
But this if fine too.

eregon · 2017-11-06T22:18:52Z

core/kernel/shared/sprintf.rb

+          format("%020A", 196).should == "0X000000000001.88P+7"
+        end
+
+        it "uses radix-1 when displays negative argument as a two's complemen" do


eregon · 2017-11-06T22:21:17Z

core/kernel/shared/sprintf.rb

+    describe "d" do
+      it "converts argument as a decimal number" do
+        format("%d", 112).should == "112"
+        format("%d", -112).should == "-112"


It would be interesting to test %d with a Bignum (> 2**64)

eregon · 2017-11-06T22:22:31Z

core/kernel/shared/sprintf.rb

+
+        format("%1$*2$c", 97, 10).should == "         a"
+        format("%1$*2$p", [], 10).should == "        []"
+        format("%1$*2$s", "abc", 10).should == "       abc"


Ultimate corner-case, can we have format("%1$*2$s", "abc", -10)?

eregon · 2017-11-06T22:24:34Z

core/kernel/shared/sprintf.rb

+      # strange behavior
+      it "does not affect G format" do
+        format("%.10g", 12.1234).should == "12.1234" # expected "12.1234000000"
+        format("%.10g", 123456789).should == "123456789" # expected "1.2345700000e+08"


Sounds like a bug, could you report it? (I think a single issue asking multiple strange behaviors for sprintf is OK)

First case: is ok. g does not print trailing zeroes, but there's the # flag if you want that.
Second case: also ok. Precision is 10, the integer has 9 digits so fits without resorting to scientific notation. Use two more digits, or two less precision, to get scientific notation.

Doc says:

Convert a floating point number using exponential form
if the exponent is [...] or greater than or
equal to the precision, [...]

Thanks for the clarification.

eregon · 2017-11-06T22:26:17Z

core/kernel/shared/sprintf.rb

+      end
+
+      # strange behavior
+      # expected to call to_str and only then to_s


I think that's OK, see my comment above about to_s/to_str.

eregon · 2017-11-06T22:29:00Z

@marcandre If you have a bit of time, could you take a look at the cases @andrykonchin annotated as strange behavior and share your thoughts?

marcandre · 2017-11-06T23:55:54Z

Wow, ambitious PR!
I commented on the specific strange behaviour cases, HTH.

marcandre · 2017-11-06T23:57:10Z

@andrykonchin great job on doubting the results you get, btw. It's important to be on the lookout for bugs when spec'ing

andrykonchin · 2017-11-08T10:10:02Z

core/kernel/shared/sprintf_encoding.rb

+  # irb(main):020:0* sprintf "Ä %s".encode('windows-1252'), "Ђ".encode('utf-8')
+  #   Encoding::CompatibilityError: incompatible character encodings: Windows-1252 and UTF-8
+  # expected to return string in UTF-8 encoding
+  it "raises Encoding::CompatibilityError if cannot convert argument and format to any of encodings" do


@eregon There is another example of strange behavior

Mmh, so those encoding are compatible according to Encoding.compatible?:

> Encoding.compatible? "windows-1252", "UTF-8" => #<Encoding:UTF-8>

But "Ђ" cannot be encoded in Windows-1252:

[13] pry(main)> "Ђ".encode('windows-1252') Encoding::UndefinedConversionError: U+0402 to WINDOWS-1252 in conversion from UTF-8 to WINDOWS-1252 from (pry):12:in `encode'

@eregon right,

Feature is
it returns a String in the argument's encoding if format encoding is more restrictive.

In the example in comments Ruby can convert format string (in 'windows-1252') to utf-8 but doesn't do this.

I think it's basically a consequence that

> "Ä %s".encode('windows-1252') << "Ђ".encode('utf-8') Encoding::CompatibilityError: incompatible character encodings: Windows-1252 and UTF-8

I think Ruby never tries to re-encode a String, unless you use encode explicitly.
The resulting encoding seems therefore to be the resulting encoding of << each %s argument .

But you are right - it's responsibility of string concatenation algorithm.

@eregon maybe, but it looks like a bug to me, because here this feature works

UPD
Hm, according to this spec we should really get exception, but why we don't get it in "returns a String in the argument's encoding if format encoding is more restrictive" spec? UTF-8 string contains not ASCII characters

But you are right - it's responsibility of string concatenation algorithm

But here the "foo %s" is ASCII-only, so it works fine.
There is special treatment for Strings with only 7-bit characters for many String operations (in that case the Encoding usually doesn't matter, it's like if it was US-ASCII).

@eregon right

andrykonchin · 2017-11-08T10:10:43Z

@eregon done, except sharing

… u formats

andrykonchin · 2017-11-15T09:34:38Z

@eregon ping
I fixed all the notes. Have I missed anything?

eregon · 2017-11-15T19:55:14Z

We just to make the Windows CI (AppVeyor) pass :)

1)
File#printf other formats c supports Unicode characters FAILED
Expected "\xD4\x86"
 to equal "\u0506"
C:/projects/spec-x948i/core/kernel/shared/sprintf.rb:309:in `block (4 levels) in <top (required)>'
C:/projects/spec-x948i/core/file/printf_spec.rb:4:in `<top (required)>'
Finished in 339.136043 seconds

Probably the file should be opened with an encoding argument, since UTF-8 is not the default on Windows.

andrykonchin · 2017-11-16T13:52:41Z

@eregon done

eregon

@andrykonchin Thank you for this awesome PR!

eregon · 2017-11-17T18:30:58Z

Let's merge this!
@andrykonchin Sorry, I was fairly busy recently so I took a while to reply.
Should be much better from now on :)

andrykonchin changed the title ~~[WIP] Add specs for Kernel#sprintf~~ Add specs for Kernel#sprintf Oct 31, 2017

andrykonchin requested a review from eregon November 1, 2017 21:04

andrykonchin self-assigned this Nov 1, 2017

eregon reviewed Nov 2, 2017

View reviewed changes

andrykonchin added 12 commits November 5, 2017 00:16

Shared example: Add types

f374b92

Shared example: Add flags

fe58da2

Shared example: Add width

c56a709

Shared example: Add precision

fd09d4c

Shared example: Add reference by name

dae4656

Include shared example for Kernel#printf

df592ac

Include shared example for Kernel#sprintf

f3e9001

Include shared example for File#printf

64f8b88

Include shared example for String#%

85d314d

Include shared example for StringIO#printf

861f764

Adopt already existed test cases

1b37772

Remove excessive specs

4ea9662

eregon reviewed Nov 6, 2017

View reviewed changes

andrykonchin commented Nov 8, 2017

View reviewed changes

andrykonchin added 3 commits November 8, 2017 16:41

Code review. Minor issues

a23597d

Code review. Share test cases for b and B, g and G, e and E, d, i and…

e952a00

… u formats

Suppress Rubocop warnings

286459d

Fix File#printf specs running on Windows

b442931

eregon approved these changes Nov 17, 2017

View reviewed changes

eregon merged commit f2f23ad into ruby:master Nov 17, 2017

eregon mentioned this pull request Nov 17, 2017

printf/sprintf/String#% specs are very limited #68

Closed

andrykonchin deleted the add-specs-for-printf branch November 17, 2017 18:56

Add specs for Kernel#sprintf #537

Add specs for Kernel#sprintf #537

Conversation

andrykonchin commented Oct 28, 2017 • edited Loading

andrykonchin commented Oct 31, 2017 • edited Loading

andrykonchin commented Oct 31, 2017

andrykonchin commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon commented Nov 2, 2017

andrykonchin commented Nov 3, 2017 • edited Loading

mjago commented Nov 3, 2017

mjago commented Nov 3, 2017

eregon commented Nov 4, 2017

andrykonchin commented Nov 6, 2017

eregon commented Nov 6, 2017

eregon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon commented Nov 6, 2017

marcandre commented Nov 6, 2017

marcandre commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrykonchin Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

eregon Nov 8, 2017 • edited by andrykonchin Loading

Choose a reason for hiding this comment

andrykonchin Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrykonchin commented Nov 8, 2017

andrykonchin commented Nov 15, 2017

eregon commented Nov 15, 2017

andrykonchin commented Nov 16, 2017

eregon left a comment

Choose a reason for hiding this comment

eregon commented Nov 17, 2017

andrykonchin commented Oct 28, 2017 •

edited

Loading

andrykonchin commented Oct 31, 2017 •

edited

Loading

andrykonchin commented Nov 3, 2017 •

edited

Loading

andrykonchin Nov 8, 2017 •

edited

Loading

eregon Nov 8, 2017 •

edited by andrykonchin

Loading

andrykonchin Nov 8, 2017 •

edited

Loading