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

Fabrication integration #30

Merged
merged 3 commits into from Oct 1, 2017
Merged

Conversation

@shkrt
Copy link
Contributor

@shkrt shkrt commented Sep 30, 2017

Purpose of this pull request:

Add Fabrication gem support for FactoryProf #22

What changes did you make? (overview):

  • extracted FactoryGirl patching method to separate class, alongside with newly created Fabrication patch;

  • unified FactoryProf#track method so it does not rely on arguments passed from FactoryGirl;

  • changed integration specs so they mimick factories created both by FactoryGirl and Fabrication;

  • changed FactoryProf's unit specs and refactored similar expectations logic;

  • added Fabrication fixtures;

Is there anything you'd like reviewers to focus on?

I assumed that one app may have bundled with both FactoryGirl and Fabrication. Respective integration spec reflects this case.

I did not include neither readme updates, nor changelog entries yet. I can get them ready if the reviewers would find this PR worthy to merge.

@shkrt shkrt force-pushed the shkrt:feature/integrate_fabrication branch from 21094f1 to 69aedb1 Sep 30, 2017
@shkrt shkrt changed the title Feature/integrate fabrication Fabrication integration Sep 30, 2017
@shkrt shkrt force-pushed the shkrt:feature/integrate_fabrication branch from 69aedb1 to 4ec15ee Sep 30, 2017
Copy link
Collaborator

@palkan palkan left a comment

Looks great! Thank you for your work!

Definitely worth to get merged) So feel free to update all the docs.

FactoryGirl.build_stubbed(:user)
User.first
expect(result.stacks.size).to eq 0
matcher :have_no_stacks do

This comment has been minimized.

@palkan

palkan Oct 1, 2017
Collaborator

I'd prefer no to dry out specs this way; that makes specs less readable (especially when matchers have hard-coded expectations). Sometimes duplication is not evil.

@@ -1,13 +1,19 @@
# frozen_string_literal: true

require "test_prof/factory_prof/factory_girl_patch"

This comment has been minimized.

@palkan

palkan Oct 1, 2017
Collaborator

Let's move patches requires to the corresponding factory_builders, since we apply them there.

@@ -5,7 +5,7 @@ module FactoryProf
# Wrap #run method with FactoryProf tracking
module FactoryGirlPatch
def run(strategy = @strategy)
FactoryProf.track(strategy, @name) { super }
TestProf::FactoryProf::FactoryBuilders::FactoryGirl.track(strategy, @name) { super }

This comment has been minimized.

@palkan

palkan Oct 1, 2017
Collaborator

Can't we just use FactoryBuilders::FactoryGirl without TestProf::FactoryProf::...?

@shkrt shkrt force-pushed the shkrt:feature/integrate_fabrication branch from 4ec15ee to 635a9d3 Oct 1, 2017
@shkrt
Copy link
Contributor Author

@shkrt shkrt commented Oct 1, 2017

@palkan Thanks for the review! I updated the PR according to your comments. Also readme and changelog entries added.

@palkan
palkan approved these changes Oct 1, 2017
@palkan palkan merged commit 3d46476 into test-prof:master Oct 1, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shkrt shkrt deleted the shkrt:feature/integrate_fabrication branch Oct 1, 2017
@palkan
Copy link
Collaborator

@palkan palkan commented Oct 1, 2017

Thank you!

I need your full name (in Russian) for CultOfMartians (if you don't mind)

@shkrt
Copy link
Contributor Author

@shkrt shkrt commented Oct 1, 2017

@palkan full name in Russian is Руслан Гафуров

@palkan
Copy link
Collaborator

@palkan palkan commented Oct 2, 2017

@shkrt http://cultofmartians.com/tasks/test-prof-fabrication.html

btw, how can I found on Twitter? I'd like to make an announcement

@shkrt
Copy link
Contributor Author

@shkrt shkrt commented Oct 2, 2017

@palkan Sorry, I have no Twitter account 😢

@palkan
Copy link
Collaborator

@palkan palkan commented Oct 2, 2017

You are lucky man) ok, no problem)

@palkan
Copy link
Collaborator

@palkan palkan commented Aug 2, 2019

Hey @shkrt!

Could you please send me your actual email address to palkan@evl.ms? We plan to send some goodies to the Cult participants)

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

Successfully merging this pull request may close these issues.

None yet

2 participants