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

apologies for the late submit #47

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

elle-johnnie
Copy link

Lingering concerns: I was unable to get omniauth mock to work with a before do block so I called a login before every 'require login' test. This defies all intentions to keep it DRY :(. On many tests I kept getting an error that expected :success but actual response was :found. I changed those specific tests to found but I really don't understand why some of the tests passed with success while others didn't. Lastly, I also had trouble when specifying redirect paths in the test. Even though actions on localhost were performing as expected. For those I changed test expectation to a more generic: 'must respond with redirect'. Any input/advice on the above is appreciated!

@dHelmgren
Copy link

MediaRanker Revisted

What We're Looking For

Feature Feedback
General
Appropriate Tests on WorksController yes
Appropriate Tests on SessionsController yes
Appropriate Tests on UsersController yes
Completed OAuth Yes
Basic Authorization (who can see what) Yes
Overall See my comment, you did a good job despite your before block snafu. I'm not sure what's happening with the found keyword, but I'll take a look at it in detail with Chris when I get the chance.

# Tell OmniAuth to use this user's info when it sees
# an auth callback from github
# call test helper method mock_auth
OmniAuth.config.mock_auth[:github] = OmniAuth::AuthHash.new(mock_auth_hash(user))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So your befores weren't working because of a bug in Mini-Test, but here is where I would instead be calling perform_login.

Since before is broken, the best way to fix is to use let instead.

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