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

Conversation

Projects
None yet
2 participants
@shkrt
Contributor

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 from Feature/integrate fabrication to Fabrication integration Sep 30, 2017

@shkrt shkrt force-pushed the shkrt:feature/integrate_fabrication branch from 69aedb1 to 4ec15ee Sep 30, 2017

@palkan

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

Owner

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

Owner

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

Owner

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

This comment has been minimized.

Contributor

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 palkan:master Oct 1, 2017

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

This comment has been minimized.

Owner

palkan commented Oct 1, 2017

Thank you!

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

@shkrt

This comment has been minimized.

Contributor

shkrt commented Oct 1, 2017

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

@palkan

This comment has been minimized.

Owner

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

This comment has been minimized.

Contributor

shkrt commented Oct 2, 2017

@palkan Sorry, I have no Twitter account 😢

@palkan

This comment has been minimized.

Owner

palkan commented Oct 2, 2017

You are lucky man) ok, no problem)

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