-
-
Notifications
You must be signed in to change notification settings - Fork 396
Add specs for Random::Formatter#alphanumeric #1222
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
Conversation
|
There are failures on Ruby 3.0. Could you please take a look? |
library/random/alphanumeric_spec.rb
Outdated
| to_s.should_receive(:to_s).and_return("[mock to_s]") | ||
| # Using 1 value in chars results in an infinite loop | ||
| Random.alphanumeric(1, chars: [to_s, to_s]).should == "[mock to_s]" | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems chars could contain more than 1 character and all of them will be used, not the first one. But documentation states that The argument chars specifies the character list .... As for me it's worth adding a dedicated test case for it.
99662b8 to
05ac137
Compare
e8cde3a to
f2b7732
Compare
f2b7732 to
af091b9
Compare
| require_relative '../../../spec_helper' | ||
|
|
||
| ruby_version_is "3.1" do | ||
| require 'random/formatter' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
I've considered something like this:
ruby_version_is ""..."3.1" do
require 'securerandom'
end
ruby_version_is "3.1" do
require 'random/formatter'
end
describe "Random::Formatter#alphanumeric" do
# ...
endDoes it make sense?
| @object.extend(Random::Formatter) | ||
| @object.define_singleton_method(:bytes) do |n| | ||
| "\x00".b * n | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion:
def @object.bytes(n)
"\x00".b * n
endThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it seems the #bytes method is a contract/interface between the Random::Formatter module and a class that includes it. So it might make sense to state it explicitly with a dedicated test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding random generation in these specs. "\x00".b * n seems to simple and is lacking randomness. So specs may not detect an issue with implementation.
With current #bytes method version:
@object.alphanumeric # => "AAAAAAAAAAAAAAAA"
@object.alphanumeric(chars: ['a', 'b']) # => "aaaaaaaaaaaaaaaa"So I would consider something more random:
def @object.bytes(n)
(0..255).to_a.take(n).map { |b| b.chr }.join.b
endor even call Array#shuffle on (0..255).to_a.
Another one for Ruby 3.3 (#1216), but this adds some specs for earlier Ruby versions too.
There are more methods defined on RandomFormatter, we should probably add specs for the remaining methods too.