Merge branch '52992-absorb-qa-product-qa' into 'master'

CE: Absorb product into factory

See merge request gitlab-org/gitlab-ce!22698

15 files changed, 55 insertions, 165 deletions

diff --git a/qa/qa.rb b/qa/qa.rb
index 7be7194f7b9..f00331dfe93 100644
--- a/qa/qa.rb
+++ b/qa/qa.rb
@@ -39,7 +39,6 @@ module QA
   module Factory
     autoload :ApiFabricator, 'qa/factory/api_fabricator'
     autoload :Base, 'qa/factory/base'
-    autoload :Product, 'qa/factory/product'
 
     module Resource
       autoload :Sandbox, 'qa/factory/resource/sandbox'
diff --git a/qa/qa/factory/README.md b/qa/qa/factory/README.md
index cfce096ab39..42077f60611 100644
--- a/qa/qa/factory/README.md
+++ b/qa/qa/factory/README.md
@@ -88,44 +88,6 @@ end
 The [`Project` factory](./resource/project.rb) is a good real example of Browser
 UI and API implementations.
 
-### Define attributes
-
-After the resource is fabricated, we would like to access the attributes on
-the resource. We define the attributes with `attribute` method. Suppose
-we want to access the name on the resource, we could change `attr_accessor`
-to `attribute`:
-
-```ruby
-module QA
-  module Factory
-    module Resource
-      class Shirt < Factory::Base
-        attribute :name
-
-        # ... same as before
-      end
-    end
-  end
-end
-```
-
-The difference between `attr_accessor` and `attribute` is that by using
-`attribute` it can also be accessed from the product:
-
-```ruby
-shirt =
-  QA::Factory::Resource::Shirt.fabricate! do |resource|
-    resource.name = "GitLab QA"
-  end
-
-shirt.name # => "GitLab QA"
-```
-
-In the above example, if we use `attr_accessor :name` then `shirt.name` won't
-be available. On the other hand, using `attribute :name` will allow you to use
-`shirt.name`, so most of the time you'll want to use `attribute` instead of
-`attr_accessor` unless we clearly don't need it for the product.
-
 #### Resource attributes
 
 A resource may need another resource to exist first. For instance, a project
@@ -145,7 +107,7 @@ module QA
   module Factory
     module Resource
       class Shirt < Factory::Base
-        attribute :name
+        attr_accessor :name
 
         attribute :project do
           Factory::Resource::Project.fabricate! do |resource|
@@ -206,7 +168,7 @@ module QA
   module Factory
     module Resource
       class Shirt < Factory::Base
-        attribute :name
+        attr_accessor :name
 
         attribute :project do
           Factory::Resource::Project.fabricate! do |resource|
@@ -287,7 +249,7 @@ module QA
             shirt_new.create_shirt!
           end
 
-          brand # Eagerly construct the data
+          populate(:brand) # Eagerly construct the data
         end
       end
     end
