Improving Marshal specs

[headius]

We recently received a bug report about improper linking during recursive Set dumping and loading (jruby/jruby#9405). The fix is simple, but attempting to add a spec for it brought to my attention how messy the existing Marshal specs are.

Many classes have custom spec blocks when they could be using the DATA or DATA_19 fixtures. Those fixtures currently appear to only be used for verifying loads; when I tried to use them to also verify the output of dumps, several of them fail:

1)
Marshal.dump 1..2 returns the expected output FAILED
Expected 
"\x04\bo:
Range\b:\texclF:
begini\x06:\bendi\a" == 
"\x04\bo:
Range\b:
begini\x06:\texclF:\bendi\a"
to be truthy but was false
/Users/headius/work/jruby/spec/ruby/core/marshal/dump_spec.rb:1011:in 'block (3 levels) in <top (required)>'
/Users/headius/work/jruby/spec/ruby/core/marshal/dump_spec.rb:6:in '<top (required)>'

2)
Marshal.dump 1...2 returns the expected output FAILED
Expected 
"\x04\bo:
Range\b:\texclT:
begini\x06:\bendi\a" == 
"\x04\bo:
Range\b:
begini\x06:\texclT:\bendi\a"
to be truthy but was false
/Users/headius/work/jruby/spec/ruby/core/marshal/dump_spec.rb:1011:in 'block (3 levels) in <top (required)>'
/Users/headius/work/jruby/spec/ruby/core/marshal/dump_spec.rb:6:in '<top (required)>'

...more

Both the load and dump specs have also gotten very large, making maintenance difficult. They contain logic specific to various other core types, and frequently hardcode marshal output strings that appear to have changed slightly over time.

I file this because I'm not sure the best course of action to clean this up. The specs I need for jruby/jruby#9405 would be small (and also the first Set-related Marshal specs), but I'm reluctant to just add another custom spec block for such simple cases. It would be nice to figure out the "right" way to handle Marshal dump and load specs so this doesn't continue to compound.

A few thoughts here:

• Perhaps the marshal logic for each core class should live in that class's spec directory? There could be some common fixture code to make it trivial, but even if they don't all override dump and load logic, they all have unique Marshal formats.
• Alternatively, could we clean up the canned DATA and DATA_19 hashes so that they fully pass for those cases, and only use custom specs when such canned cases can't be easily automated (such as recursive collections)?

[eregon]

I don't really see a big problem with e.g. https://github.com/ruby/spec/blob/master/core/marshal/dump_spec.rb.
Just because it's long doesn't mean it's a mess.
I think it's even pretty well organized with a group per class.
I would just add a case for Set there and in https://github.com/ruby/spec/blob/master/core/marshal/shared/load.rb.
It's simple and it works.
It would also take less time than filing or replying to this issue.

Many classes have custom spec blocks when they could be using the DATA or DATA_19 fixtures. Those fixtures currently appear to only be used for verifying loads; when I tried to use them to also verify the output of dumps, several of them fail:

Right, I see load uses those fixtures:

spec/core/marshal/shared/load.rb

Lines 426 to 437 in d3c94d1

	# Note: Ruby 1.9 should be compatible with older marshal format
	MarshalSpec::DATA.each do |description, (object, marshal, attributes)|
	it "loads a #{description}" do
	Marshal.send(@method, marshal).should == object
	end
	end
	MarshalSpec::DATA_19.each do |description, (object, marshal, attributes)|
	it "loads a #{description}" do
	Marshal.send(@method, marshal).should == object
	end
	end

But dump only checks the encoding:

spec/core/marshal/dump_spec.rb

Lines 1009 to 1013 in d3c94d1

	MarshalSpec::DATA_19.each do |description, (object, marshal, attributes)|
	it "#{description} returns a binary string" do
	Marshal.dump(object).encoding.should == Encoding::BINARY
	end
	end

It's been like that probably forever, at least for 13 years looking at the Blame:
https://github.com/ruby/spec/blame/d3c94d16969ffd6df88e25dd87b93e940c025a00/core/marshal/dump_spec.rb#L1009-L1013

It would be good to fix those to match what is currently emitted, if not already redundant with other specs.

BTW what does DATA and DATA_19 refer to? Marshal formats during Ruby 1.8/1.9?

I think in general for Marshal we want to test:

• What is the current output for dump
• All the valid cases for loads, and there can be multiple for the "same/equal" object because there has been multiple Marshal versions and even in the same Marshal version there have been changes.

It's great if you can clean these up, but I don't think it's any blocker, we have added many Marshal specs since then and it works well enough.