-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Random Test Failure: Faker::Omniauth tests fail when generating a name with an apostrophe #2943
Comments
So currently when we run Faker::Internet.email(name: "Alexis O'MConnell")
#=> o_alexis_mconnell@zieme.example This might not be what we expect. But I'm not sure what is the correct representation for this? |
Is it |
@keshavbiswa exactly... So this generator will return all of these possible permutations of the username:
and the test regex is only validating these possibilities:
Here is the relevant code that generates these values within def email(name: nil, separators: nil, domain: nil)
local_part = username(specifier: name, separators: separators)
generate_domain = domain_name(domain: domain)
return [local_part, generate_domain].join('@')
end
def username(specifier: nil, separators: %w[. _])
return shuffle(specifier.scan(/[[:word:]]+/)).join(sample(separators)).downcase
end What's happening here is that This issue has what appears to be several cascading problems that could be solved different ways:
|
I think I'll try the 3rd approach, although apostrophe's are valid, it might still look weird for others, so maybe it's best to keep it simple? Also like you mentioned, it might not be a good idea to verify just any valid email address. |
On second thought, I feel even the 3rd approach might not be a good idea since we're not covering all the scenarios. Maybe we keep the apostrophe together, do we want |
Oh, this is an interesting issue. An apostrophe is not a valid character per Google so I don't think it makes sense keeping it around for the email. I think it makes sense to just drop the apostrophe when generating the email address and merge it into the last name. Although we could use
|
@stefannibrasil It is completely valid to include an apostrophe within an email address and I'd suggest keeping this character within the If you strip out the apostrophe you will still have to accommodate for random test failures when checking with the current Omniauth regex: def email_regex(first_name, last_name)
/(#{first_name}(.|_)#{last_name}|#{last_name}(.|_)#{first_name})@(.*).(example|test)/i
end You can get around this issue by also stripping apostrophes from the first and last names within the test data, but generally speaking I think it's an easier fix to include the apostrophe instead of stripping it. Keeping the apostrophe and fixing the broken test seems to be as simple as replacing |
Ah, thank you! I was mainly concerned about separating the apostrophe from the last name but confirmed that is handled in the linked PR. |
Describe The Bug
The
Faker::Omniauth
module sometimes initializes a last name with an apostrophe (such as O'Connell), which will generate an email address that fails the validation tests withinTestFakerInternetOmniauth
.To Reproduce
With Faker v3.3.1:
test/faker/default/test_faker_omniauth.rb
.Faker::Omniauth.linkedin(name: "Alexis O'Connell")
def email_regex(first_name, last_name)
(found within the test file, also below in the reproduction script).Use the reproduction script below to reproduce the issue:
Expected Behavior
The
TestFakerInternetOmniauth
tests should pass when generating an omniauth profile that includes an apostrophe in the name.Additional Context
This email validation issue was mentioned in PR #2940 (review).
The text was updated successfully, but these errors were encountered: