Merge branch 'acunskis-resource-objects-refactor' into 'master'

E2E: Update all resource attributes when calling reload!

See merge request gitlab-org/gitlab!64089

qa/qa/page/main/login.rb

@@ -67,7 +67,7 @@ module QA
         end
 
         def sign_in_using_admin_credentials
-          admin = QA::Resource::User.new.tap do |user|
+          admin = QA::Resource::User.init do |user|
             user.username = QA::Runtime::User.admin_username
             user.password = QA::Runtime::User.admin_password
           end

qa/qa/resource/base.rb

 # frozen_string_literal: true
 
-require 'forwardable'
 require 'capybara/dsl'
 require 'active_support/core_ext/array/extract_options'
 
 module QA
   module Resource
     class Base
-      extend SingleForwardable
       include ApiFabricator
       extend Capybara::DSL
 
       NoValueError = Class.new(RuntimeError)
 
-      def_delegators :evaluator, :attribute
+      class << self
+        # Initialize new instance of class without fabrication
+        #
+        # @param [Proc] prepare_block
+        def init(&prepare_block)
+          new.tap(&prepare_block)
+        end
 
-      def self.fabricate!(*args, &prepare_block)
+        def fabricate!(*args, &prepare_block)
           fabricate_via_api!(*args, &prepare_block)
         rescue NotImplementedError
           fabricate_via_browser_ui!(*args, &prepare_block)
         end
 
-      def self.fabricate_via_browser_ui!(*args, &prepare_block)
+        def fabricate_via_browser_ui!(*args, &prepare_block)
           options = args.extract_options!
           resource = options.fetch(:resource) { new }
           parents = options.fetch(:parents) { [] }
@@ -33,7 +37,7 @@ module QA
           end
         end
 
-      def self.fabricate_via_api!(*args, &prepare_block)
+        def fabricate_via_api!(*args, &prepare_block)
           options = args.extract_options!
           resource = options.fetch(:resource) { new }
           parents = options.fetch(:parents) { [] }
@@ -47,7 +51,7 @@ module QA
           end
         end
 
-      def self.remove_via_api!(*args, &prepare_block)
+        def remove_via_api!(*args, &prepare_block)
           options = args.extract_options!
           resource = options.fetch(:resource) { new }
           parents = options.fetch(:parents) { [] }
@@ -59,12 +63,81 @@ module QA
           end
         end
 
+        private
+
+        def do_fabricate!(resource:, prepare_block:, parents: [])
+          prepare_block.call(resource) if prepare_block
+
+          resource_web_url = yield
+          resource.web_url = resource_web_url
+
+          resource
+        end
+
+        def log_fabrication(method, resource, parents, args)
+          return yield unless Runtime::Env.debug?
+
+          start = Time.now
+          prefix = "==#{'=' * parents.size}>"
+          msg = [prefix]
+          msg << "Built a #{name}"
+          msg << "as a dependency of #{parents.last}" if parents.any?
+          msg << "via #{method}"
+
+          yield.tap do
+            msg << "in #{Time.now - start} seconds"
+            puts msg.join(' ')
+            puts if parents.empty?
+          end
+        end
+
+        # Define custom attribute
+        #
+        # @param [Symbol] name
+        # @return [void]
+        def attribute(name, &block)
+          (@attribute_names ||= []).push(name) # save added attributes
+
+          attr_writer(name)
+
+          define_method(name) do
+            instance_variable_get("@#{name}") || instance_variable_set("@#{name}", populate_attribute(name, block))
+          end
+        end
+
+        # Define multiple custom attributes
+        #
+        # @param [Array] names
+        # @return [void]
+        def attributes(*names)
+          names.each { |name| attribute(name) }
+        end
+      end
+
+      # Override api reload! and update custom attributes from api_resource
+      #
+      api_reload = instance_method(:reload!)
+      define_method(:reload!) do
+        api_reload.bind_call(self)
+        return self unless api_resource
+
+        all_attributes.each do |attribute_name|
+          api_value = api_resource[attribute_name]
+
+          instance_variable_set("@#{attribute_name}", api_value) if api_value
+        end
+
+        self
+      end
+
+      attribute :web_url
+
       def fabricate!(*_args)
         raise NotImplementedError
       end
 
       def visit!
