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

Added specs and factory girl #284

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

Conversation

PraveenVignesh
Copy link

  • added factory_girl_rails gem
  • added faker gem
  • created factories for models
  • added specs for relationships
  • added specs for scopes in model

@courte
Copy link
Member

courte commented Dec 30, 2014

Thanks for adding these! I'm seeing a lot of style changes here and wanted to check -- could you take a moment to use Thoughtbot's Hound to do a quick style guide of this pull request and make sure it fits the standard Ruby guidelines?

We appreciate it, @PraveenVignesh!

@@ -3,7 +3,8 @@
describe ApplicationHelper do
describe '#display_url' do
it 'returns the domain portion of a given url' do
expect(helper.display_url('http://www.foo.com/bar/12')).to eq('www.foo.com/bar/12')
expect(helper.display_url('http://www.foo.com/bar/12'))
.to eq('www.foo.com/bar/12')

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@PraveenVignesh
Copy link
Author

Thanks @courte

Is everything fine now?

@courte
Copy link
Member

courte commented Feb 10, 2015

@PraveenVignesh - You're definitely closer! 👏 Check out the HoundCI comments above to see some of the remaining style guide issues.

Also, the testing area of the codebase has changed quite a bit since you first submitted. For example, factory-girl has already been added, along with a number of new specs. You may want to take a few steps to get your version up to date.

  1. From your own master branch, git pullYOUR_REMOTE_NAME_HEREmaster the HQ version of master
  2. git checkout your add-specs-and-factory-girl branch
  3. From there, run git rebase master and deal with any conflicts.
  4. Run git push -f origin add-specs-and-factory-girl to update your pull request.

That process will cut down on any work you might do fixing code that's not be relevant anymore. Let me know if you have any questions!


before(:each) do
@event1 = create(:event, start_date: Time.now,
end_date: Time.now + 1.day)

Choose a reason for hiding this comment

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

Align the elements of a hash literal if they span more than one line.

context 'scopes' do
before(:each) do
@organization1 = create(:organization, name: 'Spritle')
@organization2 = create(:organization, name: 'Foo')

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@PraveenVignesh PraveenVignesh force-pushed the add-specs-and-factory-girl branch from e83733b to f8c2086 Compare March 13, 2015 12:09
@organization1 = create(:organization, name: "Spritle")
@organization2 = create(:organization, name: "Foo")
@project1 = create(:project, organization_id: @organization1.id)
@project2 = create(:project, :inactive, organization_id: @organization1.id)

Choose a reason for hiding this comment

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

Line is too long. [81/80]

@PraveenVignesh PraveenVignesh force-pushed the add-specs-and-factory-girl branch 2 times, most recently from 7f6857c to 32e3cf3 Compare March 13, 2015 13:32
@@ -10,4 +10,24 @@
end_date { Time.new(2014, 10, 31) }
end
end

factory :cm_event, class: Event do
short_code { Faker::Lorem.word }

Choose a reason for hiding this comment

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

Unnecessary spacing detected.

@PraveenVignesh PraveenVignesh force-pushed the add-specs-and-factory-girl branch from 32e3cf3 to 84b7632 Compare March 13, 2015 13:50
more style fixes

added trait for job, event factories

added traits for project, style fixes, merge fixes

added trait for public event

fixed job factory
@PraveenVignesh PraveenVignesh force-pushed the add-specs-and-factory-girl branch from 84b7632 to 01869f3 Compare March 13, 2015 14:03
@PraveenVignesh
Copy link
Author

@courte I have fixed the style violations and rebase it with master. Please check this and give your feedback.

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

Successfully merging this pull request may close these issues.

3 participants