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

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

herwinw
Copy link
Member

@herwinw herwinw commented Nov 8, 2023

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?

@eregon
Copy link
Member

eregon commented Nov 8, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants