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

Add specs for Random::Formatter#alphanumeric #1222

Merged
merged 1 commit into from
Dec 7, 2024

Conversation

herwinw
Copy link
Member

@herwinw herwinw commented Nov 30, 2024

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.

@andrykonchin
Copy link
Member

There are failures on Ruby 3.0. Could you please take a look?

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
Copy link
Member

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.

@herwinw herwinw force-pushed the random_alphanumeric branch from 99662b8 to 05ac137 Compare December 2, 2024 09:42
@herwinw herwinw force-pushed the random_alphanumeric branch from f2b7732 to af091b9 Compare December 7, 2024 14:41
require_relative '../../../spec_helper'

ruby_version_is "3.1" do
require 'random/formatter'
Copy link
Member

@andrykonchin andrykonchin Dec 7, 2024

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
  # ...
end

Does it make sense?

@object.extend(Random::Formatter)
@object.define_singleton_method(:bytes) do |n|
"\x00".b * n
end
Copy link
Member

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
end

Copy link
Member

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.

Copy link
Member

@andrykonchin andrykonchin Dec 7, 2024

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
end

or even call Array#shuffle on (0..255).to_a.

@andrykonchin andrykonchin merged commit 818b5f5 into ruby:master Dec 7, 2024
14 checks passed
@herwinw herwinw deleted the random_alphanumeric branch December 7, 2024 17:44
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