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

Absorb product into factory

Closes gitlab-ce#52992

See merge request gitlab-org/gitlab-ee!8100

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'

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

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!
 

qa/qa/factory/product.rb

-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

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'

qa/qa/factory/resource/fork.rb

@@ -50,8 +50,7 @@ module QA
         end
 
         def fabricate!
-          push
-          user
+          populate(:push, :user)
 
           visit_project_with_retry
 

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|

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

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!
 

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'

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

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

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) }

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

qa/spec/factory/product_spec.rb

-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