Automatic merge of gitlab-org/gitlab-ce master

app/assets/javascripts/performance_bar/components/detailed_metric.vue

@@ -38,7 +38,11 @@ export default {
 };
 </script>
 <template>
-  <div v-if="currentRequest.details" :id="`peek-view-${metric}`" class="view">
+  <div
+    v-if="currentRequest.details"
+    :id="`peek-view-${metric}`"
+    class="view qa-performance-bar-detailed-metric"
+  >
     <button
       :data-target="`#modal-peek-${metric}-details`"
       class="btn-blank btn-link bold"

app/assets/javascripts/performance_bar/components/performance_bar_app.vue

@@ -92,7 +92,7 @@ export default {
 </script>
 <template>
   <div id="js-peek" :class="env">
-    <div v-if="currentRequest" class="d-flex container-fluid container-limited">
+    <div v-if="currentRequest" class="d-flex container-fluid container-limited qa-performance-bar">
       <div id="peek-view-host" class="view">
         <span
           v-if="hasHost"

app/assets/javascripts/performance_bar/components/request_selector.vue

@@ -37,7 +37,12 @@ export default {
 <template>
   <div id="peek-request-selector">
     <select v-model="currentRequestId">
-      <option v-for="request in requests" :key="request.id" :value="request.id">
+      <option
+        v-for="request in requests"
+        :key="request.id"
+        :value="request.id"
+        class="qa-performance-bar-request"
+      >
         {{ truncatedUrl(request.url) }}
       </option>
     </select>

app/views/admin/application_settings/_performance_bar.html.haml

@@ -5,10 +5,10 @@
     .form-group
       .form-check
         = f.check_box :performance_bar_enabled, class: 'form-check-input'
-        = f.label :performance_bar_enabled, class: 'form-check-label' do
+        = f.label :performance_bar_enabled, class: 'form-check-label qa-enable-performance-bar-checkbox' do
           Enable the Performance Bar
     .form-group
       = f.label :performance_bar_allowed_group_path, 'Allowed group', class: 'label-bold'
       = f.text_field :performance_bar_allowed_group_path, class: 'form-control', placeholder: 'my-org/my-group', value: @application_setting.performance_bar_allowed_group&.full_path
 
-  = f.submit 'Save changes', class: "btn btn-success"
+  = f.submit 'Save changes', class: 'btn btn-success qa-save-changes-button'

app/views/admin/application_settings/metrics_and_profiling.html.haml

@@ -24,7 +24,7 @@
   .settings-content
     = render 'prometheus'
 
-%section.settings.as-performance-bar.no-animate#js-performance-bar-settings{ class: ('expanded' if expanded_by_default?) }
+%section.settings.qa-performance-bar-settings.as-performance-bar.no-animate#js-performance-bar-settings{ class: ('expanded' if expanded_by_default?) }
   .settings-header
     %h4
       = _('Profiling - Performance bar')

app/views/layouts/nav/sidebar/_admin.html.haml

@@ -259,7 +259,7 @@
                 %span
                   = _('Reporting')
             = nav_link(path: 'application_settings#metrics_and_profiling') do
-              = link_to metrics_and_profiling_admin_application_settings_path, title: _('Metrics and profiling') do
+              = link_to metrics_and_profiling_admin_application_settings_path, title: _('Metrics and profiling'), class: 'qa-admin-settings-metrics-and-profiling-item' do
                 %span
                   = _('Metrics and profiling')
             = nav_link(path: 'application_settings#network') do

doc/development/code_review.md

@@ -132,14 +132,6 @@ If a developer who happens to also be a maintainer was involved in a merge reque
 as a domain expert and/or reviewer, it is recommended that they are not also picked
 as the maintainer to ultimately approve and merge it.
 
-Try to review in a timely manner; doing so allows everyone involved in the merge
-request to iterate faster as the context is fresh in memory. Further, this
-improves contributors' experiences significantly. Reviewers should aim to review
-within two working days from the date they were assigned the merge request. If
-you don't think you'll be able to review a merge request within that time, let
-the author know as soon as possible. When the author of the merge request has not
-heard anything after two days, a new reviewer should be assigned.
-
 Maintainers should check before merging if the merge request is approved by the
 required approvers.
 
@@ -220,6 +212,37 @@ It is responsibility of the author of a merge request that the merge request is
 
 Developers who have capacity can regularly check the list of [merge requests to review](https://gitlab.com/groups/gitlab-org/-/merge_requests?scope=all&utf8=%E2%9C%93&state=opened&label_name%5B%5D=ready%20for%20review) and assign any merge request they want to review.
 
+### Review turnaround time
+
+Since [unblocking others is always a top priority](https://about.gitlab.com/handbook/values/#global-optimization),
+reviewers are expected to review assigned merge requests in a timely manner,
+even when this may negatively impact their other tasks and priorities.
+Doing so allows everyone involved in the merge request to iterate faster as the
+context is fresh in memory, improves contributors' experiences significantly,
+and gives authors more time to address feedback and iterate on their work before
+the [feature freeze](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md#feature-freeze-on-the-7th-for-the-release-on-the-22nd).
+
+A turnaround time of two working days is usually acceptable, since engineers
+will typically have other things to work on while they're waiting for review,
+but don't hesitate to ask the author if it's unclear what time frame would be
+acceptable, how urgent the review is, or how significant the blockage. Authors
+are also encouraged to provide this information up-front to reviewers, but are
+expected to be mindful of the [guidelines on when to ask for review on MRs that
+are intended to go in before the feature freeze](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/PROCESS.md#between-the-1st-and-the-7th),
+and realistic in their expectations if these were not followed.
+
+If you don't think you'll be able to review a merge request within a reasonable
+time frame, let the author know as soon as possible and try to help them find
+another reviewer or maintainer who will be able to, so that they can be unblocked
+and get on with their work quickly. Of course, if you are out of office and have
+[communicated](https://about.gitlab.com/handbook/paid-time-off/#communicating-your-time-off)
+this through your GitLab.com Status, authors are expected to realize this and
+find a different reviewer themselves.
+
+When a merge request author feels like they have been blocked for longer than
+is reasonable, they are free to remind the reviewer through Slack or assign
+another reviewer.
+
 ### Reviewing code
 
 Understand why the change is necessary (fixes a bug, improves the user

qa/qa.rb

@@ -275,6 +275,7 @@ module QA
 
     module Layout
       autoload :Banner, 'qa/page/layout/banner'
+      autoload :PerformanceBar, 'qa/page/layout/performance_bar'
     end
 
     module Label
@@ -293,10 +294,12 @@ module QA
       module Settings
         autoload :Repository, 'qa/page/admin/settings/repository'
         autoload :General, 'qa/page/admin/settings/general'
+        autoload :MetricsAndProfiling, 'qa/page/admin/settings/metrics_and_profiling'
 
         module Component
           autoload :RepositoryStorage, 'qa/page/admin/settings/component/repository_storage'
           autoload :AccountAndLimit, 'qa/page/admin/settings/component/account_and_limit'
+          autoload :PerformanceBar, 'qa/page/admin/settings/component/performance_bar'
         end
       end
     end

qa/qa/page/admin/menu.rb

@@ -12,6 +12,7 @@ module QA
           element :admin_settings_item
           element :admin_settings_repository_item
           element :admin_settings_general_item
+          element :admin_settings_metrics_and_profiling_item
         end
 
         def go_to_repository_settings
@@ -30,6 +31,14 @@ module QA
           end
         end
 
+        def go_to_metrics_and_profiling_settings
+          hover_settings do
+            within_submenu do
+              click_element :admin_settings_metrics_and_profiling_item
+            end
+          end
+        end
+
         private
 
         def hover_settings

qa/qa/page/admin/settings/component/performance_bar.rb

+# frozen_string_literal: true
+
+module QA
+  module Page
+    module Admin
+      module Settings
+        module Component
+          class PerformanceBar < Page::Base
+            view 'app/views/admin/application_settings/_performance_bar.html.haml' do
+              element :enable_performance_bar_checkbox
+              element :save_changes_button
+            end
+
+            def enable_performance_bar
+              click_element :enable_performance_bar_checkbox
+              Capybara.current_session.driver.browser.manage.add_cookie(name: 'perf_bar_enabled', value: 'true')
+            end
+
+            def save_settings
+              click_element :save_changes_button
+            end
+          end
+        end
+      end
+    end
+  end
+end

qa/qa/page/admin/settings/metrics_and_profiling.rb

+# frozen_string_literal: true
+
+module QA
+  module Page
+    module Admin
+      module Settings
+        class MetricsAndProfiling < Page::Base
+          include QA::Page::Settings::Common
+
+          view 'app/views/admin/application_settings/metrics_and_profiling.html.haml' do
+            element :performance_bar_settings
+          end
+
+          def expand_performance_bar(&block)
+            expand_section(:performance_bar_settings) do
+              Component::PerformanceBar.perform(&block)
+            end
+          end
+        end
+      end
+    end
+  end
+end

qa/qa/page/layout/performance_bar.rb

+# frozen_string_literal: true
+
+module QA
+  module Page
+    module Layout
+      class PerformanceBar < Page::Base
+        view 'app/assets/javascripts/performance_bar/components/performance_bar_app.vue' do
+          element :performance_bar
+        end
+
+        view 'app/assets/javascripts/performance_bar/components/detailed_metric.vue' do
+          element :performance_bar_detailed_metric
+        end
+
+        view 'app/assets/javascripts/performance_bar/components/request_selector.vue' do
+          element :performance_bar_request
+        end
+
+        def has_performance_bar?
+          has_element?(:performance_bar)
+        end
+
+        def has_detailed_metrics?
+          all_elements(:performance_bar_detailed_metric).all? do |metric|
+            metric.has_text?(%r{\d+ms / \d+})
+          end
+        end
+
+        def has_request_for?(path)
+          has_element?(:performance_bar_request, text: path)
+        end
+      end
+    end
+  end
+end

qa/qa/specs/features/browser_ui/non_devops/performance_bar_spec.rb

+# frozen_string_literal: true
+
+module QA
+  context 'Performance bar' do
+    context 'when logged in as an admin user' do
+      before do
+        Runtime::Browser.visit(:gitlab, Page::Main::Login)
+        Page::Main::Login.perform(&:sign_in_using_admin_credentials)
+        Page::Main::Menu.perform(&:click_admin_area)
+        Page::Admin::Menu.perform(&:go_to_metrics_and_profiling_settings)
+
+        Page::Admin::Settings::MetricsAndProfiling.perform do |setting|
+          setting.expand_performance_bar do |page|
+            page.enable_performance_bar
+            page.save_settings
+          end
+        end
+      end
+
+      it 'shows results for the original request and AJAX requests' do
+        # Issue pages always make AJAX requests
+        Resource::Issue.fabricate! do |issue|
+          issue.title = 'Performance bar test'
+        end
+
+        Page::Layout::PerformanceBar.perform do |page|
+          expect(page).to have_performance_bar
+          expect(page).to have_detailed_metrics
+          expect(page).to have_request_for('realtime_changes') # Always requested on issue pages
+        end
+      end
+    end
+  end
+end