spec icon indicating copy to clipboard operation
spec copied to clipboard

Convert specs for OpenSSL::Digest to shared specs with Digest

Open herwinw opened this issue 2 years ago • 1 comments

This is going to take a bit of time, so I would like to have an approval for the idea before I actually spend a few hours on this.

Ruby has two libraries to calculate digests: digest and openssl (or the classes Digest en OpenSSL::Digest). The tests for openssl-digest currently use a lot of the constants of the digest, but they could use the specs as well. The current pull request is a little proof of concept for a single spec.

That brings us to the next point: the documentation of the OpenSSL::Digest class is weird. When I look at https://www.rubydoc.info/stdlib/openssl/OpenSSL/Digest, the only documented method is digest. https://docs.ruby-lang.org/en/3.2/OpenSSL/Digest.html describes a few more methods. But it turns out that the OpenSSL::Digest object had methods size and length too, similar to the Digest object. Or the OpenSSL::Digest object has a file method too, similar to Digest, but this is not documented. Should these undocumented methods be added to the specs too?

herwinw avatar Nov 08 '23 17:11 herwinw

Note that some/most of these methods are just from included modules:

irb(main):001:0> OpenSSL::Digest.ancestors
=> [OpenSSL::Digest, Digest::Class, Digest::Instance, Object, PP::ObjectMixin, Kernel, BasicObject]
irb(main):009:0> Digest::SHA512.ancestors
=> [Digest::SHA512, Digest::Base, Digest::Class, Digest::Instance, Object, PP::ObjectMixin, Kernel, BasicObject]

irb(main):006:0> Digest::Class.methods(false)
=> [:file, :base64digest, :digest, :hexdigest]
irb(main):007:0> Digest::Instance.instance_methods(false)
=> 
[:reset,                                                                 
 :digest_length,                                                         
 :update,                                                                
 :block_length,                                                          
 :new,                                                                   
 :digest!,                                                               
 :hexdigest!,                                                            
 :<<,                                                                    
 :==,                                                                    
 :inspect,                                                               
 :hexdigest,                                                             
 :to_s,                                                                  
 :file,                                                                  
 :digest,                                                                
 :length,                                                                
 :size,                                                                  
 :base64digest,                                                          
 :base64digest!]

So in terms of testing coverage I don't think it's so valuable to do that work, i.e., it should be enough to test Digest::Class and Digest::Instance methods once for a given digest, and just ensure all digests include those two modules.

In fact there are already some specs for Digest::Instance like library/digest/instance/new_spec.rb. They can just use Digest::MD5.new to have a concrete Digest to test. There doesn't seem to be specs for Digest::Class itself but several files under library/digest/ are actually testing Digest::Instance and Digest::Class methods. Ideally the specs would always be defined in a file matching the module owning them and not subclasses/classes including the module.

eregon avatar Nov 08 '23 18:11 eregon