From e7741b0e5247024998bad7de6ebba33933db266a Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 31 Oct 2018 02:44:47 +0000 Subject: [PATCH] CE: Absorb product into factory --- qa/qa.rb | 1 - qa/qa/factory/README.md | 57 ++++--------------- qa/qa/factory/base.rb | 8 ++- qa/qa/factory/product.rb | 35 ------------ qa/qa/factory/repository/project_push.rb | 2 - qa/qa/factory/resource/fork.rb | 3 +- qa/qa/factory/resource/merge_request.rb | 6 +- .../resource/merge_request_from_fork.rb | 4 +- .../resource/project_imported_from_github.rb | 3 +- qa/qa/factory/resource/project_milestone.rb | 3 +- qa/qa/factory/resource/sandbox.rb | 1 - qa/qa/factory/resource/ssh_key.rb | 6 +- qa/qa/factory/resource/user.rb | 6 +- qa/spec/factory/base_spec.rb | 56 +++++++++--------- qa/spec/factory/product_spec.rb | 29 ---------- 15 files changed, 55 insertions(+), 165 deletions(-) delete mode 100644 qa/qa/factory/product.rb delete mode 100644 qa/spec/factory/product_spec.rb 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 -- GitLab