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.

Sign up for GitHub

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

Jump to bottom

Add the request spec helper method, #login_user. #57

Merged
merged 2 commits into from
Apr 12, 2018
Merged

Add the request spec helper method, #login_user. #57

merged 2 commits into from
Apr 12, 2018

Conversation

ebihara99999
Copy link
Contributor

There is no test helper for request specs now. It' not convenient officially recommending to write request specs to a new rails app.
If this PR is merged, I will add the documentation how to use the helper.

Thank you for review.

@ebihara99999
Copy link
Contributor Author

checks are failed...should I use the hash rocket?

@joshbuker
Copy link
Member

@ebihara99999 try merging/rebasing the upstream changes and see if that fixes travis. (Now that we've removed the older version on HEAD)

@ebihara99999 ebihara99999 force-pushed the add_request_test_hepler branch from 6782e3f to c1ca954 Compare August 4, 2017 14:22
@ebihara99999
Copy link
Contributor Author

@athix
Codacy failed because of using Ruby 2.1 parser. Would you tell me how to configure codacy not using ruby 2.1?

@ebihara99999 ebihara99999 mentioned this pull request Feb 3, 2018
Closed
@joshbuker
Copy link
Member

@ebihara99999 Sorry for the late reply.

I can't see an easy way to configure the ruby version from codacy, so I'm adding a rubocop config that should hopefully solve the issue. From there we should be able to rebase this branch and merge.

@joshbuker joshbuker mentioned this pull request Feb 3, 2018
Merged
@ebihara99999
Copy link
Contributor Author

@athix Thanks for dealing with it!

@ebihara99999 ebihara99999 force-pushed the add_request_test_hepler branch from c1ca954 to c265b7a Compare February 10, 2018 13:34
@ebihara99999
Copy link
Contributor Author

ebihara99999 commented Feb 10, 2018 •
edited
Loading

At first I rebased the upstream changes, but it kept failing. But I found it was able to be fixed by removing the self, which raises any errors. The cause of the codacy failure is redundant self, I should've come up with how to fix it. I appreciate #107 when you are busy. Anyway, it seems ready to be merged. Would you work on merging this?

joshbuker
joshbuker approved these changes Feb 10, 2018
View reviewed changes
Copy link
Member

@joshbuker joshbuker left a comment

Choose a reason for hiding this comment

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

Ideally I'd like to test this locally and see how it interacts with rspec and such, but I don't have the time currently. That said, this shouldn't be able to affect any existing projects negatively. I think we're good to go on merging here.

lib/sorcery/test_helpers/rails/request.rb
module Request
# Accepts arguments for user to login, the password, route to use and HTTP method
# Defaults - @user, 'secret', 'user_sessions_url' and http_method: POST
def login_user(user = nil, password = 'secret', route = nil, http_method = :post)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if secret being the password default makes the most sense here...I can't think of anything that would make more sense off the top of my head though, so I think it should be fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the password default value. I added modification from Integration#login, which forces us to use ‘secret’. I don’t think the deafault value isn’t needed because what is really needed is to choose the password value, and I will remove the value if necessary.

@joshbuker joshbuker requested a review from Ch4s3 February 10, 2018 22:03
@ebihara99999
Copy link
Contributor Author

I also have the same opinion for testing. I'm sorry I don't add the test.

But even more, IMO, I would like to give some modifications to tests now step by step if I have time. Sometimes testing is not easy because test codes are a little bit old.

@Ch4s3
Copy link
Contributor

Ch4s3 commented Mar 2, 2018 •
edited
Loading

This looks good to me., and you two should feel free to merge things like this without me if you want. I'm always happy to weigh in, but I don't want to hold everyone up just because I'm busy.

@ebihara99999
Copy link
Contributor Author

Thank you for saying that, even though I'm just a contributor and can't close issues and merge PRs, I feel free to move thing forward @Ch4s3

@anthony-robin
Copy link

anthony-robin commented Apr 11, 2018 •
edited
Loading

Any news on this ? 🙏

@anthony-robin anthony-robin mentioned this pull request Apr 11, 2018
Closed
@joshbuker joshbuker added the high priority Extra attention is needed label Apr 11, 2018
@joshbuker joshbuker merged commit d7d468a into Sorcery:master Apr 12, 2018
joshbuker added a commit that referenced this pull request Apr 12, 2018
@joshbuker
Update CHANGELOG.md
ddb73b5 
Add CHANGELOG entries for #56 and #57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ch4s3 Ch4s3 Ch4s3 approved these changes

@joshbuker joshbuker joshbuker approved these changes

Assignees
No one assigned
Labels
high priority Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ebihara99999 @joshbuker @Ch4s3 @anthony-robin