-
Notifications
You must be signed in to change notification settings - Fork 750
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
create missing tests for groups form helper #1470
create missing tests for groups form helper #1470
Conversation
Thank you for opening this pull request with us! Be sure to follow our Pull Request Practices. Let us know if you have any questions on Slack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start! Thanks for taking the time to write tests :)
Left some feedback - let me know if there's any further questions! Awesome work!
Hi, @julianguyen, thank you for the feedback! I apologize for what is probably a dumb question, but I've never edited a pr before. How would I update my pull with any other pr's submitted in the last 24 hours so my branch is up to date, and then make your recommended changes to sync them back up to my pr? |
@scw248 Not a dumb question at all! So we've documented our recommended git workflow! You'll want to sync your fork to this repository. To update your PR with your changes, simply create more commits on top of the existing ones! If you need any more help, let's sync on Slack! :) |
@julianguyen thank you! |
@julianguyen, I believe I made all of the corrections properly as far as adding an extra 'end', removing white space, removing unnecessary variables, and replacing the inputs with concrete values. Could you please take a look at my updated commit? |
Hi @julianguyen, I just pushed a new set of code, but the circleci error seems to be with a different folder than what I'm working on: app/components/Story/index.jsx:30:16. Could you clarify if this is an error on my end or if this was an error from someone else's pr? |
@scw248 You're correct, looks like a bug was introduced! I just merged in a fix for it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! Thanks 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran your tests locally and it's actually failing. Please run rpec
locally and do not rely on CircleCI to run your tests.
@julianguyen Thank you for your help! I just committed again and have some additional issues with my code I'm working on. I tried running rspec after bundle install, but I kept getting this error: |
@scw248 What happens when you run |
@julianguyen, sorry for the delay. When I run the
It looks like the response of user: nqthqn on the below github issue might solve it, but let me know if you suggest I try something else. |
Thanks @julianguyen. I downloaded yarn and ran it, but am now getting engine 'node' incompatibilities. I apologize for all of the back and forth, but could you let me know if you have any time to connect on slack this week? |
Hi @julianguyen, so I've figured out why yarn wasn't building and had to update some node packages. I am down to two specs being incorrect, and they're for the same thing: ` 1) GroupsFormHelper#new_group_props has invalid arguments returns nil
` I can't quite seem to figure out why the test is running my valid_inputs and not catching nil. I'm going to submit my updated code now, but could you take a look? |
Hey @scw248 let's pair program on this! Let's coordinate on Slack when to do so :D |
efbb224
to
8338ba6
Compare
Hey @julianguyen, it looks like CircleCi is coming up with an error that is not appearing when I run Rspec locally. It appears to have a problem with what we changed the action name to as well as the value. Is this something that should be updated with CircleCi? Happy to work on it further locally if needed |
end | ||
end | ||
|
||
describe '#edit_group_props' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the test is failing cause of how we nested these tests. If you look at the CircleCI failure you'll see the test reads as #new_group_props #edit_group_props has invalid arguments returns correct props
. We need to move this describe block to level above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh gotcha, corrected! It looks like the CircleCi issue now is with the OmniAuth gem. It's suggesting that it be disabled until patch is available. Wane me to disable it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh :O We don't want to disable it though since it's needed for authentication.
How did you figure out it was an issue with this gem? Is there a page you can link me to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, no worries! I just saw it in the audit check if you scroll down in CircleCi: https://circleci.com/gh/ifmeorg/ifme/8697?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, that is a valid issue. I'll merge your tests since they pass! I'll take care of the gem vulnerability issue, thanks for calling it out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! Thank you for writing these tests!
Thank you for merging this pull request with us! If you haven't already, in another pull request, please add yourself to our Contribute page. |
@julianguyen Thank you very much for the opportunity, and your help! I look forward to contributing more soon! |
Description
I used the the form_helper_spec to model the groups_form_helper_spec file that I created. Tested that inputs are valid.
Corresponding Issue
#1408
Screenshots
Test Coverage
I apologize, but I'm not sure I understand what this field means.
✅
🚫
@julianguyen, could you let me know if I'm on the right track with this pr? I know there are probably a few details that I'm missing.