Skip to content

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

Merged
joshbuker merged 2 commits into
Sorcery:masterfrom
ebihara99999:add_request_test_hepler
Apr 12, 2018
Merged

joshbuker merged 2 commits into
Sorcery:masterfrom
ebihara99999:add_request_test_hepler

Conversation

@ebihara99999

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@joshbuker

Copy link
Copy Markdown
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
Copy Markdown
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?

@joshbuker

Copy link
Copy Markdown
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.

@ebihara99999

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

ebihara99999 commented Feb 10, 2018

Copy link
Copy Markdown
Contributor Author

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 left a comment

Copy link
Copy Markdown
Member

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.

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Ch4s3 commented Mar 2, 2018

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

anthony-robin commented Apr 11, 2018

Copy link
Copy Markdown

Any news on this ? 🙏

@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
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

Labels

high priority Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants