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

OmniAuth ready responses #507 #804

Closed
wants to merge 31 commits into from

Conversation

jesse-spevack
Copy link
Contributor

This pull request resolves OmniAuth ready responses #507

  1. Faker::Internet::Omniauth.google - Returns a randomized hash that mimics the auth hash described in the OmniAuth Google OAuth2 Strategy documentation
  2. Faker::Internet::Omniauth.linkedin - Returns a randomized hash that mimics the auth hash described in the Omniauth LinedIn documenation
  3. Faker::Internet::Omniauth.twitter - Returns a randomized hash that mimics the auth hash described in the Omniauth Twitter documentation
  4. Faker::Internet::Omniauth.facebook - Returns a randomized hash that mimics the auth hash described in the Omniauth Facebook documentation

My testing approach was to write one test for each fake omniauth provider and one assertion for each key of each omniauth hash. Assertions range in specificity from least specific, checking data type, to most specific, checking value and or format.

I purposely chose to create an Omniauth subclass to the Internet class because first, that was described in the #507 issue description and also because Omniauth feels like a pretty niche use case that fits nicely under a more broad 'Internet' heading.

I strove for readability, though I recognize that I'm proposing adding a lengthy new class file. However, I think this is justified because most of the length is the natural result of the length of the authentication hashes being returned - as opposed to lengthy logic. Another approach would be to create separate modules for each omniauth type, but I think that adds unnecessary complexity at the scale of four omniauth providers. This might be an approach worth considering if future contributors build on this pull request and add additional providers.

@jesse-spevack
Copy link
Contributor Author

I was failing travis because of two things:

  1. I had a require 'pry' in a file.
  2. I had added a test to test_faker_internet.rb mistakingly

I fixed both issues and now it looks like this is passing Travis CI and ready for review.

Thanks for taking a look at this!

@stympy
Copy link
Contributor

stympy commented Feb 4, 2017

Merged in e69921a

@stympy stympy closed this Feb 4, 2017
@jesse-spevack
Copy link
Contributor Author

Thanks @stympy!

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

Successfully merging this pull request may close these issues.

2 participants