-        Runtime::Logger.debug(%Q[Visiting #{self.class.name} at "#{web_url}"])
+        Runtime::Logger.debug(%(Visiting #{self.class.name} at "#{web_url}"))
 
         # Just in case an async action is not yet complete
         Support::WaitForRequests.wait_for_requests
@@ -78,14 +151,12 @@ module QA
         Support::WaitForRequests.wait_for_requests
       end
 
-      def populate(*attributes)
-        attributes.each(&method(:public_send))
+      def populate(*attribute_names)
+        attribute_names.each { |attribute_name| public_send(attribute_name) }
       end
 
-      def wait_until(max_duration: 60, sleep_interval: 0.1)
-        QA::Support::Waiter.wait_until(max_duration: max_duration, sleep_interval: sleep_interval) do
-          yield
-        end
+      def wait_until(max_duration: 60, sleep_interval: 0.1, &block)
+        QA::Support::Waiter.wait_until(max_duration: max_duration, sleep_interval: sleep_interval, &block)
       end
 
       private
@@ -101,70 +172,27 @@ module QA
       def attribute_value(name, block)
         api_value = api_resource&.dig(name)
 
-        if api_value && block
-          log_having_both_api_result_and_block(name, api_value)
-        end
+        log_having_both_api_result_and_block(name, api_value) if api_value && block
 
         api_value || (block && instance_exec(&block))
       end
 
-      def log_having_both_api_result_and_block(name, api_value)
-        QA::Runtime::Logger.info "<#{self.class}> Attribute #{name.inspect} has both API response `#{api_value}` and a block. API response will be picked. Block will be ignored."
-      end
-
-      def self.do_fabricate!(resource:, prepare_block:, parents: [])
-        prepare_block.call(resource) if prepare_block
-
-        resource_web_url = yield
-        resource.web_url = resource_web_url
-
-        resource
+      # Get all defined attributes across all parents
+      #
+      # @return [Array<Symbol>]
+      def all_attributes
+        @all_attributes ||= self.class.ancestors
+                                .select { |clazz| clazz <= QA::Resource::Base }
+                                .map { |clazz| clazz.instance_variable_get(:@attribute_names) }
+                                .flatten
+                                .compact
       end
-      private_class_method :do_fabricate!
-
-      def self.log_fabrication(method, resource, parents, args)
-        return yield unless Runtime::Env.debug?
-
-        start = Time.now
-        prefix = "==#{'=' * parents.size}>"
-        msg = [prefix]
-        msg << "Built a #{name}"
-        msg << "as a dependency of #{parents.last}" if parents.any?
-        msg << "via #{method}"
 
-        yield.tap do
-          msg << "in #{Time.now - start} seconds"
-          puts msg.join(' ')
-          puts if parents.empty?
-        end
-      end
-      private_class_method :log_fabrication
-
-      def self.evaluator
-        @evaluator ||= Base::DSL.new(self)
-      end
-      private_class_method :evaluator
-
-      class DSL
-        def initialize(base)
-          @base = base
-        end
-
-        def attribute(name, &block)
-          @base.module_eval do
-            attr_writer(name)
-
-            define_method(name) do
-              instance_variable_get("@#{name}") ||
-                instance_variable_set(
-                  "@#{name}",
-                  populate_attribute(name, block))
-            end
-          end
-        end
+      def log_having_both_api_result_and_block(name, api_value)
+        QA::Runtime::Logger.info(<<~MSG.strip)
+          <#{self.class}> Attribute #{name.inspect} has both API response `#{api_value}` and a block. API response will be picked. Block will be ignored.
+        MSG
       end
-
-      attribute :web_url
     end
   end
 end

qa/qa/resource/group.rb

@@ -3,7 +3,7 @@
 module QA
   module Resource
     class Group < GroupBase
-      attr_accessor :description
+      attributes :require_two_factor_authentication, :description
 
       attribute :full_path do
         determine_full_path
@@ -15,8 +15,6 @@ module QA
         end
       end
 
-      attribute :require_two_factor_authentication
-
       def initialize
         @path = Runtime::Namespace.name
         @description = "QA test run at #{Runtime::Namespace.time}"

qa/qa/resource/group_base.rb

@@ -9,17 +9,17 @@ module QA
 
       attr_accessor :path
 
-      attribute :id
-      attribute :runners_token
-      attribute :name
-      attribute :full_path
+      attributes :id,
+                 :runners_token,
+                 :name,
+                 :full_path
 
       # Get group labels
       #
       # @return [Array<QA::Resource::GroupLabel>]
       def labels
         parse_body(api_get_from("#{api_get_path}/labels")).map do |label|
-          GroupLabel.new.tap do |resource|
+          GroupLabel.init do |resource|
             resource.api_client = api_client
             resource.group = self
             resource.id = label[:id]

qa/qa/resource/merge_request.rb

 # frozen_string_literal: true
 
 require 'securerandom'
-require 'active_support/core_ext/object/blank'
 
 module QA
   module Resource
     class MergeRequest < Base
       attr_accessor :approval_rules,
-                    :id,
-                    :title,
-                    :description,
                     :source_branch,
-                    :target_branch,
                     :target_new_branch,
                     :assignee,
                     :milestone,
@@ -22,9 +17,12 @@ module QA
                   :wait_for_merge,
                   :template
 
-      attribute :merge_when_pipeline_succeeds
-      attribute :merge_status
-      attribute :state
+      attributes :iid,
+                 :title,
+                 :description,
+                 :merge_when_pipeline_succeeds,
+                 :merge_status,
+                 :state
 
       attribute :project do
         Project.fabricate! do |resource|
@@ -32,11 +30,15 @@ module QA
         end
       end
 
+      attribute :target_branch do
+        project.default_branch
+      end
+
       attribute :target do
         Repository::ProjectPush.fabricate! do |resource|
           resource.project = project
           resource.branch_name = target_branch
-          resource.new_branch = @target_new_branch
+          resource.new_branch = target_new_branch
           resource.remote_branch = target_branch
         end
       end
@@ -62,7 +64,6 @@ module QA
         @labels = []
         @file_name = "added_file-#{SecureRandom.hex(8)}.txt"
         @file_content = "File Added"
-        @target_branch = project.default_branch
         @target_new_branch = true
         @no_preparation = false
         @wait_for_merge = true
@@ -89,21 +90,19 @@ module QA
       end
 
       def fabricate_via_api!
-        raise ResourceNotFoundError unless id
-
         resource_web_url(api_get)
-      rescue ResourceNotFoundError
+      rescue ResourceNotFoundError, NoValueError # rescue if iid not populated
         populate_target_and_source_if_required
 
         super
       end
 
       def api_merge_path
-        "/projects/#{project.id}/merge_requests/#{id}/merge"
+        "/projects/#{project.id}/merge_requests/#{iid}/merge"
       end
 
       def api_get_path
-        "/projects/#{project.id}/merge_requests/#{id}"
+        "/projects/#{project.id}/merge_requests/#{iid}"
       end
 
       def api_post_path
@@ -112,18 +111,22 @@ module QA
 
       def api_post_body
         {
-          description: @description,
-          source_branch: @source_branch,
-          target_branch: @target_branch,
-          title: @title
+          description: description,
+          source_branch: source_branch,
+          target_branch: target_branch,
+          title: title
         }
       end
 
+      def api_comments_path
+        "#{api_get_path}/notes"
+      end
+
       def merge_via_api!
         Support::Waiter.wait_until(sleep_interval: 1) do
-          QA::Runtime::Logger.debug("Waiting until merge request with id '#{id}' can be merged")
+          QA::Runtime::Logger.debug("Waiting until merge request with id '#{iid}' can be merged")
 
-          reload!.api_resource[:merge_status] == 'can_be_merged'
+          reload!.merge_status == 'can_be_merged'
         end
 
         Support::Retrier.retry_on_exception do
@@ -141,12 +144,12 @@ module QA
         end
       end
 
-      def reload!
-        # Refabricate so that we can return a new object with updated attributes
-        self.class.fabricate_via_api! do |resource|
-          resource.project = project
-          resource.id = api_resource[:iid]
-        end
+      # Get MR comments
+      #
+      # @return [Array]
+      def comments
+        response = get(Runtime::API::Request.new(api_client, api_comments_path).url)
+        parse_body(response)
       end
 
       private
@@ -158,8 +161,6 @@ module QA
       end
 
       def populate_target_and_source_if_required
-        @target_branch ||= project.default_branch
-
         populate(:target, :source) unless @no_preparation
       end
     end

qa/qa/resource/user.rb

@@ -25,7 +25,7 @@ module QA
       end
 
       def self.default
-        Resource::User.new.tap do |user|
+        Resource::User.init do |user|
           user.username = Runtime::User.ldap_user? ? Runtime::User.ldap_username : Runtime::User.username
           user.password = Runtime::User.ldap_user? ? Runtime::User.ldap_password : Runtime::User.password
         end

qa/qa/service/cluster_provider/k3s.rb

@@ -9,7 +9,7 @@ module QA
         end
 
         def setup
-          @k3s = Service::DockerRun::K3s.new.tap do |k3s|
+          @k3s = Service::DockerRun::K3s.init do |k3s|
             k3s.remove!
             k3s.register!
 

qa/qa/specs/features/api/3_create/merge_request/push_options_mwps_spec.rb

@@ -64,7 +64,7 @@ module QA
 
         merge_request = Resource::MergeRequest.fabricate_via_api! do |mr|
           mr.project = project
-          mr.id = merge_request[:iid]
+          mr.iid = merge_request[:iid]
         end
 
         expect(merge_request.state).to eq('opened')
@@ -109,7 +109,7 @@ module QA
         merge_request = Support::Waiter.wait_until(sleep_interval: 5) do
           mr = Resource::MergeRequest.fabricate_via_api! do |mr|
             mr.project = project
-            mr.id = merge_request[:iid]
+            mr.iid = merge_request[:iid]
           end
 
           next unless mr.state == 'merged'

qa/qa/specs/features/api/3_create/merge_request/push_options_remove_source_branch_spec.rb

@@ -34,7 +34,7 @@ module QA
 
         merge_request = Resource::MergeRequest.fabricate_via_api! do |mr|
           mr.project = project
-          mr.id = merge_request[:iid]
+          mr.iid = merge_request[:iid]
         end.merge_via_api!
 
         expect(merge_request[:state]).to eq('merged')

qa/qa/specs/features/api/3_create/merge_request/push_options_target_branch_spec.rb

@@ -55,7 +55,7 @@ module QA
 
         merge_request = Resource::MergeRequest.fabricate_via_api! do |mr|
           mr.project = project
-          mr.id = merge_request[:iid]
+          mr.iid = merge_request[:iid]
         end.merge_via_api!
 
         expect(merge_request[:state]).to eq('merged')

qa/qa/specs/features/api/3_create/merge_request/push_options_title_description_spec.rb

@@ -39,7 +39,7 @@ module QA
 
         merge_request = Resource::MergeRequest.fabricate_via_api! do |mr|
           mr.project = project
-          mr.id = merge_request[:iid]
+          mr.iid = merge_request[:iid]
         end.merge_via_api!
 
         expect(merge_request[:state]).to eq('merged')

qa/qa/specs/features/api/5_package/container_registry_spec.rb

@@ -17,8 +17,8 @@ module QA
       end
 
       let(:registry) do
-        Resource::RegistryRepository.new.tap do |repository|
-          repository.name = "#{project.path_with_namespace}"
+        Resource::RegistryRepository.init do |repository|
+          repository.name = project.path_with_namespace
           repository.project = project
           repository.tag_name = 'master'
         end

qa/qa/specs/features/browser_ui/1_manage/group/bulk_import_group_spec.rb

@@ -36,7 +36,7 @@ module QA
       end
 
       let(:imported_group) do
-        Resource::Group.new.tap do |group|
+        Resource::Group.init do |group|
           group.api_client = api_client
           group.sandbox = sandbox
           group.path = source_group.path
@@ -44,7 +44,7 @@ module QA
       end
 
       let(:imported_subgroup) do
-        Resource::Group.new.tap do |group|
+        Resource::Group.init do |group|
           group.api_client = api_client
           group.sandbox = imported_group
           group.path = subgroup.path

qa/qa/specs/features/browser_ui/1_manage/project/dashboard_images_spec.rb

@@ -6,7 +6,7 @@ module QA
   RSpec.describe 'Manage', quarantine: { issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/212145', type: :stale } do
     describe 'Check for broken images', :requires_admin do
       before(:context) do
-        admin = QA::Resource::User.new.tap do |user|
+        admin = QA::Resource::User.init do |user|
           user.username = QA::Runtime::User.admin_username
           user.password = QA::Runtime::User.admin_password
         end

qa/qa/specs/features/browser_ui/1_manage/project/import_github_repo_spec.rb

@@ -105,9 +105,15 @@ module QA
 
       def verify_merge_requests_import
         merge_requests = imported_project.merge_requests
+        merge_request = Resource::MergeRequest.init do |mr|
+          mr.project = imported_project
+          mr.iid = merge_requests.first[:iid]
+          mr.api_client = api_client
+        end.reload!
+        mr_comments = merge_request.comments.map { |comment| comment[:body] } # rubocop:disable Rails/Pluck
 
         expect(merge_requests.length).to eq(1)
-        expect(merge_requests.first).to include(
+        expect(merge_request.api_resource).to include(
           title: 'Improve readme',
           state: 'opened',
           target_branch: 'main',
@@ -117,6 +123,13 @@ module QA
             *Created by: gitlab-qa-github*\n\nThis improves the README file a bit.\r\n\r\nTODO:\r\n\r\n \r\n\r\n- [ ] Do foo\r\n- [ ]  Make bar\r\n  - [ ]  Think about baz
           DSC
         )
+        expect(mr_comments).to eq(
+          [
+            "*Created by: gitlab-qa-github*\n\n[PR comment by @sliaquat] Nice work! ",
+            "*Created by: gitlab-qa-github*\n\n[Single diff comment] Nice addition",
+            "*Created by: gitlab-qa-github*\n\n[Single diff comment] Good riddance"
+          ]
+        )
       end
     end
   end

qa/qa/specs/features/browser_ui/3_create/jenkins/jenkins_build_status_spec.rb

@@ -94,7 +94,7 @@ module QA
       end
 
       def run_jenkins_server
-        Service::DockerRun::Jenkins.new.tap do |runner|
+        Service::DockerRun::Jenkins.init do |runner|
           runner.pull
           runner.register!
         end

qa/qa/specs/features/browser_ui/3_create/repository/push_http_private_token_spec.rb

@@ -8,7 +8,7 @@ module QA
 
         access_token = Resource::PersonalAccessToken.fabricate!.token
 
-        user = Resource::User.new.tap do |user|
+        user = Resource::User.init do |user|
           user.username = Runtime::User.username
           user.password = access_token
         end

qa/qa/specs/features/browser_ui/5_package/composer_registry_spec.rb

@@ -14,7 +14,7 @@ module QA
       end
 
       let(:package) do
-        Resource::Package.new.tap do |package|
+        Resource::Package.init do |package|
           package.name = "my_package-#{SecureRandom.hex(4)}"
           package.project = project
         end

qa/qa/specs/features/browser_ui/5_package/conan_repository_spec.rb

@@ -12,7 +12,7 @@ module QA
       end
 
       let(:package) do
-        Resource::Package.new.tap do |package|
+        Resource::Package.init do |package|
           package.name = 'conantest'
           package.project = project
         end

qa/qa/specs/features/browser_ui/5_package/generic_repository_spec.rb

@@ -10,7 +10,7 @@ module QA
       end
 
       let(:package) do
-        Resource::Package.new.tap do |package|
+        Resource::Package.init do |package|
           package.name = "my_package"
           package.project = project
         end

qa/qa/specs/features/browser_ui/5_package/maven_gradle_repository_spec.rb

@@ -24,7 +24,7 @@ module QA
       end
 
       let(:package) do
-        Resource::Package.new.tap do |package|
+        Resource::Package.init do |package|
           package.name = package_name
           package.project = project
         end

qa/qa/specs/features/browser_ui/5_package/maven_repository_spec.rb

@@ -33,7 +33,7 @@ module QA
       end
 
       let(:package) do
-        Resource::Package.new.tap do |package|
+        Resource::Package.init do |package|
           package.name = package_name
           package.project = project
         end

qa/qa/specs/features/browser_ui/5_package/npm_registry_spec.rb

@@ -120,7 +120,7 @@ module QA
       end
 
       let(:package) do
-        Resource::Package.new.tap do |package|
+        Resource::Package.init do |package|
           package.name = "@#{registry_scope}/#{project.name}"
           package.project = project
         end

qa/qa/specs/features/browser_ui/5_package/nuget_repository_spec.rb

@@ -14,7 +14,7 @@ module QA
       end
 
       let(:package) do
-        Resource::Package.new.tap do |package|
+        Resource::Package.init do |package|
           package.name = "dotnetcore-#{SecureRandom.hex(8)}"
           package.project = project
         end

qa/qa/specs/features/browser_ui/5_package/pypi_repository_spec.rb

@@ -11,7 +11,7 @@ module QA
       end
 
       let(:package) do
-        Resource::Package.new.tap do |package|
+        Resource::Package.init do |package|
           package.name = 'mypypipackage'
           package.project = project
         end

qa/qa/specs/features/browser_ui/5_package/rubygems_registry_spec.rb

@@ -12,7 +12,7 @@ module QA
       end
 
       let(:package) do
-        Resource::Package.new.tap do |package|
+        Resource::Package.init do |package|
           package.name = 'mygem'
           package.project = project
         end

qa/qa/specs/features/ee/browser_ui/1_manage/group/group_saml_enforced_sso_new_account_spec.rb

@@ -16,7 +16,7 @@ module QA
       # The user that signs in via the IDP with username `user3` and password `user3pass`
       # will have `user_3` as username in GitLab
       let(:user) do
-        QA::Resource::User.new.tap do |user|
+        QA::Resource::User.init do |user|
           user.username = 'user_3'
           user.email = 'user_3@example.com'
           user.name = 'User Three'

qa/qa/specs/features/ee/browser_ui/1_manage/instance/instance_audit_logs_spec.rb

@@ -22,7 +22,7 @@ module QA
       context 'Failed sign in', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/736' do
         before do
           Runtime::Browser.visit(:gitlab, Page::Main::Login)
-          invalid_user = QA::Resource::User.new.tap do |user|
+          invalid_user = Resource::User.init do |user|
             user.username = 'bad_user_name'
             user.password = 'bad_pasword'
           end

qa/qa/specs/features/ee/browser_ui/3_create/repository/push_rules_spec.rb

@@ -114,7 +114,7 @@ module QA
         end
 
         it 'rejects non-member users', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/1778', quarantine: { issue: 'https://gitlab.com/gitlab-org/gitlab/-/issues/224465', type: :investigating } do
-          non_member_user = Resource::User.new.tap do |user|
+          non_member_user = Resource::User.init do |user|
             user.username = ''
             user.password = ''
             user.name = 'non_member_user'
@@ -205,7 +205,7 @@ module QA
           user.password = Runtime::User.password
         end
 
-        @root = Resource::User.new.tap do |user|
+        @root = Resource::User.init do |user|
           user.username = 'root'
           user.name = 'GitLab QA'
           user.email = 'root@gitlab.com'