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

start use ffaker and refact some tests #95

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gabrielbaldao
Copy link

Resolves #1

Description

I'm trying improve the tests in this project with best pratices, we can improve together and write more tests.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not
    work as expected)

@gabrielbaldao gabrielbaldao force-pushed the start-use-ffaker-and-refact-some-tests branch 2 times, most recently from 5f61fcb to cadf7b5 Compare October 16, 2019 17:28
@gabrielbaldao gabrielbaldao force-pushed the start-use-ffaker-and-refact-some-tests branch from cadf7b5 to eb44ad9 Compare October 16, 2019 17:54
@cflipse
Copy link
Collaborator

cflipse commented Oct 24, 2019

Couple of things:

  • Can you give a quick explanation of the difference between faker and ffaker ?

  • Please keep commits separated into a single idea. If you're going to add a new gem, do that in a single commit, and a second -- or sequence of -- commit that modifies usage to use the new gem.

  • if you're going to remove a gem, that too should be it's own commit, once the migration commits have been applied. And you should probably check that it's not in use elsewhere. (this is a hint)

  • I acutally strongly prefer not to use the FactoryBot syntax mixins. The data generator source should be abundantly clear while reading the specs.

Otherwise, this looks good

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