ruby / ruby/spec

Improving Marshal specs

Open
#1,355 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Ruby
Stars
622
Forks
402
Avg merge
14h 51m
Merged PRs (30d)
6

Description

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)?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reading spec/ruby/core/marshal/dump_spec.rb and the existing DATA and DATA_19 fixture usage, then reproduce the reported Range dump failures. Compare the current dump and load spec structure and determine how the fixtures should cover both directions, including the recursive Set case from jruby/jruby#9405. Done means the Marshal specs use a clear maintainable approach and the needed Set coverage is included without compounding custom blocks.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
testing-qa
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.