@@ -295,9 +257,12 @@ module QA
 end
 ```
 
-This will make sure we construct the data right after we created the shirt.
-The drawback for this will become we're forced to construct the data even
-if we don't really need to use it.
+The `populate` method will iterate through its arguments and call each
+attribute respectively. Here `populate(:brand)` has the same effect as
+just `brand`. Using the populate method makes the intention clearer.
+
+With this, it will make sure we construct the data right after we create the
+shirt. The drawback is that this will always construct the data when the resource is fabricated even if we don't need to use the data.
 
 Alternatively, we could just make sure we're on the right page before
 constructing the brand data:
@@ -307,7 +272,7 @@ module QA
   module Factory
     module Resource
       class Shirt < Factory::Base
-        attribute :name
+        attr_accessor :name
 
         attribute :project do
           Factory::Resource::Project.fabricate! do |resource|
@@ -385,7 +350,7 @@ end
 
 **Notes on attributes precedence:**
 
-- attributes from the factory have the highest precedence
+- factory instance variables have the highest precedence
 - attributes from the API response take precedence over attributes from the
   block (usually from Browser UI)
 - attributes without a value will raise a `QA::Factory::Base::NoValueError` error
diff --git a/qa/qa/factory/base.rb b/qa/qa/factory/base.rb
index e82e16f9415..e28a00c545b 100644
--- a/qa/qa/factory/base.rb
+++ b/qa/qa/factory/base.rb
@@ -22,12 +22,16 @@ module QA
         visit(web_url)
       end
 
+      def populate(*attributes)
+        attributes.each(&method(:public_send))
+      end
+
       private
 
       def populate_attribute(name, block)
         value = attribute_value(name, block)
 
-        raise NoValueError, "No value was computed for product #{name} of factory #{self.class.name}." unless value
+        raise NoValueError, "No value was computed for #{name} of #{self.class.name}." unless value
 
         value
       end
@@ -84,7 +88,7 @@ module QA
         resource_web_url = yield
         factory.web_url = resource_web_url
 
-        Factory::Product.new(factory)
+        factory
       end
       private_class_method :do_fabricate!
 
diff --git a/qa/qa/factory/product.rb b/qa/qa/factory/product.rb
deleted file mode 100644
index 34df0bda8e5..00000000000
--- a/qa/qa/factory/product.rb
+++ /dev/null
@@ -1,35 +0,0 @@
-require 'capybara/dsl'
-
-module QA
-  module Factory
-    class Product
-      include Capybara::DSL
-
-      attr_reader :factory
-
-      def initialize(factory)
-        @factory = factory
-
-        define_attributes
-      end
-
-      def visit!
-        visit(web_url)
-      end
-
-      def populate(*attributes)
-        attributes.each(&method(:public_send))
-      end
-
-      private
-
-      def define_attributes
-        factory.class.attributes_names.each do |name|
-          define_singleton_method(name) do
-            factory.public_send(name)
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/qa/qa/factory/repository/project_push.rb b/qa/qa/factory/repository/project_push.rb
index a9dfbc0a783..272b7fc5818 100644
--- a/qa/qa/factory/repository/project_push.rb
+++ b/qa/qa/factory/repository/project_push.rb
@@ -9,8 +9,6 @@ module QA
           end
         end
 
-        attribute :output
-
         def initialize
           @file_name = 'file.txt'
           @file_content = '# This is test project'
diff --git a/qa/qa/factory/resource/fork.rb b/qa/qa/factory/resource/fork.rb
index 0fac4377040..b1e874af893 100644
--- a/qa/qa/factory/resource/fork.rb
+++ b/qa/qa/factory/resource/fork.rb
@@ -50,8 +50,7 @@ module QA
         end
 
         def fabricate!
-          push
-          user
+          populate(:push, :user)
 
           visit_project_with_retry
 
diff --git a/qa/qa/factory/resource/merge_request.rb b/qa/qa/factory/resource/merge_request.rb
index 92b8bdf4a21..4b7d2287f98 100644
--- a/qa/qa/factory/resource/merge_request.rb
+++ b/qa/qa/factory/resource/merge_request.rb
@@ -12,8 +12,6 @@ module QA
                       :milestone,
                       :labels
 
-        attribute :source_branch
-
         attribute :project do
           Factory::Resource::Project.fabricate! do |resource|
             resource.name = 'project-with-merge-request'
@@ -52,8 +50,8 @@ module QA
         end
 
         def fabricate!
-          target
-          source
+          populate(:target, :source)
+
           project.visit!
           Page::Project::Show.perform(&:new_merge_request)
           Page::MergeRequest::New.perform do |page|
diff --git a/qa/qa/factory/resource/merge_request_from_fork.rb b/qa/qa/factory/resource/merge_request_from_fork.rb
index fbe062539b9..1311bf625a6 100644
--- a/qa/qa/factory/resource/merge_request_from_fork.rb
+++ b/qa/qa/factory/resource/merge_request_from_fork.rb
@@ -18,8 +18,10 @@ module QA
         end
 
         def fabricate!
-          push
+          populate(:push)
+
           fork.visit!
+
           Page::Project::Show.perform(&:new_merge_request)
           Page::MergeRequest::New.perform(&:create_merge_request)
         end
diff --git a/qa/qa/factory/resource/project_imported_from_github.rb b/qa/qa/factory/resource/project_imported_from_github.rb
index f62092ae122..ce20641e6cc 100644
--- a/qa/qa/factory/resource/project_imported_from_github.rb
+++ b/qa/qa/factory/resource/project_imported_from_github.rb
@@ -4,14 +4,13 @@ module QA
   module Factory
     module Resource
       class ProjectImportedFromGithub < Resource::Project
+        attr_accessor :name
         attr_writer :personal_access_token, :github_repository_path
 
         attribute :group do
           Factory::Resource::Group.fabricate!
         end
 
-        attribute :name
-
         def fabricate!
           group.visit!
 
diff --git a/qa/qa/factory/resource/project_milestone.rb b/qa/qa/factory/resource/project_milestone.rb
index cfda58dc103..383f534c12c 100644
--- a/qa/qa/factory/resource/project_milestone.rb
+++ b/qa/qa/factory/resource/project_milestone.rb
@@ -2,14 +2,13 @@ module QA
   module Factory
     module Resource
       class ProjectMilestone < Factory::Base
+        attr_reader :title
         attr_accessor :description
 
         attribute :project do
           Factory::Resource::Project.fabricate!
         end
 
-        attribute :title
-
         def title=(title)
           @title = "#{title}-#{SecureRandom.hex(4)}"
           @description = 'A milestone'
diff --git a/qa/qa/factory/resource/sandbox.rb b/qa/qa/factory/resource/sandbox.rb
index 56bcda9e2f3..a125bac65dd 100644
--- a/qa/qa/factory/resource/sandbox.rb
+++ b/qa/qa/factory/resource/sandbox.rb
@@ -9,7 +9,6 @@ module QA
         attr_reader :path
 
         attribute :id
-        attribute :path
 
         def initialize
           @path = Runtime::Namespace.sandbox_name
diff --git a/qa/qa/factory/resource/ssh_key.rb b/qa/qa/factory/resource/ssh_key.rb
index a48a93fbe65..6f952eda36f 100644
--- a/qa/qa/factory/resource/ssh_key.rb
+++ b/qa/qa/factory/resource/ssh_key.rb
@@ -6,11 +6,9 @@ module QA
       class SSHKey < Factory::Base
         extend Forwardable
 
-        def_delegators :key, :private_key, :public_key, :fingerprint
+        attr_accessor :title
 
-        attribute :private_key
-        attribute :title
-        attribute :fingerprint
+        def_delegators :key, :private_key, :public_key, :fingerprint
 
         def key
           @key ||= Runtime::Key::RSA.new
diff --git a/qa/qa/factory/resource/user.rb b/qa/qa/factory/resource/user.rb
index 6e6f46f7a95..e361face1f0 100644
--- a/qa/qa/factory/resource/user.rb
+++ b/qa/qa/factory/resource/user.rb
@@ -5,6 +5,7 @@ module QA
     module Resource
       class User < Factory::Base
         attr_reader :unique_id
+        attr_writer :username, :password
 
         def initialize
           @unique_id = SecureRandom.hex(8)
@@ -30,11 +31,6 @@ module QA
           defined?(@username) && defined?(@password)
         end
 
-        attribute :name
-        attribute :username
-        attribute :email
-        attribute :password
-
         def fabricate!
           # Don't try to log-out if we're not logged-in
           if Page::Main::Menu.perform { |p| p.has_personal_area?(wait: 0) }
diff --git a/qa/spec/factory/base_spec.rb b/qa/spec/factory/base_spec.rb
index d7b92052894..e9584a27d63 100644
--- a/qa/spec/factory/base_spec.rb
+++ b/qa/spec/factory/base_spec.rb
@@ -4,8 +4,7 @@ describe QA::Factory::Base do
   include Support::StubENV
 
   let(:factory) { spy('factory') }
-  let(:product) { spy('product') }
-  let(:product_location) { 'http://product_location' }
+  let(:location) { 'http://location' }
 
   shared_context 'fabrication context' do
     subject do
@@ -17,9 +16,8 @@ describe QA::Factory::Base do
     end
 
     before do
-      allow(subject).to receive(:current_url).and_return(product_location)
+      allow(subject).to receive(:current_url).and_return(location)
       allow(subject).to receive(:new).and_return(factory)
-      allow(QA::Factory::Product).to receive(:new).with(factory).and_return(product)
     end
   end
 
@@ -28,7 +26,7 @@ describe QA::Factory::Base do
 
     it 'yields factory before calling factory method' do
       expect(factory).to receive(:something!).ordered
-      expect(factory).to receive(fabrication_method_used).ordered.and_return(product_location)
+      expect(factory).to receive(fabrication_method_used).ordered.and_return(location)
 
       subject.public_send(fabrication_method_called, factory: factory) do |factory|
         factory.something!
@@ -37,7 +35,7 @@ describe QA::Factory::Base do
 
     it 'does not log the factory and build method when QA_DEBUG=false' do
       stub_env('QA_DEBUG', 'false')
-      expect(factory).to receive(fabrication_method_used).and_return(product_location)
+      expect(factory).to receive(fabrication_method_used).and_return(location)
 
       expect { subject.public_send(fabrication_method_called, 'something', factory: factory) }
         .not_to output.to_stdout
@@ -71,17 +69,17 @@ describe QA::Factory::Base do
 
     it_behaves_like 'fabrication method', :fabricate_via_api!
 
-    it 'instantiates the factory, calls factory method returns fabrication product' do
-      expect(factory).to receive(:fabricate_via_api!).and_return(product_location)
+    it 'instantiates the factory, calls factory method returns the resource' do
+      expect(factory).to receive(:fabricate_via_api!).and_return(location)
 
       result = subject.fabricate_via_api!(factory: factory, parents: [])
 
-      expect(result).to eq(product)
+      expect(result).to eq(factory)
     end
 
     it 'logs the factory and build method when QA_DEBUG=true' do
       stub_env('QA_DEBUG', 'true')
-      expect(factory).to receive(:fabricate_via_api!).and_return(product_location)
+      expect(factory).to receive(:fabricate_via_api!).and_return(location)
 
       expect { subject.fabricate_via_api!(factory: factory, parents: []) }
         .to output(/==> Built a MyFactory via api with args \[\] in [\d\w\.\-]+/)
@@ -100,10 +98,10 @@ describe QA::Factory::Base do
       expect(factory).to have_received(:fabricate!).with('something')
     end
 
-    it 'returns fabrication product' do
+    it 'returns fabrication resource' do
       result = subject.fabricate_via_browser_ui!('something', factory: factory, parents: [])
 
-      expect(result).to eq(product)
+      expect(result).to eq(factory)
     end
 
     it 'logs the factory and build method when QA_DEBUG=true' do
@@ -140,44 +138,44 @@ describe QA::Factory::Base do
   describe '.attribute' do
     include_context 'simple factory'
 
-    it 'appends new product attribute' do
+    it 'appends new attribute' do
       expect(subject.attributes_names).to eq([:no_block, :test, :web_url])
     end
 
-    context 'when the product attribute is populated via a block' do
-      it 'returns a fabrication product and defines factory attributes as its methods' do
+    context 'when the attribute is populated via a block' do
+      it 'returns value from the block' do
         result = subject.fabricate!(factory: factory)
 
-        expect(result).to be_a(QA::Factory::Product)
+        expect(result).to be_a(described_class)
         expect(result.test).to eq('block')
       end
     end
 
-    context 'when the product attribute is populated via the api' do
+    context 'when the attribute is populated via the api' do
       let(:api_resource) { { no_block: 'api' } }
 
       before do
         expect(factory).to receive(:api_resource).and_return(api_resource)
       end
 
-      it 'returns a fabrication product and defines factory attributes as its methods' do
+      it 'returns value from api' do
         result = subject.fabricate!(factory: factory)
 
-        expect(result).to be_a(QA::Factory::Product)
+        expect(result).to be_a(described_class)
         expect(result.no_block).to eq('api')
       end
 
-      context 'when the attribute also has a block in the factory' do
+      context 'when the attribute also has a block' do
         let(:api_resource) { { test: 'api_with_block' } }
 
         before do
           allow(QA::Runtime::Logger).to receive(:info)
         end
 
-        it 'returns the api value and emits an INFO log entry' do
+        it 'returns value from api and emits an INFO log entry' do
           result = subject.fabricate!(factory: factory)
 
-          expect(result).to be_a(QA::Factory::Product)
+          expect(result).to be_a(described_class)
           expect(result.test).to eq('api_with_block')
           expect(QA::Runtime::Logger)
             .to have_received(:info).with(/api_with_block/)
@@ -185,15 +183,15 @@ describe QA::Factory::Base do
       end
     end
 
-    context 'when the product attribute is populated via a factory attribute' do
+    context 'when the attribute is populated via direct assignment' do
       before do
         factory.test = 'value'
       end
 
-      it 'returns a fabrication product and defines factory attributes as its methods' do
+      it 'returns value from the assignment' do
         result = subject.fabricate!(factory: factory)
 
-        expect(result).to be_a(QA::Factory::Product)
+        expect(result).to be_a(described_class)
         expect(result.test).to eq('value')
       end
 
@@ -202,21 +200,21 @@ describe QA::Factory::Base do
           allow(factory).to receive(:api_resource).and_return({ test: 'api' })
         end
 
-        it 'returns the factory attribute for the product' do
+        it 'returns value from the assignment' do
           result = subject.fabricate!(factory: factory)
 
-          expect(result).to be_a(QA::Factory::Product)
+          expect(result).to be_a(described_class)
           expect(result.test).to eq('value')
         end
       end
     end
 
-    context 'when the product attribute has no value' do
+    context 'when the attribute has no value' do
       it 'raises an error because no values could be found' do
         result = subject.fabricate!(factory: factory)
 
         expect { result.no_block }
-          .to raise_error(described_class::NoValueError, "No value was computed for product no_block of factory #{factory.class.name}.")
+          .to raise_error(described_class::NoValueError, "No value was computed for no_block of #{factory.class.name}.")
       end
     end
   end
diff --git a/qa/spec/factory/product_spec.rb b/qa/spec/factory/product_spec.rb
deleted file mode 100644
index 5b6eaa13e9c..00000000000
--- a/qa/spec/factory/product_spec.rb
+++ /dev/null
@@ -1,29 +0,0 @@
-describe QA::Factory::Product do
-  let(:factory) do
-    Class.new(QA::Factory::Base) do
-      attribute :test do
-        'block'
-      end
-
-      attribute :no_block
-    end.new
-  end
-
-  let(:product) { spy('product') }
-  let(:product_location) { 'http://product_location' }
-
-  subject { described_class.new(factory) }
-
-  before do
-    factory.web_url = product_location
-  end
-
-  describe '.visit!' do
-    it 'makes it possible to visit fabrication product' do
-      allow_any_instance_of(described_class)
-        .to receive(:visit).and_return('visited some url')
-
-      expect(subject.visit!).to eq 'visited some url'
-    end
-  end
-end