diff options
86 files changed, 2995 insertions, 310 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index e10958b3bee..024f2929252 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -27,6 +27,7 @@ variables: GET_SOURCES_ATTEMPTS: "3" KNAPSACK_RSPEC_SUITE_REPORT_PATH: knapsack/${CI_PROJECT_NAME}/rspec_report-master.json KNAPSACK_SPINACH_SUITE_REPORT_PATH: knapsack/${CI_PROJECT_NAME}/spinach_report-master.json + FLAKY_RSPEC_SUITE_REPORT_PATH: rspec_flaky/${CI_PROJECT_NAME}/report-master.json before_script: - bundle --version @@ -45,16 +46,17 @@ stages: tags: - gitlab-org -.knapsack-state: &knapsack-state +.tests-metadata-state: &tests-metadata-state services: [] variables: SETUP_DB: "false" USE_BUNDLE_INSTALL: "false" - KNAPSACK_S3_BUCKET: "gitlab-ce-cache" + TESTS_METADATA_S3_BUCKET: "gitlab-ce-cache" artifacts: expire_in: 31d paths: - knapsack/ + - rspec_flaky/ .use-pg: &use-pg services: @@ -86,7 +88,7 @@ stages: except: - /(^docs[\/-].*|.*-docs$)/ -.rspec-knapsack: &rspec-knapsack +.rspec-metadata: &rspec-metadata <<: *dedicated-runner <<: *pull-cache stage: test @@ -96,8 +98,13 @@ stages: - export CI_NODE_TOTAL=${JOB_NAME[-1]} - export KNAPSACK_REPORT_PATH=knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json - export KNAPSACK_GENERATE_REPORT=true + - export ALL_FLAKY_RSPEC_REPORT_PATH=rspec_flaky/${CI_PROJECT_NAME}/all_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json + - export NEW_FLAKY_RSPEC_REPORT_PATH=rspec_flaky/${CI_PROJECT_NAME}/new_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json + - export FLAKY_RSPEC_GENERATE_REPORT=true - export CACHE_CLASSES=true - cp ${KNAPSACK_RSPEC_SUITE_REPORT_PATH} ${KNAPSACK_REPORT_PATH} + - cp ${FLAKY_RSPEC_SUITE_REPORT_PATH} ${ALL_FLAKY_RSPEC_REPORT_PATH} + - '[[ -f $NEW_FLAKY_RSPEC_REPORT_PATH ]] || echo "{}" > ${NEW_FLAKY_RSPEC_REPORT_PATH}' - scripts/gitaly-test-spawn - knapsack rspec "--color --format documentation" artifacts: @@ -106,20 +113,21 @@ stages: paths: - coverage/ - knapsack/ + - rspec_flaky/ - tmp/capybara/ -.rspec-knapsack-pg: &rspec-knapsack-pg - <<: *rspec-knapsack +.rspec-metadata-pg: &rspec-metadata-pg + <<: *rspec-metadata <<: *use-pg <<: *except-docs -.rspec-knapsack-mysql: &rspec-knapsack-mysql - <<: *rspec-knapsack +.rspec-metadata-mysql: &rspec-metadata-mysql + <<: *rspec-metadata <<: *use-mysql <<: *only-if-want-mysql <<: *except-docs -.spinach-knapsack: &spinach-knapsack +.spinach-metadata: &spinach-metadata <<: *dedicated-runner <<: *pull-cache stage: test @@ -140,13 +148,13 @@ stages: - knapsack/ - tmp/capybara/ -.spinach-knapsack-pg: &spinach-knapsack-pg - <<: *spinach-knapsack +.spinach-metadata-pg: &spinach-metadata-pg + <<: *spinach-metadata <<: *use-pg <<: *except-docs -.spinach-knapsack-mysql: &spinach-knapsack-mysql - <<: *spinach-knapsack +.spinach-metadata-mysql: &spinach-metadata-mysql + <<: *spinach-metadata <<: *use-mysql <<: *only-if-want-mysql <<: *except-docs @@ -176,40 +184,70 @@ build-package: - //@gitlab-org/gitlab-ce - //@gitlab-org/gitlab-ee -# Prepare and merge knapsack tests -knapsack: - <<: *knapsack-state +# Retrieve knapsack and rspec_flaky reports +retrieve-tests-metadata: + <<: *tests-metadata-state <<: *dedicated-runner <<: *except-docs stage: prepare cache: - key: knapsack - paths: - - knapsack/ + key: tests_metadata policy: pull script: - mkdir -p knapsack/${CI_PROJECT_NAME}/ - - wget -O $KNAPSACK_RSPEC_SUITE_REPORT_PATH http://${KNAPSACK_S3_BUCKET}.s3.amazonaws.com/$KNAPSACK_RSPEC_SUITE_REPORT_PATH || rm $KNAPSACK_RSPEC_SUITE_REPORT_PATH - - wget -O $KNAPSACK_SPINACH_SUITE_REPORT_PATH http://${KNAPSACK_S3_BUCKET}.s3.amazonaws.com/$KNAPSACK_SPINACH_SUITE_REPORT_PATH || rm $KNAPSACK_SPINACH_SUITE_REPORT_PATH + - wget -O $KNAPSACK_RSPEC_SUITE_REPORT_PATH http://${TESTS_METADATA_S3_BUCKET}.s3.amazonaws.com/$KNAPSACK_RSPEC_SUITE_REPORT_PATH || rm $KNAPSACK_RSPEC_SUITE_REPORT_PATH + - wget -O $KNAPSACK_SPINACH_SUITE_REPORT_PATH http://${TESTS_METADATA_S3_BUCKET}.s3.amazonaws.com/$KNAPSACK_SPINACH_SUITE_REPORT_PATH || rm $KNAPSACK_SPINACH_SUITE_REPORT_PATH - '[[ -f $KNAPSACK_RSPEC_SUITE_REPORT_PATH ]] || echo "{}" > ${KNAPSACK_RSPEC_SUITE_REPORT_PATH}' - '[[ -f $KNAPSACK_SPINACH_SUITE_REPORT_PATH ]] || echo "{}" > ${KNAPSACK_SPINACH_SUITE_REPORT_PATH}' + - mkdir -p rspec_flaky/${CI_PROJECT_NAME}/ + - wget -O $FLAKY_RSPEC_SUITE_REPORT_PATH http://${TESTS_METADATA_S3_BUCKET}.s3.amazonaws.com/$FLAKY_RSPEC_SUITE_REPORT_PATH || rm $FLAKY_RSPEC_SUITE_REPORT_PATH + - '[[ -f $FLAKY_RSPEC_SUITE_REPORT_PATH ]] || echo "{}" > ${FLAKY_RSPEC_SUITE_REPORT_PATH}' -update-knapsack: - <<: *knapsack-state +update-tests-metadata: + <<: *tests-metadata-state <<: *dedicated-runner <<: *only-canonical-masters stage: post-test cache: - key: knapsack + key: tests_metadata paths: - knapsack/ + - rspec_flaky/ policy: push script: - retry gem install fog-aws mime-types - scripts/merge-reports ${KNAPSACK_RSPEC_SUITE_REPORT_PATH} knapsack/${CI_PROJECT_NAME}/rspec-pg_node_*.json - scripts/merge-reports ${KNAPSACK_SPINACH_SUITE_REPORT_PATH} knapsack/${CI_PROJECT_NAME}/spinach-pg_node_*.json - - '[[ -z ${KNAPSACK_S3_BUCKET} ]] || scripts/sync-reports put $KNAPSACK_S3_BUCKET $KNAPSACK_RSPEC_SUITE_REPORT_PATH $KNAPSACK_SPINACH_SUITE_REPORT_PATH' + - scripts/merge-reports ${FLAKY_RSPEC_SUITE_REPORT_PATH} rspec_flaky/${CI_PROJECT_NAME}/all_node_*.json + - '[[ -z ${TESTS_METADATA_S3_BUCKET} ]] || scripts/sync-reports put $TESTS_METADATA_S3_BUCKET $KNAPSACK_RSPEC_SUITE_REPORT_PATH $KNAPSACK_SPINACH_SUITE_REPORT_PATH' + - '[[ -z ${TESTS_METADATA_S3_BUCKET} ]] || scripts/sync-reports put $TESTS_METADATA_S3_BUCKET $FLAKY_RSPEC_SUITE_REPORT_PATH' - rm -f knapsack/${CI_PROJECT_NAME}/*_node_*.json + - rm -f rspec_flaky/${CI_PROJECT_NAME}/all_node_*.json + +flaky-examples-check: + <<: *dedicated-runner + image: ruby:2.3-alpine + services: [] + before_script: [] + cache: {} + variables: + SETUP_DB: "false" + USE_BUNDLE_INSTALL: "false" + NEW_FLAKY_SPECS_REPORT: rspec_flaky/${CI_PROJECT_NAME}/new_rspec_flaky_examples.json + stage: post-test + allow_failure: yes + only: + - branches + except: + - master + artifacts: + expire_in: 30d + paths: + - rspec_flaky/ + script: + - '[[ -f $NEW_FLAKY_SPECS_REPORT ]] || echo "{}" > ${NEW_FLAKY_SPECS_REPORT}' + - scripts/merge-reports $NEW_FLAKY_SPECS_REPORT rspec_flaky/${CI_PROJECT_NAME}/new_node_*.json + - scripts/detect-new-flaky-examples $NEW_FLAKY_SPECS_REPORT setup-test-env: <<: *use-pg @@ -232,69 +270,69 @@ setup-test-env: - public/assets - tmp/tests -rspec-pg 0 25: *rspec-knapsack-pg -rspec-pg 1 25: *rspec-knapsack-pg -rspec-pg 2 25: *rspec-knapsack-pg -rspec-pg 3 25: *rspec-knapsack-pg -rspec-pg 4 25: *rspec-knapsack-pg -rspec-pg 5 25: *rspec-knapsack-pg -rspec-pg 6 25: *rspec-knapsack-pg -rspec-pg 7 25: *rspec-knapsack-pg -rspec-pg 8 25: *rspec-knapsack-pg -rspec-pg 9 25: *rspec-knapsack-pg -rspec-pg 10 25: *rspec-knapsack-pg -rspec-pg 11 25: *rspec-knapsack-pg -rspec-pg 12 25: *rspec-knapsack-pg -rspec-pg 13 25: *rspec-knapsack-pg -rspec-pg 14 25: *rspec-knapsack-pg -rspec-pg 15 25: *rspec-knapsack-pg -rspec-pg 16 25: *rspec-knapsack-pg -rspec-pg 17 25: *rspec-knapsack-pg -rspec-pg 18 25: *rspec-knapsack-pg -rspec-pg 19 25: *rspec-knapsack-pg -rspec-pg 20 25: *rspec-knapsack-pg -rspec-pg 21 25: *rspec-knapsack-pg -rspec-pg 22 25: *rspec-knapsack-pg -rspec-pg 23 25: *rspec-knapsack-pg -rspec-pg 24 25: *rspec-knapsack-pg - -rspec-mysql 0 25: *rspec-knapsack-mysql -rspec-mysql 1 25: *rspec-knapsack-mysql -rspec-mysql 2 25: *rspec-knapsack-mysql -rspec-mysql 3 25: *rspec-knapsack-mysql -rspec-mysql 4 25: *rspec-knapsack-mysql -rspec-mysql 5 25: *rspec-knapsack-mysql -rspec-mysql 6 25: *rspec-knapsack-mysql -rspec-mysql 7 25: *rspec-knapsack-mysql -rspec-mysql 8 25: *rspec-knapsack-mysql -rspec-mysql 9 25: *rspec-knapsack-mysql -rspec-mysql 10 25: *rspec-knapsack-mysql -rspec-mysql 11 25: *rspec-knapsack-mysql -rspec-mysql 12 25: *rspec-knapsack-mysql -rspec-mysql 13 25: *rspec-knapsack-mysql -rspec-mysql 14 25: *rspec-knapsack-mysql -rspec-mysql 15 25: *rspec-knapsack-mysql -rspec-mysql 16 25: *rspec-knapsack-mysql -rspec-mysql 17 25: *rspec-knapsack-mysql -rspec-mysql 18 25: *rspec-knapsack-mysql -rspec-mysql 19 25: *rspec-knapsack-mysql -rspec-mysql 20 25: *rspec-knapsack-mysql -rspec-mysql 21 25: *rspec-knapsack-mysql -rspec-mysql 22 25: *rspec-knapsack-mysql -rspec-mysql 23 25: *rspec-knapsack-mysql -rspec-mysql 24 25: *rspec-knapsack-mysql - -spinach-pg 0 5: *spinach-knapsack-pg -spinach-pg 1 5: *spinach-knapsack-pg -spinach-pg 2 5: *spinach-knapsack-pg -spinach-pg 3 5: *spinach-knapsack-pg -spinach-pg 4 5: *spinach-knapsack-pg - -spinach-mysql 0 5: *spinach-knapsack-mysql -spinach-mysql 1 5: *spinach-knapsack-mysql -spinach-mysql 2 5: *spinach-knapsack-mysql -spinach-mysql 3 5: *spinach-knapsack-mysql -spinach-mysql 4 5: *spinach-knapsack-mysql +rspec-pg 0 25: *rspec-metadata-pg +rspec-pg 1 25: *rspec-metadata-pg +rspec-pg 2 25: *rspec-metadata-pg +rspec-pg 3 25: *rspec-metadata-pg +rspec-pg 4 25: *rspec-metadata-pg +rspec-pg 5 25: *rspec-metadata-pg +rspec-pg 6 25: *rspec-metadata-pg +rspec-pg 7 25: *rspec-metadata-pg +rspec-pg 8 25: *rspec-metadata-pg +rspec-pg 9 25: *rspec-metadata-pg +rspec-pg 10 25: *rspec-metadata-pg +rspec-pg 11 25: *rspec-metadata-pg +rspec-pg 12 25: *rspec-metadata-pg +rspec-pg 13 25: *rspec-metadata-pg +rspec-pg 14 25: *rspec-metadata-pg +rspec-pg 15 25: *rspec-metadata-pg +rspec-pg 16 25: *rspec-metadata-pg +rspec-pg 17 25: *rspec-metadata-pg +rspec-pg 18 25: *rspec-metadata-pg +rspec-pg 19 25: *rspec-metadata-pg +rspec-pg 20 25: *rspec-metadata-pg +rspec-pg 21 25: *rspec-metadata-pg +rspec-pg 22 25: *rspec-metadata-pg +rspec-pg 23 25: *rspec-metadata-pg +rspec-pg 24 25: *rspec-metadata-pg + +rspec-mysql 0 25: *rspec-metadata-mysql +rspec-mysql 1 25: *rspec-metadata-mysql +rspec-mysql 2 25: *rspec-metadata-mysql +rspec-mysql 3 25: *rspec-metadata-mysql +rspec-mysql 4 25: *rspec-metadata-mysql +rspec-mysql 5 25: *rspec-metadata-mysql +rspec-mysql 6 25: *rspec-metadata-mysql +rspec-mysql 7 25: *rspec-metadata-mysql +rspec-mysql 8 25: *rspec-metadata-mysql +rspec-mysql 9 25: *rspec-metadata-mysql +rspec-mysql 10 25: *rspec-metadata-mysql +rspec-mysql 11 25: *rspec-metadata-mysql +rspec-mysql 12 25: *rspec-metadata-mysql +rspec-mysql 13 25: *rspec-metadata-mysql +rspec-mysql 14 25: *rspec-metadata-mysql +rspec-mysql 15 25: *rspec-metadata-mysql +rspec-mysql 16 25: *rspec-metadata-mysql +rspec-mysql 17 25: *rspec-metadata-mysql +rspec-mysql 18 25: *rspec-metadata-mysql +rspec-mysql 19 25: *rspec-metadata-mysql +rspec-mysql 20 25: *rspec-metadata-mysql +rspec-mysql 21 25: *rspec-metadata-mysql +rspec-mysql 22 25: *rspec-metadata-mysql +rspec-mysql 23 25: *rspec-metadata-mysql +rspec-mysql 24 25: *rspec-metadata-mysql + +spinach-pg 0 5: *spinach-metadata-pg +spinach-pg 1 5: *spinach-metadata-pg +spinach-pg 2 5: *spinach-metadata-pg +spinach-pg 3 5: *spinach-metadata-pg +spinach-pg 4 5: *spinach-metadata-pg + +spinach-mysql 0 5: *spinach-metadata-mysql +spinach-mysql 1 5: *spinach-metadata-mysql +spinach-mysql 2 5: *spinach-metadata-mysql +spinach-mysql 3 5: *spinach-metadata-mysql +spinach-mysql 4 5: *spinach-metadata-mysql # Static analysis jobs .ruby-static-analysis: &ruby-static-analysis @@ -354,7 +392,7 @@ ee_compat_check: except: - master - tags - - /^[\d-]+-stable(-ee)?$/ + - /^[\d-]+-stable(-ee)?/ allow_failure: yes cache: key: "ee_compat_check_repo" diff --git a/.rubocop.yml b/.rubocop.yml index 18d8e009da6..d25b4ac39c9 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1045,7 +1045,7 @@ RSpec/BeforeAfterAll: RSpec/DescribeClass: Enabled: false -# Use `described_class` for tested class / module. +# Checks that the second argument to `describe` specifies a method. RSpec/DescribeMethod: Enabled: false @@ -1053,8 +1053,7 @@ RSpec/DescribeMethod: RSpec/DescribeSymbol: Enabled: true -# Checks that the second argument to top level describe is the tested method -# name. +# Checks that tests use `described_class`. RSpec/DescribedClass: Enabled: true diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 02e0ba74158..1bb04b59a2a 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -725,9 +725,9 @@ } // TODO: change global style and remove mixin -@mixin new-style-dropdown { - .dropdown-menu, - .dropdown-menu-nav { +@mixin new-style-dropdown($selector: '') { + #{$selector}.dropdown-menu, + #{$selector}.dropdown-menu-nav { .divider { margin: 6px 0; } @@ -773,7 +773,7 @@ } } - .dropdown-menu-align-right { + #{$selector}.dropdown-menu-align-right { margin-top: 2px; } } diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index bf5f124d142..96409b10b99 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -339,6 +339,8 @@ a > code { @extend .ref-name; } +@include new-style-dropdown('.git-revision-dropdown'); + /** * Apply Markdown typography * diff --git a/app/controllers/admin/appearances_controller.rb b/app/controllers/admin/appearances_controller.rb index 4b0ec54b3f4..92df1c8dff0 100644 --- a/app/controllers/admin/appearances_controller.rb +++ b/app/controllers/admin/appearances_controller.rb @@ -45,7 +45,7 @@ class Admin::AppearancesController < Admin::ApplicationController # Use callbacks to share common setup or constraints between actions. def set_appearance - @appearance = Appearance.last || Appearance.new + @appearance = Appearance.current || Appearance.new end # Only allow a trusted parameter "white list" through. diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb index ea441b1736b..b75e401a8df 100644 --- a/app/controllers/concerns/authenticates_with_two_factor.rb +++ b/app/controllers/concerns/authenticates_with_two_factor.rb @@ -69,7 +69,7 @@ module AuthenticatesWithTwoFactor if U2fRegistration.authenticate(user, u2f_app_id, user_params[:device_response], session[:challenge]) # Remove any lingering user data from login session.delete(:otp_user_id) - session.delete(:challenges) + session.delete(:challenge) remember_me(user) if user_params[:remember_me] == '1' sign_in(user) diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index 74fe45e1ff6..f71ab702e71 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -52,8 +52,10 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController end def load_events - @events = Event.in_projects(load_projects(params.merge(non_public: true))) - @events = event_filter.apply_filter(@events).with_associations - @events = @events.limit(20).offset(params[:offset] || 0) + projects = load_projects(params.merge(non_public: true)) + + @events = EventCollection + .new(projects, offset: params[:offset].to_i, filter: event_filter) + .to_a end end diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index f9c31920302..19a5db6fd17 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -29,9 +29,9 @@ class DashboardController < Dashboard::ApplicationController current_user.authorized_projects end - @events = Event.in_projects(projects) - @events = @event_filter.apply_filter(@events).with_associations - @events = @events.limit(20).offset(params[:offset] || 0) + @events = EventCollection + .new(projects, offset: params[:offset].to_i, filter: @event_filter) + .to_a end def set_show_full_reference diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 27137ffde54..f76b3f69e9e 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -160,9 +160,9 @@ class GroupsController < Groups::ApplicationController end def load_events - @events = Event.in_projects(@projects) - @events = event_filter.apply_filter(@events).with_associations - @events = @events.limit(20).offset(params[:offset] || 0) + @events = EventCollection + .new(@projects, offset: params[:offset].to_i, filter: event_filter) + .to_a end def user_actions diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 8dfe0f51709..e93f34498d6 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -301,10 +301,11 @@ class ProjectsController < Projects::ApplicationController end def load_events - @events = @project.events.recent - @events = event_filter.apply_filter(@events).with_associations - limit = (params[:limit] || 20).to_i - @events = @events.limit(limit).offset(params[:offset] || 0) + projects = Project.where(id: @project.id) + + @events = EventCollection + .new(projects, offset: params[:offset].to_i, filter: event_filter) + .to_a end def project_params diff --git a/app/finders/admin/projects_finder.rb b/app/finders/admin/projects_finder.rb index a5ba791a513..7176bfe22d6 100644 --- a/app/finders/admin/projects_finder.rb +++ b/app/finders/admin/projects_finder.rb @@ -18,7 +18,7 @@ class Admin::ProjectsFinder end def execute - items = Project.with_statistics + items = Project.without_deleted.with_statistics items = items.in_namespace(namespace_id) if namespace_id.present? items = items.where(visibility_level: visibility_level) if visibility_level.present? items = items.with_push if with_push.present? diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb index 16136d02530..cdf5fa5d4b7 100644 --- a/app/helpers/appearances_helper.rb +++ b/app/helpers/appearances_helper.rb @@ -20,7 +20,7 @@ module AppearancesHelper end def brand_item - @appearance ||= Appearance.first + @appearance ||= Appearance.current end def brand_header_logo diff --git a/app/models/appearance.rb b/app/models/appearance.rb index f9c48482be7..ff15689ecac 100644 --- a/app/models/appearance.rb +++ b/app/models/appearance.rb @@ -8,7 +8,27 @@ class Appearance < ActiveRecord::Base validates :logo, file_size: { maximum: 1.megabyte } validates :header_logo, file_size: { maximum: 1.megabyte } + validate :single_appearance_row, on: :create + mount_uploader :logo, AttachmentUploader mount_uploader :header_logo, AttachmentUploader has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + + CACHE_KEY = 'current_appearance'.freeze + + after_commit :flush_redis_cache + + def self.current + Rails.cache.fetch(CACHE_KEY) { first } + end + + def flush_redis_cache + Rails.cache.delete(CACHE_KEY) + end + + def single_appearance_row + if self.class.any? + errors.add(:single_appearance_row, 'Only 1 appearances row can exist') + end + end end diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index 944725d91c3..3692bcc680d 100644 --- a/app/models/broadcast_message.rb +++ b/app/models/broadcast_message.rb @@ -14,9 +14,15 @@ class BroadcastMessage < ActiveRecord::Base default_value_for :color, '#E75E40' default_value_for :font, '#FFFFFF' + CACHE_KEY = 'broadcast_message_current'.freeze + + after_commit :flush_redis_cache + def self.current - Rails.cache.fetch("broadcast_message_current", expires_in: 1.minute) do - where('ends_at > :now AND starts_at <= :now', now: Time.zone.now).order([:created_at, :id]).to_a + Rails.cache.fetch(CACHE_KEY) do + where('ends_at > :now AND starts_at <= :now', now: Time.zone.now) + .reorder(id: :asc) + .to_a end end @@ -31,4 +37,8 @@ class BroadcastMessage < ActiveRecord::Base def ended? ends_at < Time.zone.now end + + def flush_redis_cache + Rails.cache.delete(CACHE_KEY) + end end diff --git a/app/models/event.rb b/app/models/event.rb index 8d93a228494..f2a560a6b56 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -48,6 +48,7 @@ class Event < ActiveRecord::Base belongs_to :author, class_name: "User" belongs_to :project belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations + has_one :push_event_payload, foreign_key: :event_id # For Hash only serialize :data # rubocop:disable Cop/ActiveRecordSerialize @@ -55,19 +56,51 @@ class Event < ActiveRecord::Base # Callbacks after_create :reset_project_activity after_create :set_last_repository_updated_at, if: :push? + after_create :replicate_event_for_push_events_migration # Scopes scope :recent, -> { reorder(id: :desc) } scope :code_push, -> { where(action: PUSHED) } - scope :in_projects, ->(projects) do - where(project_id: projects.pluck(:id)).recent + scope :in_projects, -> (projects) do + sub_query = projects + .except(:order) + .select(1) + .where('projects.id = events.project_id') + + where('EXISTS (?)', sub_query).recent + end + + scope :with_associations, -> do + # We're using preload for "push_event_payload" as otherwise the association + # is not always available (depending on the query being built). + includes(:author, :project, project: :namespace) + .preload(:target, :push_event_payload) end - scope :with_associations, -> { includes(:author, :project, project: :namespace).preload(:target) } scope :for_milestone_id, ->(milestone_id) { where(target_type: "Milestone", target_id: milestone_id) } + self.inheritance_column = 'action' + class << self + def find_sti_class(action) + if action.to_i == PUSHED + PushEvent + else + Event + end + end + + def subclass_from_attributes(attrs) + # Without this Rails will keep calling this method on the returned class, + # resulting in an infinite loop. + return unless self == Event + + action = attrs.with_indifferent_access[inheritance_column].to_i + + PushEvent if action == PUSHED + end + # Update Gitlab::ContributionsCalendar#activity_dates if this changes def contributions where("action = ? OR (target_type IN (?) AND action IN (?)) OR (target_type = ? AND action = ?)", @@ -290,6 +323,16 @@ class Event < ActiveRecord::Base @commits ||= (data[:commits] || []).reverse end + def commit_title + commit = commits.last + + commit[:message] if commit + end + + def commit_id + commit_to || commit_from + end + def commits_count data[:total_commits_count] || commits.count || 0 end @@ -385,6 +428,16 @@ class Event < ActiveRecord::Base user ? author_id == user.id : false end + # We're manually replicating data into the new table since database triggers + # are not dumped to db/schema.rb. This could mean that a new installation + # would not have the triggers in place, thus losing events data in GitLab + # 10.0. + def replicate_event_for_push_events_migration + new_attributes = attributes.with_indifferent_access.except(:title, :data) + + EventForMigration.create!(new_attributes) + end + private def recent_update? diff --git a/app/models/event_collection.rb b/app/models/event_collection.rb new file mode 100644 index 00000000000..8b8244314af --- /dev/null +++ b/app/models/event_collection.rb @@ -0,0 +1,98 @@ +# A collection of events to display in an event list. +# +# An EventCollection is meant to be used for displaying events to a user (e.g. +# in a controller), it's not suitable for building queries that are used for +# building other queries. +class EventCollection + # To prevent users from putting too much pressure on the database by cycling + # through thousands of events we put a limit on the number of pages. + MAX_PAGE = 10 + + # projects - An ActiveRecord::Relation object that returns the projects for + # which to retrieve events. + # filter - An EventFilter instance to use for filtering events. + def initialize(projects, limit: 20, offset: 0, filter: nil) + @projects = projects + @limit = limit + @offset = offset + @filter = filter + end + + # Returns an Array containing the events. + def to_a + return [] if current_page > MAX_PAGE + + relation = if Gitlab::Database.join_lateral_supported? + relation_with_join_lateral + else + relation_without_join_lateral + end + + relation.with_associations.to_a + end + + private + + # Returns the events relation to use when JOIN LATERAL is not supported. + # + # This relation simply gets all the events for all authorized projects, then + # limits that set. + def relation_without_join_lateral + events = filtered_events.in_projects(projects) + + paginate_events(events) + end + + # Returns the events relation to use when JOIN LATERAL is supported. + # + # This relation is built using JOIN LATERAL, producing faster queries than a + # regular LIMIT + OFFSET approach. + def relation_with_join_lateral + projects_for_lateral = projects.select(:id).to_sql + + lateral = filtered_events + .limit(limit_for_join_lateral) + .where('events.project_id = projects_for_lateral.id') + .to_sql + + # The outer query does not need to re-apply the filters since the JOIN + # LATERAL body already takes care of this. + outer = base_relation + .from("(#{projects_for_lateral}) projects_for_lateral") + .joins("JOIN LATERAL (#{lateral}) AS #{Event.table_name} ON true") + + paginate_events(outer) + end + + def filtered_events + @filter ? @filter.apply_filter(base_relation) : base_relation + end + + def paginate_events(events) + events.limit(@limit).offset(@offset) + end + + def base_relation + # We want to have absolute control over the event queries being built, thus + # we're explicitly opting out of any default scopes that may be set. + Event.unscoped.recent + end + + def limit_for_join_lateral + # Applying the OFFSET on the inside of a JOIN LATERAL leads to incorrect + # results. To work around this we need to increase the inner limit for every + # page. + # + # This means that on page 1 we use LIMIT 20, and an outer OFFSET of 0. On + # page 2 we use LIMIT 40 and an outer OFFSET of 20. + @limit + @offset + end + + def current_page + (@offset / @limit) + 1 + end + + def projects + @projects.except(:order) + end +end diff --git a/app/models/event_for_migration.rb b/app/models/event_for_migration.rb new file mode 100644 index 00000000000..a1672da5eec --- /dev/null +++ b/app/models/event_for_migration.rb @@ -0,0 +1,5 @@ +# This model is used to replicate events between the old "events" table and the +# new "events_for_migration" table that will replace "events" in GitLab 10.0. +class EventForMigration < ActiveRecord::Base + self.table_name = 'events_for_migration' +end diff --git a/app/models/push_event.rb b/app/models/push_event.rb new file mode 100644 index 00000000000..3f1ff979de6 --- /dev/null +++ b/app/models/push_event.rb @@ -0,0 +1,126 @@ +class PushEvent < Event + # This validation exists so we can't accidentally use PushEvent with a + # different "action" value. + validate :validate_push_action + + # Authors are required as they're used to display who pushed data. + # + # We're just validating the presence of the ID here as foreign key constraints + # should ensure the ID points to a valid user. + validates :author_id, presence: true + + # The project is required to build links to commits, commit ranges, etc. + # + # We're just validating the presence of the ID here as foreign key constraints + # should ensure the ID points to a valid project. + validates :project_id, presence: true + + # The "data" field must not be set for push events since it's not used and a + # waste of space. + validates :data, absence: true + + # These fields are also not used for push events, thus storing them would be a + # waste. + validates :target_id, absence: true + validates :target_type, absence: true + + def self.sti_name + PUSHED + end + + def push? + true + end + + def push_with_commits? + !!(commit_from && commit_to) + end + + def tag? + return super unless push_event_payload + + push_event_payload.tag? + end + + def branch? + return super unless push_event_payload + + push_event_payload.branch? + end + + def valid_push? + return super unless push_event_payload + + push_event_payload.ref.present? + end + + def new_ref? + return super unless push_event_payload + + push_event_payload.created? + end + + def rm_ref? + return super unless push_event_payload + + push_event_payload.removed? + end + + def commit_from + return super unless push_event_payload + + push_event_payload.commit_from + end + + def commit_to + return super unless push_event_payload + + push_event_payload.commit_to + end + + def ref_name + return super unless push_event_payload + + push_event_payload.ref + end + + def ref_type + return super unless push_event_payload + + push_event_payload.ref_type + end + + def branch_name + return super unless push_event_payload + + ref_name + end + + def tag_name + return super unless push_event_payload + + ref_name + end + + def commit_title + return super unless push_event_payload + + push_event_payload.commit_title + end + + def commit_id + commit_to || commit_from + end + + def commits_count + return super unless push_event_payload + + push_event_payload.commit_count + end + + def validate_push_action + return if action == PUSHED + + errors.add(:action, "the action #{action.inspect} is not valid") + end +end diff --git a/app/models/push_event_payload.rb b/app/models/push_event_payload.rb new file mode 100644 index 00000000000..6cdb1cd4fe9 --- /dev/null +++ b/app/models/push_event_payload.rb @@ -0,0 +1,22 @@ +class PushEventPayload < ActiveRecord::Base + include ShaAttribute + + belongs_to :event, inverse_of: :push_event_payload + + validates :event_id, :commit_count, :action, :ref_type, presence: true + validates :commit_title, length: { maximum: 70 } + + sha_attribute :commit_from + sha_attribute :commit_to + + enum action: { + created: 0, + removed: 1, + pushed: 2 + } + + enum ref_type: { + branch: 0, + tag: 1 + } +end diff --git a/app/services/event_create_service.rb b/app/services/event_create_service.rb index 0f3a485a3fd..0b7e4f187f7 100644 --- a/app/services/event_create_service.rb +++ b/app/services/event_create_service.rb @@ -71,7 +71,14 @@ class EventCreateService end def push(project, current_user, push_data) - create_event(project, current_user, Event::PUSHED, data: push_data) + # We're using an explicit transaction here so that any errors that may occur + # when creating push payload data will result in the event creation being + # rolled back as well. + Event.transaction do + event = create_event(project, current_user, Event::PUSHED) + + PushEventPayloadService.new(event, push_data).execute + end Users::ActivityService.new(current_user, 'push').execute end diff --git a/app/services/push_event_payload_service.rb b/app/services/push_event_payload_service.rb new file mode 100644 index 00000000000..b0a389c85f9 --- /dev/null +++ b/app/services/push_event_payload_service.rb @@ -0,0 +1,120 @@ +# Service class for creating push event payloads as stored in the +# "push_event_payloads" table. +# +# Example: +# +# data = Gitlab::DataBuilder::Push.build(...) +# event = Event.create(...) +# +# PushEventPayloadService.new(event, data).execute +class PushEventPayloadService + # event - The event this push payload belongs to. + # push_data - A Hash produced by `Gitlab::DataBuilder::Push.build` to use for + # building the push payload. + def initialize(event, push_data) + @event = event + @push_data = push_data + end + + # Creates and returns a new PushEventPayload row. + # + # This method will raise upon encountering validation errors. + # + # Returns an instance of PushEventPayload. + def execute + @event.build_push_event_payload( + commit_count: commit_count, + action: action, + ref_type: ref_type, + commit_from: commit_from_id, + commit_to: commit_to_id, + ref: trimmed_ref, + commit_title: commit_title, + event_id: @event.id + ) + + @event.push_event_payload.save! + @event.push_event_payload + end + + # Returns the commit title to use. + # + # The commit title is limited to the first line and a maximum of 70 + # characters. + def commit_title + commit = @push_data.fetch(:commits).last + + return nil unless commit && commit[:message] + + raw_msg = commit[:message] + + # Find where the first line ends, without turning the entire message into an + # Array of lines (this is a waste of memory for large commit messages). + index = raw_msg.index("\n") + message = index ? raw_msg[0..index] : raw_msg + + message.strip.truncate(70) + end + + def commit_from_id + if create? + nil + else + revision_before + end + end + + def commit_to_id + if remove? + nil + else + revision_after + end + end + + def commit_count + @push_data.fetch(:total_commits_count) + end + + def ref + @push_data.fetch(:ref) + end + + def revision_before + @push_data.fetch(:before) + end + + def revision_after + @push_data.fetch(:after) + end + + def trimmed_ref + Gitlab::Git.ref_name(ref) + end + + def create? + Gitlab::Git.blank_ref?(revision_before) + end + + def remove? + Gitlab::Git.blank_ref?(revision_after) + end + + def action + if create? + :created + elsif remove? + :removed + else + :pushed + end + end + + def ref_type + if Gitlab::Git.tag_ref?(ref) + :tag + else + :branch + end + end +end diff --git a/app/views/events/_commit.html.haml b/app/views/events/_commit.html.haml index ad434a64556..98cdcca3ecc 100644 --- a/app/views/events/_commit.html.haml +++ b/app/views/events/_commit.html.haml @@ -1,5 +1,5 @@ %li.commit .commit-row-title - = link_to truncate_sha(commit[:id]), project_commit_path(project, commit[:id]), class: "commit-sha", alt: '', title: truncate_sha(commit[:id]) + = link_to truncate_sha(event.commit_id), project_commit_path(project, event.commit_id), class: "commit-sha", alt: '', title: truncate_sha(event.commit_id) · - = markdown event_commit_title(commit[:message]), project: project, pipeline: :single_line, author: event.author + = markdown event_commit_title(event.commit_title), project: project, pipeline: :single_line, author: event.author diff --git a/app/views/events/_event_push.atom.haml b/app/views/events/_event_push.atom.haml index 9fcacfbbf36..bf655f9d21a 100644 --- a/app/views/events/_event_push.atom.haml +++ b/app/views/events/_event_push.atom.haml @@ -1,14 +1,13 @@ %div{ xmlns: "http://www.w3.org/1999/xhtml" } - - event.commits.first(15).each do |commit| - %p - %strong= commit[:author][:name] - = link_to "(##{truncate_sha(commit[:id])})", project_commit_path(event.project, id: commit[:id]) - %i - at - = commit[:timestamp].to_time.to_s(:short) - %blockquote= markdown(escape_once(commit[:message]), pipeline: :atom, project: event.project, author: event.author) - - if event.commits_count > 15 + %p + %strong= event.author_name + = link_to "(#{truncate_sha(event.commit_id)})", project_commit_path(event.project, event.commit_id) + %i + at + = event.created_at.to_s(:short) + %blockquote= markdown(escape_once(event.commit_title), pipeline: :atom, project: event.project, author: event.author) + - if event.commits_count > 1 %p %i \... and - = pluralize(event.commits_count - 15, "more commit") + = pluralize(event.commits_count - 1, "more commit") diff --git a/app/views/events/event/_push.html.haml b/app/views/events/event/_push.html.haml index 54b414cc62a..973c652ad88 100644 --- a/app/views/events/event/_push.html.haml +++ b/app/views/events/event/_push.html.haml @@ -14,9 +14,7 @@ - if event.push_with_commits? .event-body %ul.well-list.event_commits - - few_commits = event.commits[0...2] - - few_commits.each do |commit| - = render "events/commit", commit: commit, project: project, event: event + = render "events/commit", project: project, event: event - create_mr = event.new_ref? && create_mr_button?(project.default_branch, event.ref_name, project) && event.authored_by?(current_user) - if event.commits_count > 1 @@ -44,9 +42,6 @@ = link_to create_mr_path(project.default_branch, event.ref_name, project) do Create Merge Request - elsif event.rm_ref? - - repository = project.repository - - last_commit = repository.commit(event.commit_from) - - if last_commit - .event-body - %ul.well-list.event_commits - = render "events/commit", commit: last_commit, project: project, event: event + .event-body + %ul.well-list.event_commits + = render "events/commit", project: project, event: event diff --git a/changelogs/unreleased/35435-pending-delete-project-error-in-admin-interface-fix.yml b/changelogs/unreleased/35435-pending-delete-project-error-in-admin-interface-fix.yml new file mode 100644 index 00000000000..8539615faac --- /dev/null +++ b/changelogs/unreleased/35435-pending-delete-project-error-in-admin-interface-fix.yml @@ -0,0 +1,4 @@ +--- +title: Project pending delete no longer return 500 error in admins projects view +merge_request: 13389 +author: diff --git a/changelogs/unreleased/36213-return-is_admin-in-users-api-when-current_user-is-admin.yml b/changelogs/unreleased/36213-return-is_admin-in-users-api-when-current_user-is-admin.yml new file mode 100644 index 00000000000..b51b5e58b39 --- /dev/null +++ b/changelogs/unreleased/36213-return-is_admin-in-users-api-when-current_user-is-admin.yml @@ -0,0 +1,6 @@ +--- +title: Include the `is_admin` field in the `GET /users/:id` API when current user + is an admin +merge_request: +author: +type: fixed diff --git a/changelogs/unreleased/appearances-caching-and-schema.yml b/changelogs/unreleased/appearances-caching-and-schema.yml new file mode 100644 index 00000000000..5743f6e0f2d --- /dev/null +++ b/changelogs/unreleased/appearances-caching-and-schema.yml @@ -0,0 +1,4 @@ +--- +title: Cache Appearance instances in Redis +merge_request: +author: diff --git a/changelogs/unreleased/broadcast-messages-cache.yml b/changelogs/unreleased/broadcast-messages-cache.yml new file mode 100644 index 00000000000..a3c9e1ff465 --- /dev/null +++ b/changelogs/unreleased/broadcast-messages-cache.yml @@ -0,0 +1,4 @@ +--- +title: Better caching and indexing of broadcast messages +merge_request: +author: diff --git a/changelogs/unreleased/migrate-events-into-a-new-format.yml b/changelogs/unreleased/migrate-events-into-a-new-format.yml new file mode 100644 index 00000000000..8a29f75323f --- /dev/null +++ b/changelogs/unreleased/migrate-events-into-a-new-format.yml @@ -0,0 +1,4 @@ +--- +title: Migrate events into a new format to reduce the storage necessary and improve performance +merge_request: +author: diff --git a/changelogs/unreleased/use-a-specialized-class-for-querying-events.yml b/changelogs/unreleased/use-a-specialized-class-for-querying-events.yml new file mode 100644 index 00000000000..6c1ec10aa12 --- /dev/null +++ b/changelogs/unreleased/use-a-specialized-class-for-querying-events.yml @@ -0,0 +1,4 @@ +--- +title: Use a specialized class for querying events to improve performance +merge_request: +author: diff --git a/db/migrate/20170608152747_prepare_events_table_for_push_events_migration.rb b/db/migrate/20170608152747_prepare_events_table_for_push_events_migration.rb new file mode 100644 index 00000000000..f4f03bbabaf --- /dev/null +++ b/db/migrate/20170608152747_prepare_events_table_for_push_events_migration.rb @@ -0,0 +1,51 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class PrepareEventsTableForPushEventsMigration < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + # The order of these columns is deliberate and results in the following + # columns and sizes: + # + # * id (4 bytes) + # * project_id (4 bytes) + # * author_id (4 bytes) + # * target_id (4 bytes) + # * created_at (8 bytes) + # * updated_at (8 bytes) + # * action (2 bytes) + # * target_type (variable) + # + # Unfortunately we can't make the "id" column a bigint/bigserial as Rails 4 + # does not support this properly. + create_table :events_for_migration do |t| + t.references :project, + index: true, + foreign_key: { on_delete: :cascade } + + t.integer :author_id, index: true, null: false + t.integer :target_id + + t.timestamps_with_timezone null: false + + t.integer :action, null: false, limit: 2, index: true + t.string :target_type + + t.index %i[target_type target_id] + end + + # t.references doesn't like it when the column name doesn't make the table + # name so we have to add the foreign key separately. + add_concurrent_foreign_key(:events_for_migration, :users, column: :author_id) + end + + def down + drop_table :events_for_migration + end +end diff --git a/db/migrate/20170608152748_create_push_event_payloads_tables.rb b/db/migrate/20170608152748_create_push_event_payloads_tables.rb new file mode 100644 index 00000000000..6c55ad1f2f7 --- /dev/null +++ b/db/migrate/20170608152748_create_push_event_payloads_tables.rb @@ -0,0 +1,46 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreatePushEventPayloadsTables < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + create_table :push_event_payloads, id: false do |t| + t.bigint :commit_count, null: false + + t.integer :event_id, null: false + t.integer :action, null: false, limit: 2 + t.integer :ref_type, null: false, limit: 2 + + t.binary :commit_from + t.binary :commit_to + + t.text :ref + t.string :commit_title, limit: 70 + + t.index :event_id, unique: true + end + + # We're adding a foreign key to the _shadow_ table, and this is deliberate. + # By using the shadow table we don't have to recreate/revalidate this + # foreign key after swapping the "events_for_migration" and "events" tables. + # + # The "events_for_migration" table has a foreign key to "projects.id" + # ensuring that project removals also remove events from the shadow table + # (and thus also from this table). + add_concurrent_foreign_key( + :push_event_payloads, + :events_for_migration, + column: :event_id + ) + end + + def down + drop_table :push_event_payloads + end +end diff --git a/db/migrate/20170727123534_add_index_on_events_project_id_id.rb b/db/migrate/20170727123534_add_index_on_events_project_id_id.rb new file mode 100644 index 00000000000..1c4aaaf9dd6 --- /dev/null +++ b/db/migrate/20170727123534_add_index_on_events_project_id_id.rb @@ -0,0 +1,37 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIndexOnEventsProjectIdId < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + COLUMNS = %i[project_id id].freeze + TABLES = %i[events events_for_migration].freeze + + disable_ddl_transaction! + + def up + TABLES.each do |table| + add_concurrent_index(table, COLUMNS) unless index_exists?(table, COLUMNS) + + # We remove the index _after_ adding the new one since MySQL doesn't let + # you remove an index when a foreign key exists for the same column. + if index_exists?(table, :project_id) + remove_concurrent_index(table, :project_id) + end + end + end + + def down + TABLES.each do |table| + unless index_exists?(table, :project_id) + add_concurrent_index(table, :project_id) + end + + unless index_exists?(table, COLUMNS) + remove_concurrent_index(table, COLUMNS) + end + end + end +end diff --git a/db/migrate/20170809133343_add_broadcast_messages_index.rb b/db/migrate/20170809133343_add_broadcast_messages_index.rb new file mode 100644 index 00000000000..4ab2ddb059d --- /dev/null +++ b/db/migrate/20170809133343_add_broadcast_messages_index.rb @@ -0,0 +1,21 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddBroadcastMessagesIndex < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + COLUMNS = %i[starts_at ends_at id].freeze + + def up + add_concurrent_index :broadcast_messages, COLUMNS + end + + def down + remove_concurrent_index :broadcast_messages, COLUMNS + end +end diff --git a/db/migrate/20170809134534_add_broadcast_message_not_null_constraints.rb b/db/migrate/20170809134534_add_broadcast_message_not_null_constraints.rb new file mode 100644 index 00000000000..13e8ef52f22 --- /dev/null +++ b/db/migrate/20170809134534_add_broadcast_message_not_null_constraints.rb @@ -0,0 +1,17 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddBroadcastMessageNotNullConstraints < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + COLUMNS = %i[starts_at ends_at created_at updated_at message_html] + + def change + COLUMNS.each do |column| + change_column_null :broadcast_messages, column, false + end + end +end diff --git a/db/migrate/20170809142252_cleanup_appearances_schema.rb b/db/migrate/20170809142252_cleanup_appearances_schema.rb new file mode 100644 index 00000000000..90d12925ba2 --- /dev/null +++ b/db/migrate/20170809142252_cleanup_appearances_schema.rb @@ -0,0 +1,33 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CleanupAppearancesSchema < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + NOT_NULL_COLUMNS = %i[title description description_html created_at updated_at] + + TIME_COLUMNS = %i[created_at updated_at] + + def up + NOT_NULL_COLUMNS.each do |column| + change_column_null :appearances, column, false + end + + TIME_COLUMNS.each do |column| + change_column :appearances, column, :datetime_with_timezone + end + end + + def down + NOT_NULL_COLUMNS.each do |column| + change_column_null :appearances, column, true + end + + TIME_COLUMNS.each do |column| + change_column :appearances, column, :datetime # rubocop: disable Migration/Datetime + end + end +end diff --git a/db/post_migrate/20170627101016_schedule_event_migrations.rb b/db/post_migrate/20170627101016_schedule_event_migrations.rb new file mode 100644 index 00000000000..1f34375ff0d --- /dev/null +++ b/db/post_migrate/20170627101016_schedule_event_migrations.rb @@ -0,0 +1,40 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class ScheduleEventMigrations < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + BUFFER_SIZE = 1000 + + disable_ddl_transaction! + + class Event < ActiveRecord::Base + include EachBatch + + self.table_name = 'events' + end + + def up + jobs = [] + + Event.each_batch(of: 1000) do |relation| + min, max = relation.pluck('MIN(id), MAX(id)').first + + if jobs.length == BUFFER_SIZE + # We push multiple jobs at a time to reduce the time spent in + # Sidekiq/Redis operations. We're using this buffer based approach so we + # don't need to run additional queries for every range. + BackgroundMigrationWorker.perform_bulk(jobs) + jobs.clear + end + + jobs << ['MigrateEventsToPushEventPayloads', [min, max]] + end + + BackgroundMigrationWorker.perform_bulk(jobs) unless jobs.empty? + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index ed3cf70bcdd..9d3b3ad1826 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170807160457) do +ActiveRecord::Schema.define(version: 20170809142252) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -28,13 +28,13 @@ ActiveRecord::Schema.define(version: 20170807160457) do end create_table "appearances", force: :cascade do |t| - t.string "title" - t.text "description" + t.string "title", null: false + t.text "description", null: false t.string "header_logo" t.string "logo" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.text "description_html" + t.text "description_html", null: false t.integer "cached_markdown_version" end @@ -163,16 +163,18 @@ ActiveRecord::Schema.define(version: 20170807160457) do create_table "broadcast_messages", force: :cascade do |t| t.text "message", null: false - t.datetime "starts_at" - t.datetime "ends_at" - t.datetime "created_at" - t.datetime "updated_at" + t.datetime "starts_at", null: false + t.datetime "ends_at", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false t.string "color" t.string "font" - t.text "message_html" + t.text "message_html", null: false t.integer "cached_markdown_version" end + add_index "broadcast_messages", ["starts_at", "ends_at", "id"], name: "index_broadcast_messages_on_starts_at_and_ends_at_and_id", using: :btree + create_table "chat_names", force: :cascade do |t| t.integer "user_id", null: false t.integer "service_id", null: false @@ -530,10 +532,25 @@ ActiveRecord::Schema.define(version: 20170807160457) do add_index "events", ["action"], name: "index_events_on_action", using: :btree add_index "events", ["author_id"], name: "index_events_on_author_id", using: :btree add_index "events", ["created_at"], name: "index_events_on_created_at", using: :btree - add_index "events", ["project_id"], name: "index_events_on_project_id", using: :btree + add_index "events", ["project_id", "id"], name: "index_events_on_project_id_and_id", using: :btree add_index "events", ["target_id"], name: "index_events_on_target_id", using: :btree add_index "events", ["target_type"], name: "index_events_on_target_type", using: :btree + create_table "events_for_migration", force: :cascade do |t| + t.integer "project_id" + t.integer "author_id", null: false + t.integer "target_id" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.integer "action", limit: 2, null: false + t.string "target_type" + end + + add_index "events_for_migration", ["action"], name: "index_events_for_migration_on_action", using: :btree + add_index "events_for_migration", ["author_id"], name: "index_events_for_migration_on_author_id", using: :btree + add_index "events_for_migration", ["project_id", "id"], name: "index_events_for_migration_on_project_id_and_id", using: :btree + add_index "events_for_migration", ["target_type", "target_id"], name: "index_events_for_migration_on_target_type_and_target_id", using: :btree + create_table "feature_gates", force: :cascade do |t| t.string "feature_key", null: false t.string "key", null: false @@ -1254,6 +1271,19 @@ ActiveRecord::Schema.define(version: 20170807160457) do add_index "protected_tags", ["project_id"], name: "index_protected_tags_on_project_id", using: :btree + create_table "push_event_payloads", id: false, force: :cascade do |t| + t.integer "commit_count", limit: 8, null: false + t.integer "event_id", null: false + t.integer "action", limit: 2, null: false + t.integer "ref_type", limit: 2, null: false + t.binary "commit_from" + t.binary "commit_to" + t.text "ref" + t.string "commit_title", limit: 70 + end + + add_index "push_event_payloads", ["event_id"], name: "index_push_event_payloads_on_event_id", unique: true, using: :btree + create_table "redirect_routes", force: :cascade do |t| t.integer "source_id", null: false t.string "source_type", null: false @@ -1654,6 +1684,8 @@ ActiveRecord::Schema.define(version: 20170807160457) do add_foreign_key "deployments", "projects", name: "fk_b9a3851b82", on_delete: :cascade add_foreign_key "environments", "projects", name: "fk_d1c8c1da6a", on_delete: :cascade add_foreign_key "events", "projects", name: "fk_0434b48643", on_delete: :cascade + add_foreign_key "events_for_migration", "projects", on_delete: :cascade + add_foreign_key "events_for_migration", "users", column: "author_id", name: "fk_edfd187b6f", on_delete: :cascade add_foreign_key "forked_project_links", "projects", column: "forked_to_project_id", name: "fk_434510edb0", on_delete: :cascade add_foreign_key "gpg_keys", "users", on_delete: :cascade add_foreign_key "gpg_signatures", "gpg_keys", on_delete: :nullify @@ -1696,6 +1728,7 @@ ActiveRecord::Schema.define(version: 20170807160457) do add_foreign_key "protected_tag_create_access_levels", "protected_tags" add_foreign_key "protected_tag_create_access_levels", "users" add_foreign_key "protected_tags", "projects", name: "fk_8e4af87648", on_delete: :cascade + add_foreign_key "push_event_payloads", "events_for_migration", column: "event_id", name: "fk_36c74129da", on_delete: :cascade add_foreign_key "releases", "projects", name: "fk_47fe2a0596", on_delete: :cascade add_foreign_key "services", "projects", name: "fk_71cce407f9", on_delete: :cascade add_foreign_key "snippets", "projects", name: "fk_be41fd4bb7", on_delete: :cascade diff --git a/doc/api/events.md b/doc/api/events.md index 3d5170f3f1e..129af0afa35 100644 --- a/doc/api/events.md +++ b/doc/api/events.md @@ -79,7 +79,6 @@ Example response: "target_id":160, "target_type":"Issue", "author_id":25, - "data":null, "target_title":"Qui natus eos odio tempore et quaerat consequuntur ducimus cupiditate quis.", "created_at":"2017-02-09T10:43:19.667Z", "author":{ @@ -99,7 +98,6 @@ Example response: "target_id":159, "target_type":"Issue", "author_id":21, - "data":null, "target_title":"Nostrum enim non et sed optio illo deleniti non.", "created_at":"2017-02-09T10:43:19.426Z", "author":{ @@ -151,7 +149,6 @@ Example response: "target_id": 830, "target_type": "Issue", "author_id": 1, - "data": null, "target_title": "Public project search field", "author": { "name": "Dmitriy Zaporozhets", @@ -166,7 +163,7 @@ Example response: { "title": null, "project_id": 15, - "action_name": "opened", + "action_name": "pushed", "target_id": null, "target_type": null, "author_id": 1, @@ -179,31 +176,14 @@ Example response: "web_url": "http://localhost:3000/root" }, "author_username": "john", - "data": { - "before": "50d4420237a9de7be1304607147aec22e4a14af7", - "after": "c5feabde2d8cd023215af4d2ceeb7a64839fc428", - "ref": "refs/heads/master", - "user_id": 1, - "user_name": "Dmitriy Zaporozhets", - "repository": { - "name": "gitlabhq", - "url": "git@dev.gitlab.org:gitlab/gitlabhq.git", - "description": "GitLab: self hosted Git management software. \r\nDistributed under the MIT License.", - "homepage": "https://dev.gitlab.org/gitlab/gitlabhq" - }, - "commits": [ - { - "id": "c5feabde2d8cd023215af4d2ceeb7a64839fc428", - "message": "Add simple search to projects in public area", - "timestamp": "2013-05-13T18:18:08+00:00", - "url": "https://dev.gitlab.org/gitlab/gitlabhq/commit/c5feabde2d8cd023215af4d2ceeb7a64839fc428", - "author": { - "name": "Dmitriy Zaporozhets", - "email": "dmitriy.zaporozhets@gmail.com" - } - } - ], - "total_commits_count": 1 + "push_data": { + "commit_count": 1, + "action": "pushed", + "ref_type": "branch", + "commit_from": "50d4420237a9de7be1304607147aec22e4a14af7", + "commit_to": "c5feabde2d8cd023215af4d2ceeb7a64839fc428", + "ref": "master", + "commit_title": "Add simple search to projects in public area" }, "target_title": null }, @@ -214,7 +194,6 @@ Example response: "target_id": 840, "target_type": "Issue", "author_id": 1, - "data": null, "target_title": "Finish & merge Code search PR", "author": { "name": "Dmitriy Zaporozhets", @@ -233,7 +212,6 @@ Example response: "target_id": 1312, "target_type": "Note", "author_id": 1, - "data": null, "target_title": null, "created_at": "2015-12-04T10:33:58.089Z", "note": { @@ -305,7 +283,6 @@ Example response: "target_iid":160, "target_type":"Issue", "author_id":25, - "data":null, "target_title":"Qui natus eos odio tempore et quaerat consequuntur ducimus cupiditate quis.", "created_at":"2017-02-09T10:43:19.667Z", "author":{ @@ -326,7 +303,6 @@ Example response: "target_iid":159, "target_type":"Issue", "author_id":21, - "data":null, "target_title":"Nostrum enim non et sed optio illo deleniti non.", "created_at":"2017-02-09T10:43:19.426Z", "author":{ diff --git a/doc/development/testing.md b/doc/development/testing.md index ea94c87d8c6..c7eac3cf40c 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -157,8 +157,9 @@ trade-off: - Unit tests are usually cheap, and you should consider them like the basement of your house: you need them to be confident that your code is behaving - correctly. However if you run only unit tests without integration / system tests, you might [miss] the [big] [picture]! -- Integration tests are a bit more expensive, but don't abuse them. A feature test + correctly. However if you run only unit tests without integration / system + tests, you might [miss] the [big] [picture]! +- Integration tests are a bit more expensive, but don't abuse them. A system test is often better than an integration test that is stubbing a lot of internals. - System tests are expensive (compared to unit tests), even more if they require a JavaScript driver. Make sure to follow the guidelines in the [Speed](#test-speed) @@ -195,11 +196,27 @@ Please consult the [dedicated "Frontend testing" guide](./fe_guide/testing.md). - Try to match the ordering of tests to the ordering within the class. - Try to follow the [Four-Phase Test][four-phase-test] pattern, using newlines to separate phases. -- Try to use `Gitlab.config.gitlab.host` rather than hard coding `'localhost'` +- Use `Gitlab.config.gitlab.host` rather than hard coding `'localhost'` +- Don't assert against the absolute value of a sequence-generated attribute (see + [Gotchas](gotchas.md#dont-assert-against-the-absolute-value-of-a-sequence-generated-attribute)). +- Don't supply the `:each` argument to hooks since it's the default. - On `before` and `after` hooks, prefer it scoped to `:context` over `:all` [four-phase-test]: https://robots.thoughtbot.com/four-phase-test +### Automatic retries and flaky tests detection + +On our CI, we use [rspec-retry] to automatically retry a failing example a few +times (see [`spec/spec_helper.rb`] for the precise retries count). + +We also use a home-made `RspecFlaky::Listener` listener which records flaky +examples in a JSON report file on `master` (`retrieve-tests-metadata` and `update-tests-metadata` jobs), and warns when a new flaky example +is detected in any other branch (`flaky-examples-check` job). In the future, the +`flaky-examples-check` job will not be allowed to fail. + +[rspec-retry]: https://github.com/NoRedInk/rspec-retry +[`spec/spec_helper.rb`]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/spec/spec_helper.rb + ### `let` variables GitLab's RSpec suite has made extensive use of `let` variables to reduce @@ -270,6 +287,15 @@ complexity of RSpec expectations.They should be placed under a certain type of specs only (e.g. features, requests etc.) but shouldn't be if they apply to multiple type of specs. +#### have_gitlab_http_status + +Prefer `have_gitlab_http_status` over `have_http_status` because the former +could also show the response body whenever the status mismatched. This would +be very useful whenever some tests start breaking and we would love to know +why without editing the source and rerun the tests. + +This is especially useful whenever it's showing 500 internal server error. + ### Shared contexts All shared contexts should be be placed under `spec/support/shared_contexts/`. diff --git a/features/steps/shared/project.rb b/features/steps/shared/project.rb index 00f7cded2ae..605c9a3ab71 100644 --- a/features/steps/shared/project.rb +++ b/features/steps/shared/project.rb @@ -71,28 +71,14 @@ module SharedProject step 'project "Shop" has push event' do @project = Project.find_by(name: "Shop") - - data = { - before: Gitlab::Git::BLANK_SHA, - after: "6d394385cf567f80a8fd85055db1ab4c5295806f", - ref: "refs/heads/fix", - user_id: @user.id, - user_name: @user.name, - repository: { - name: @project.name, - url: "localhost/rubinius", - description: "", - homepage: "localhost/rubinius", - private: true - } - } - - @event = Event.create( - project: @project, - action: Event::PUSHED, - data: data, - author_id: @user.id - ) + @event = create(:push_event, project: @project, author: @user) + + create(:push_event_payload, + event: @event, + action: :created, + commit_to: '6d394385cf567f80a8fd85055db1ab4c5295806f', + ref: 'fix', + commit_count: 1) end step 'I should see project "Shop" activity feed' do diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 3bb1910a441..18cd604a216 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -497,14 +497,24 @@ module API expose :author, using: Entities::UserBasic end + class PushEventPayload < Grape::Entity + expose :commit_count, :action, :ref_type, :commit_from, :commit_to + expose :ref, :commit_title + end + class Event < Grape::Entity expose :title, :project_id, :action_name expose :target_id, :target_iid, :target_type, :author_id - expose :data, :target_title + expose :target_title expose :created_at expose :note, using: Entities::Note, if: ->(event, options) { event.note? } expose :author, using: Entities::UserBasic, if: ->(event, options) { event.author } + expose :push_event_payload, + as: :push_data, + using: PushEventPayload, + if: -> (event, _) { event.push? } + expose :author_username do |event, options| event.author&.username end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 99b8b62691f..3582ed81b0f 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -257,7 +257,15 @@ module API message << " " << trace.join("\n ") API.logger.add Logger::FATAL, message - rack_response({ 'message' => '500 Internal Server Error' }.to_json, 500) + + response_message = + if Rails.env.test? + message + else + '500 Internal Server Error' + end + + rack_response({ 'message' => response_message }.to_json, 500) end # project helpers diff --git a/lib/api/users.rb b/lib/api/users.rb index a590f2692a2..e2019d6d512 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -79,22 +79,17 @@ module API end desc 'Get a single user' do - success Entities::UserBasic + success Entities::User end params do requires :id, type: Integer, desc: 'The ID of the user' end get ":id" do user = User.find_by(id: params[:id]) - not_found!('User') unless user + not_found!('User') unless user && can?(current_user, :read_user, user) - if current_user && current_user.admin? - present user, with: Entities::UserPublic - elsif can?(current_user, :read_user, user) - present user, with: Entities::User - else - render_api_error!("User not found.", 404) - end + opts = current_user&.admin? ? { with: Entities::UserWithAdmin } : {} + present user, opts end desc 'Create a user. Available only for admins.' do diff --git a/lib/api/v3/entities.rb b/lib/api/v3/entities.rb index 4a2e9c9cbb0..a9a35f2a4bd 100644 --- a/lib/api/v3/entities.rb +++ b/lib/api/v3/entities.rb @@ -25,14 +25,24 @@ module API expose(:downvote?) { |note| false } end + class PushEventPayload < Grape::Entity + expose :commit_count, :action, :ref_type, :commit_from, :commit_to + expose :ref, :commit_title + end + class Event < Grape::Entity expose :title, :project_id, :action_name expose :target_id, :target_type, :author_id - expose :data, :target_title + expose :target_title expose :created_at expose :note, using: Entities::Note, if: ->(event, options) { event.note? } expose :author, using: ::API::Entities::UserBasic, if: ->(event, options) { event.author } + expose :push_event_payload, + as: :push_data, + using: PushEventPayload, + if: -> (event, _) { event.push? } + expose :author_username do |event, options| event.author&.username end diff --git a/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb b/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb new file mode 100644 index 00000000000..432f7c3e706 --- /dev/null +++ b/lib/gitlab/background_migration/migrate_events_to_push_event_payloads.rb @@ -0,0 +1,176 @@ +module Gitlab + module BackgroundMigration + # Class that migrates events for the new push event payloads setup. All + # events are copied to a shadow table, and push events will also have a row + # created in the push_event_payloads table. + class MigrateEventsToPushEventPayloads + class Event < ActiveRecord::Base + self.table_name = 'events' + + serialize :data + + BLANK_REF = ('0' * 40).freeze + TAG_REF_PREFIX = 'refs/tags/'.freeze + MAX_INDEX = 69 + PUSHED = 5 + + def push_event? + action == PUSHED && data.present? + end + + def commit_title + commit = commits.last + + return nil unless commit && commit[:message] + + index = commit[:message].index("\n") + message = index ? commit[:message][0..index] : commit[:message] + + message.strip.truncate(70) + end + + def commit_from_sha + if create? + nil + else + data[:before] + end + end + + def commit_to_sha + if remove? + nil + else + data[:after] + end + end + + def data + super || {} + end + + def commits + data[:commits] || [] + end + + def commit_count + data[:total_commits_count] || 0 + end + + def ref + data[:ref] + end + + def trimmed_ref_name + if ref_type == :tag + ref[10..-1] + else + ref[11..-1] + end + end + + def create? + data[:before] == BLANK_REF + end + + def remove? + data[:after] == BLANK_REF + end + + def push_action + if create? + :created + elsif remove? + :removed + else + :pushed + end + end + + def ref_type + if ref.start_with?(TAG_REF_PREFIX) + :tag + else + :branch + end + end + end + + class EventForMigration < ActiveRecord::Base + self.table_name = 'events_for_migration' + end + + class PushEventPayload < ActiveRecord::Base + self.table_name = 'push_event_payloads' + + enum action: { + created: 0, + removed: 1, + pushed: 2 + } + + enum ref_type: { + branch: 0, + tag: 1 + } + end + + # start_id - The start ID of the range of events to process + # end_id - The end ID of the range to process. + def perform(start_id, end_id) + return unless migrate? + + find_events(start_id, end_id).each { |event| process_event(event) } + end + + def process_event(event) + replicate_event(event) + create_push_event_payload(event) if event.push_event? + end + + def replicate_event(event) + new_attributes = event.attributes + .with_indifferent_access.except(:title, :data) + + EventForMigration.create!(new_attributes) + rescue ActiveRecord::InvalidForeignKey + # A foreign key error means the associated event was removed. In this + # case we'll just skip migrating the event. + end + + def create_push_event_payload(event) + commit_from = pack(event.commit_from_sha) + commit_to = pack(event.commit_to_sha) + + PushEventPayload.create!( + event_id: event.id, + commit_count: event.commit_count, + ref_type: event.ref_type, + action: event.push_action, + commit_from: commit_from, + commit_to: commit_to, + ref: event.trimmed_ref_name, + commit_title: event.commit_title + ) + rescue ActiveRecord::InvalidForeignKey + # A foreign key error means the associated event was removed. In this + # case we'll just skip migrating the event. + end + + def find_events(start_id, end_id) + Event + .where('NOT EXISTS (SELECT true FROM events_for_migration WHERE events_for_migration.id = events.id)') + .where(id: start_id..end_id) + end + + def migrate? + Event.table_exists? && PushEventPayload.table_exists? && + EventForMigration.table_exists? + end + + def pack(value) + value ? [value].pack('H*') : nil + end + end + end +end diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index d7dab584a44..e001d25e7b7 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -25,6 +25,10 @@ module Gitlab database_version.match(/\A(?:PostgreSQL |)([^\s]+).*\z/)[1] end + def self.join_lateral_supported? + postgresql? && version.to_f >= 9.3 + end + def self.nulls_last_order(field, direction = 'ASC') order = "#{field} #{direction}" diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index c5c05bfe2fb..9d9ebcb389a 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -3,18 +3,22 @@ project_tree: - labels: :priorities - milestones: - - :events + - events: + - :push_event_payload - issues: - - :events + - events: + - :push_event_payload - :timelogs - notes: - :author - - :events + - events: + - :push_event_payload - label_links: - label: :priorities - milestone: - - :events + - events: + - :push_event_payload - snippets: - :award_emoji - notes: @@ -25,21 +29,25 @@ project_tree: - merge_requests: - notes: - :author - - :events + - events: + - :push_event_payload - merge_request_diff: - :merge_request_diff_commits - :merge_request_diff_files - - :events + - events: + - :push_event_payload - :timelogs - label_links: - label: :priorities - milestone: - - :events + - events: + - :push_event_payload - pipelines: - notes: - :author - - :events + - events: + - :push_event_payload - :stages - :statuses - :triggers @@ -107,6 +115,8 @@ excluded_attributes: statuses: - :trace - :token + push_event_payload: + - :event_id methods: labels: diff --git a/lib/rspec_flaky/example.rb b/lib/rspec_flaky/example.rb new file mode 100644 index 00000000000..b6e790cbbab --- /dev/null +++ b/lib/rspec_flaky/example.rb @@ -0,0 +1,46 @@ +module RspecFlaky + # This is a wrapper class for RSpec::Core::Example + class Example + delegate :status, :exception, to: :execution_result + + def initialize(rspec_example) + @rspec_example = rspec_example.try(:example) || rspec_example + end + + def uid + @uid ||= Digest::MD5.hexdigest("#{description}-#{file}") + end + + def example_id + rspec_example.id + end + + def file + metadata[:file_path] + end + + def line + metadata[:line_number] + end + + def description + metadata[:full_description] + end + + def attempts + rspec_example.try(:attempts) || 1 + end + + private + + attr_reader :rspec_example + + def metadata + rspec_example.metadata + end + + def execution_result + rspec_example.execution_result + end + end +end diff --git a/lib/rspec_flaky/flaky_example.rb b/lib/rspec_flaky/flaky_example.rb new file mode 100644 index 00000000000..f81fb90e870 --- /dev/null +++ b/lib/rspec_flaky/flaky_example.rb @@ -0,0 +1,39 @@ +module RspecFlaky + # This represents a flaky RSpec example and is mainly meant to be saved in a JSON file + class FlakyExample < OpenStruct + def initialize(example) + if example.respond_to?(:example_id) + super( + example_id: example.example_id, + file: example.file, + line: example.line, + description: example.description, + last_attempts_count: example.attempts, + flaky_reports: 1) + else + super + end + end + + def first_flaky_at + self[:first_flaky_at] || Time.now + end + + def last_flaky_at + Time.now + end + + def last_flaky_job + return unless ENV['CI_PROJECT_URL'] && ENV['CI_JOB_ID'] + + "#{ENV['CI_PROJECT_URL']}/-/jobs/#{ENV['CI_JOB_ID']}" + end + + def to_h + super.merge( + first_flaky_at: first_flaky_at, + last_flaky_at: last_flaky_at, + last_flaky_job: last_flaky_job) + end + end +end diff --git a/lib/rspec_flaky/listener.rb b/lib/rspec_flaky/listener.rb new file mode 100644 index 00000000000..ec2fbd9e36c --- /dev/null +++ b/lib/rspec_flaky/listener.rb @@ -0,0 +1,75 @@ +require 'json' + +module RspecFlaky + class Listener + attr_reader :all_flaky_examples, :new_flaky_examples + + def initialize + @new_flaky_examples = {} + @all_flaky_examples = init_all_flaky_examples + end + + def example_passed(notification) + current_example = RspecFlaky::Example.new(notification.example) + + return unless current_example.attempts > 1 + + flaky_example_hash = all_flaky_examples[current_example.uid] + + all_flaky_examples[current_example.uid] = + if flaky_example_hash + FlakyExample.new(flaky_example_hash).tap do |ex| + ex.last_attempts_count = current_example.attempts + ex.flaky_reports += 1 + end + else + FlakyExample.new(current_example).tap do |ex| + new_flaky_examples[current_example.uid] = ex + end + end + end + + def dump_summary(_) + write_report_file(all_flaky_examples, all_flaky_examples_report_path) + + if new_flaky_examples.any? + Rails.logger.warn "\nNew flaky examples detected:\n" + Rails.logger.warn JSON.pretty_generate(to_report(new_flaky_examples)) + + write_report_file(new_flaky_examples, new_flaky_examples_report_path) + end + end + + def to_report(examples) + Hash[examples.map { |k, ex| [k, ex.to_h] }] + end + + private + + def init_all_flaky_examples + return {} unless File.exist?(all_flaky_examples_report_path) + + all_flaky_examples = JSON.parse(File.read(all_flaky_examples_report_path)) + + Hash[(all_flaky_examples || {}).map { |k, ex| [k, FlakyExample.new(ex)] }] + end + + def write_report_file(examples, file_path) + return unless ENV['FLAKY_RSPEC_GENERATE_REPORT'] == 'true' + + report_path_dir = File.dirname(file_path) + FileUtils.mkdir_p(report_path_dir) unless Dir.exist?(report_path_dir) + File.write(file_path, JSON.pretty_generate(to_report(examples))) + end + + def all_flaky_examples_report_path + @all_flaky_examples_report_path ||= ENV['ALL_FLAKY_RSPEC_REPORT_PATH'] || + Rails.root.join("rspec_flaky/all-report.json") + end + + def new_flaky_examples_report_path + @new_flaky_examples_report_path ||= ENV['NEW_FLAKY_RSPEC_REPORT_PATH'] || + Rails.root.join("rspec_flaky/new-report.json") + end + end +end diff --git a/scripts/detect-new-flaky-examples b/scripts/detect-new-flaky-examples new file mode 100755 index 00000000000..3bee4f9a34b --- /dev/null +++ b/scripts/detect-new-flaky-examples @@ -0,0 +1,21 @@ +#!/usr/bin/env ruby + +require 'json' + +report_file = ARGV.shift +unless report_file + puts 'usage: detect-new-flaky-examples <report-file>' + exit 1 +end + +puts "Loading #{report_file}..." +report = JSON.parse(File.read(report_file)) + +if report.any? + puts "New flaky examples were detected!\n" + puts JSON.pretty_generate(report) + exit 1 +else + puts "No new flaky examples detected.\n" + exit 0 +end diff --git a/scripts/merge-reports b/scripts/merge-reports index aad76bcc327..3a421f1f1fc 100755 --- a/scripts/merge-reports +++ b/scripts/merge-reports @@ -4,7 +4,7 @@ require 'json' main_report_file = ARGV.shift unless main_report_file - puts 'usage: merge_reports <main-report> [extra reports...]' + puts 'usage: merge-reports <main-report> [extra reports...]' exit 1 end diff --git a/spec/controllers/admin/projects_controller_spec.rb b/spec/controllers/admin/projects_controller_spec.rb index 65587064eb1..373260b3978 100644 --- a/spec/controllers/admin/projects_controller_spec.rb +++ b/spec/controllers/admin/projects_controller_spec.rb @@ -12,12 +12,24 @@ describe Admin::ProjectsController do it 'retrieves the project for the given visibility level' do get :index, visibility_level: [Gitlab::VisibilityLevel::PUBLIC] + expect(response.body).to match(project.name) end it 'does not retrieve the project' do get :index, visibility_level: [Gitlab::VisibilityLevel::INTERNAL] + expect(response.body).not_to match(project.name) end + + it 'does not respond with projects pending deletion' do + pending_delete_project = create(:project, pending_delete: true) + + get :index + + expect(response).to have_http_status(200) + expect(response.body).not_to match(pending_delete_project.name) + expect(response.body).to match(project.name) + end end end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index a64ad73cba8..2cecd2646fc 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -92,8 +92,14 @@ describe UsersController do before do sign_in(user) project.team << [user, :developer] - EventCreateService.new.push(project, user, []) - EventCreateService.new.push(forked_project, user, []) + + push_data = Gitlab::DataBuilder::Push.build_sample(project, user) + + fork_push_data = Gitlab::DataBuilder::Push + .build_sample(forked_project, user) + + EventCreateService.new.push(project, user, push_data) + EventCreateService.new.push(forked_project, user, fork_push_data) end it 'includes forked projects' do diff --git a/spec/factories/events.rb b/spec/factories/events.rb index 11d2016955c..ad9f7e2caef 100644 --- a/spec/factories/events.rb +++ b/spec/factories/events.rb @@ -2,6 +2,7 @@ FactoryGirl.define do factory :event do project author factory: :user + action Event::JOINED trait(:created) { action Event::CREATED } trait(:updated) { action Event::UPDATED } @@ -20,4 +21,19 @@ FactoryGirl.define do target factory: :closed_issue end end + + factory :push_event, class: PushEvent do + project factory: :project_empty_repo + author factory: :user + action Event::PUSHED + end + + factory :push_event_payload do + event + commit_count 1 + action :pushed + ref_type :branch + ref 'master' + commit_to '3cdce97ed87c91368561584e7358f4d46e3e173c' + end end diff --git a/spec/features/calendar_spec.rb b/spec/features/calendar_spec.rb index 64fbc80cb81..9a597a2d690 100644 --- a/spec/features/calendar_spec.rb +++ b/spec/features/calendar_spec.rb @@ -42,14 +42,14 @@ feature 'Contributions Calendar', :js do end def push_code_contribution - push_params = { - project: contributed_project, - action: Event::PUSHED, - author_id: user.id, - data: { commit_count: 3 } - } - - Event.create(push_params) + event = create(:push_event, project: contributed_project, author: user) + + create(:push_event_payload, + event: event, + commit_from: '11f9ac0a48b62cef25eedede4c1819964f08d5ce', + commit_to: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', + commit_count: 3, + ref: 'master') end def note_comment_contribution diff --git a/spec/features/dashboard/activity_spec.rb b/spec/features/dashboard/activity_spec.rb index 4917dfcf1d1..582868bac1e 100644 --- a/spec/features/dashboard/activity_spec.rb +++ b/spec/features/dashboard/activity_spec.rb @@ -23,27 +23,19 @@ feature 'Dashboard > Activity' do create(:merge_request, author: user, source_project: project, target_project: project) end - let(:push_event_data) do - { - before: Gitlab::Git::BLANK_SHA, - after: '0220c11b9a3e6c69dc8fd35321254ca9a7b98f7e', - ref: 'refs/heads/new_design', - user_id: user.id, - user_name: user.name, - repository: { - name: project.name, - url: 'localhost/rubinius', - description: '', - homepage: 'localhost/rubinius', - private: true - } - } - end - let(:note) { create(:note, project: project, noteable: merge_request) } let!(:push_event) do - create(:event, :pushed, data: push_event_data, project: project, author: user) + event = create(:push_event, project: project, author: user) + + create(:push_event_payload, + event: event, + action: :created, + commit_to: '0220c11b9a3e6c69dc8fd35321254ca9a7b98f7e', + ref: 'new_design', + commit_count: 1) + + event end let!(:merged_event) do diff --git a/spec/finders/admin/projects_finder_spec.rb b/spec/finders/admin/projects_finder_spec.rb index 4e367d39cf3..28e36330029 100644 --- a/spec/finders/admin/projects_finder_spec.rb +++ b/spec/finders/admin/projects_finder_spec.rb @@ -38,6 +38,12 @@ describe Admin::ProjectsFinder do it { is_expected.to match_array([shared_project, public_project, internal_project, private_project]) } end + context 'with pending delete project' do + let!(:pending_delete_project) { create(:project, pending_delete: true) } + + it { is_expected.not_to include(pending_delete_project) } + end + context 'filter by namespace_id' do let(:namespace) { create(:namespace) } let!(:project_in_namespace) { create(:project, namespace: namespace) } diff --git a/spec/finders/contributed_projects_finder_spec.rb b/spec/finders/contributed_projects_finder_spec.rb index 2d079ea83b4..60ea98e61c7 100644 --- a/spec/finders/contributed_projects_finder_spec.rb +++ b/spec/finders/contributed_projects_finder_spec.rb @@ -14,8 +14,8 @@ describe ContributedProjectsFinder do private_project.add_developer(current_user) public_project.add_master(source_user) - create(:event, :pushed, project: public_project, target: public_project, author: source_user) - create(:event, :pushed, project: private_project, target: private_project, author: source_user) + create(:push_event, project: public_project, author: source_user) + create(:push_event, project: private_project, author: source_user) end describe 'without a current user' do diff --git a/spec/lib/event_filter_spec.rb b/spec/lib/event_filter_spec.rb index b0efcab47fb..87ae6b6cf01 100644 --- a/spec/lib/event_filter_spec.rb +++ b/spec/lib/event_filter_spec.rb @@ -5,7 +5,7 @@ describe EventFilter do let(:source_user) { create(:user) } let!(:public_project) { create(:project, :public) } - let!(:push_event) { create(:event, :pushed, project: public_project, target: public_project, author: source_user) } + let!(:push_event) { create(:push_event, project: public_project, author: source_user) } let!(:merged_event) { create(:event, :merged, project: public_project, target: public_project, author: source_user) } let!(:created_event) { create(:event, :created, project: public_project, target: public_project, author: source_user) } let!(:updated_event) { create(:event, :updated, project: public_project, target: public_project, author: source_user) } diff --git a/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb b/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb new file mode 100644 index 00000000000..87f45619e7a --- /dev/null +++ b/spec/lib/gitlab/background_migration/migrate_events_to_push_event_payloads_spec.rb @@ -0,0 +1,423 @@ +require 'spec_helper' + +describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads::Event do + describe '#commit_title' do + it 'returns nil when there are no commits' do + expect(described_class.new.commit_title).to be_nil + end + + it 'returns nil when there are commits without commit messages' do + event = described_class.new + + allow(event).to receive(:commits).and_return([{ id: '123' }]) + + expect(event.commit_title).to be_nil + end + + it 'returns the commit message when it is less than 70 characters long' do + event = described_class.new + + allow(event).to receive(:commits).and_return([{ message: 'Hello world' }]) + + expect(event.commit_title).to eq('Hello world') + end + + it 'returns the first line of a commit message if multiple lines are present' do + event = described_class.new + + allow(event).to receive(:commits).and_return([{ message: "Hello\n\nworld" }]) + + expect(event.commit_title).to eq('Hello') + end + + it 'truncates the commit to 70 characters when it is too long' do + event = described_class.new + + allow(event).to receive(:commits).and_return([{ message: 'a' * 100 }]) + + expect(event.commit_title).to eq(('a' * 67) + '...') + end + end + + describe '#commit_from_sha' do + it 'returns nil when pushing to a new ref' do + event = described_class.new + + allow(event).to receive(:create?).and_return(true) + + expect(event.commit_from_sha).to be_nil + end + + it 'returns the ID of the first commit when pushing to an existing ref' do + event = described_class.new + + allow(event).to receive(:create?).and_return(false) + allow(event).to receive(:data).and_return(before: '123') + + expect(event.commit_from_sha).to eq('123') + end + end + + describe '#commit_to_sha' do + it 'returns nil when removing an existing ref' do + event = described_class.new + + allow(event).to receive(:remove?).and_return(true) + + expect(event.commit_to_sha).to be_nil + end + + it 'returns the ID of the last commit when pushing to an existing ref' do + event = described_class.new + + allow(event).to receive(:remove?).and_return(false) + allow(event).to receive(:data).and_return(after: '123') + + expect(event.commit_to_sha).to eq('123') + end + end + + describe '#data' do + it 'returns the deserialized data' do + event = described_class.new(data: { before: '123' }) + + expect(event.data).to eq(before: '123') + end + + it 'returns an empty hash when no data is present' do + event = described_class.new + + expect(event.data).to eq({}) + end + end + + describe '#commits' do + it 'returns an Array of commits' do + event = described_class.new(data: { commits: [{ id: '123' }] }) + + expect(event.commits).to eq([{ id: '123' }]) + end + + it 'returns an empty array when no data is present' do + event = described_class.new + + expect(event.commits).to eq([]) + end + end + + describe '#commit_count' do + it 'returns the number of commits' do + event = described_class.new(data: { total_commits_count: 2 }) + + expect(event.commit_count).to eq(2) + end + + it 'returns 0 when no data is present' do + event = described_class.new + + expect(event.commit_count).to eq(0) + end + end + + describe '#ref' do + it 'returns the name of the ref' do + event = described_class.new(data: { ref: 'refs/heads/master' }) + + expect(event.ref).to eq('refs/heads/master') + end + end + + describe '#trimmed_ref_name' do + it 'returns the trimmed ref name for a branch' do + event = described_class.new(data: { ref: 'refs/heads/master' }) + + expect(event.trimmed_ref_name).to eq('master') + end + + it 'returns the trimmed ref name for a tag' do + event = described_class.new(data: { ref: 'refs/tags/v1.2' }) + + expect(event.trimmed_ref_name).to eq('v1.2') + end + end + + describe '#create?' do + it 'returns true when creating a new ref' do + event = described_class.new(data: { before: described_class::BLANK_REF }) + + expect(event.create?).to eq(true) + end + + it 'returns false when pushing to an existing ref' do + event = described_class.new(data: { before: '123' }) + + expect(event.create?).to eq(false) + end + end + + describe '#remove?' do + it 'returns true when removing an existing ref' do + event = described_class.new(data: { after: described_class::BLANK_REF }) + + expect(event.remove?).to eq(true) + end + + it 'returns false when pushing to an existing ref' do + event = described_class.new(data: { after: '123' }) + + expect(event.remove?).to eq(false) + end + end + + describe '#push_action' do + let(:event) { described_class.new } + + it 'returns :created when creating a new ref' do + allow(event).to receive(:create?).and_return(true) + + expect(event.push_action).to eq(:created) + end + + it 'returns :removed when removing an existing ref' do + allow(event).to receive(:create?).and_return(false) + allow(event).to receive(:remove?).and_return(true) + + expect(event.push_action).to eq(:removed) + end + + it 'returns :pushed when pushing to an existing ref' do + allow(event).to receive(:create?).and_return(false) + allow(event).to receive(:remove?).and_return(false) + + expect(event.push_action).to eq(:pushed) + end + end + + describe '#ref_type' do + let(:event) { described_class.new } + + it 'returns :tag for a tag' do + allow(event).to receive(:ref).and_return('refs/tags/1.2') + + expect(event.ref_type).to eq(:tag) + end + + it 'returns :branch for a branch' do + allow(event).to receive(:ref).and_return('refs/heads/1.2') + + expect(event.ref_type).to eq(:branch) + end + end +end + +describe Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads do + let(:migration) { described_class.new } + let(:project) { create(:project_empty_repo) } + let(:author) { create(:user) } + + # We can not rely on FactoryGirl as the state of Event may change in ways that + # the background migration does not expect, hence we use the Event class of + # the migration itself. + def create_push_event(project, author, data = nil) + klass = Gitlab::BackgroundMigration::MigrateEventsToPushEventPayloads::Event + + klass.create!( + action: klass::PUSHED, + project_id: project.id, + author_id: author.id, + data: data + ) + end + + # The background migration relies on a temporary table, hence we're migrating + # to a specific version of the database where said table is still present. + before :all do + ActiveRecord::Migration.verbose = false + + ActiveRecord::Migrator + .migrate(ActiveRecord::Migrator.migrations_paths, 20170608152748) + end + + after :all do + ActiveRecord::Migrator.migrate(ActiveRecord::Migrator.migrations_paths) + + ActiveRecord::Migration.verbose = true + end + + describe '#perform' do + it 'returns if data should not be migrated' do + allow(migration).to receive(:migrate?).and_return(false) + + expect(migration).not_to receive(:find_events) + + migration.perform(1, 10) + end + + it 'migrates the range of events if data is to be migrated' do + event1 = create_push_event(project, author, { commits: [] }) + event2 = create_push_event(project, author, { commits: [] }) + + allow(migration).to receive(:migrate?).and_return(true) + + expect(migration).to receive(:process_event).twice + + migration.perform(event1.id, event2.id) + end + end + + describe '#process_event' do + it 'processes a regular event' do + event = double(:event, push_event?: false) + + expect(migration).to receive(:replicate_event) + expect(migration).not_to receive(:create_push_event_payload) + + migration.process_event(event) + end + + it 'processes a push event' do + event = double(:event, push_event?: true) + + expect(migration).to receive(:replicate_event) + expect(migration).to receive(:create_push_event_payload) + + migration.process_event(event) + end + end + + describe '#replicate_event' do + it 'replicates the event to the "events_for_migration" table' do + event = create_push_event( + project, + author, + data: { commits: [] }, + title: 'bla' + ) + + attributes = event + .attributes.with_indifferent_access.except(:title, :data) + + expect(described_class::EventForMigration) + .to receive(:create!) + .with(attributes) + + migration.replicate_event(event) + end + end + + describe '#create_push_event_payload' do + let(:push_data) do + { + commits: [], + ref: 'refs/heads/master', + before: '156e0e9adc587a383a7eeb5b21ddecb9044768a8', + after: '0' * 40, + total_commits_count: 1 + } + end + + let(:event) do + create_push_event(project, author, push_data) + end + + before do + # The foreign key in push_event_payloads at this point points to the + # "events_for_migration" table so we need to make sure a row exists in + # said table. + migration.replicate_event(event) + end + + it 'creates a push event payload for an event' do + payload = migration.create_push_event_payload(event) + + expect(PushEventPayload.count).to eq(1) + expect(payload.valid?).to eq(true) + end + + it 'does not create push event payloads for removed events' do + allow(event).to receive(:id).and_return(-1) + + payload = migration.create_push_event_payload(event) + + expect(payload).to be_nil + expect(PushEventPayload.count).to eq(0) + end + + it 'encodes and decodes the commit IDs from and to binary data' do + payload = migration.create_push_event_payload(event) + packed = migration.pack(push_data[:before]) + + expect(payload.commit_from).to eq(packed) + expect(payload.commit_to).to be_nil + end + end + + describe '#find_events' do + it 'returns the events for the given ID range' do + event1 = create_push_event(project, author, { commits: [] }) + event2 = create_push_event(project, author, { commits: [] }) + event3 = create_push_event(project, author, { commits: [] }) + events = migration.find_events(event1.id, event2.id) + + expect(events.length).to eq(2) + expect(events.pluck(:id)).not_to include(event3.id) + end + end + + describe '#migrate?' do + it 'returns true when data should be migrated' do + allow(described_class::Event) + .to receive(:table_exists?).and_return(true) + + allow(described_class::PushEventPayload) + .to receive(:table_exists?).and_return(true) + + allow(described_class::EventForMigration) + .to receive(:table_exists?).and_return(true) + + expect(migration.migrate?).to eq(true) + end + + it 'returns false if the "events" table does not exist' do + allow(described_class::Event) + .to receive(:table_exists?).and_return(false) + + expect(migration.migrate?).to eq(false) + end + + it 'returns false if the "push_event_payloads" table does not exist' do + allow(described_class::Event) + .to receive(:table_exists?).and_return(true) + + allow(described_class::PushEventPayload) + .to receive(:table_exists?).and_return(false) + + expect(migration.migrate?).to eq(false) + end + + it 'returns false when the "events_for_migration" table does not exist' do + allow(described_class::Event) + .to receive(:table_exists?).and_return(true) + + allow(described_class::PushEventPayload) + .to receive(:table_exists?).and_return(true) + + allow(described_class::EventForMigration) + .to receive(:table_exists?).and_return(false) + + expect(migration.migrate?).to eq(false) + end + end + + describe '#pack' do + it 'packs a SHA1 into a 20 byte binary string' do + packed = migration.pack('156e0e9adc587a383a7eeb5b21ddecb9044768a8') + + expect(packed.bytesize).to eq(20) + end + + it 'returns nil if the input value is nil' do + expect(migration.pack(nil)).to be_nil + end + end +end diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index c5f9aecd867..5fa94999d25 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -51,6 +51,28 @@ describe Gitlab::Database do end end + describe '.join_lateral_supported?' do + it 'returns false when using MySQL' do + allow(described_class).to receive(:postgresql?).and_return(false) + + expect(described_class.join_lateral_supported?).to eq(false) + end + + it 'returns false when using PostgreSQL 9.2' do + allow(described_class).to receive(:postgresql?).and_return(true) + allow(described_class).to receive(:version).and_return('9.2.1') + + expect(described_class.join_lateral_supported?).to eq(false) + end + + it 'returns true when using PostgreSQL 9.3.0 or newer' do + allow(described_class).to receive(:postgresql?).and_return(true) + allow(described_class).to receive(:version).and_return('9.3.0') + + expect(described_class.join_lateral_supported?).to eq(true) + end + end + describe '.nulls_last_order' do context 'when using PostgreSQL' do before do diff --git a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb index a0e5e401359..f5c9680bf59 100644 --- a/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb +++ b/spec/lib/gitlab/health_checks/fs_shards_check_spec.rb @@ -106,12 +106,6 @@ describe Gitlab::HealthChecks::FsShardsCheck do }.with_indifferent_access end - # Unsolved intermittent failure in CI https://gitlab.com/gitlab-org/gitlab-ce/issues/31128 - around do |example| # rubocop:disable RSpec/AroundBlock - times_to_try = ENV['CI'] ? 4 : 1 - example.run_with_retry retry: times_to_try - end - it 'provides metrics' do metrics = described_class.metrics diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 6a41afe0c25..8da02b0cf00 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -22,6 +22,7 @@ events: - author - project - target +- push_event_payload notes: - award_emoji - project @@ -272,3 +273,5 @@ timelogs: - issue - merge_request - user +push_event_payload: +- event diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 4dce48f8079..ae3b0173160 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -36,6 +36,14 @@ Event: - updated_at - action - author_id +PushEventPayload: +- commit_count +- action +- ref_type +- commit_from +- commit_to +- ref +- commit_title Note: - id - note diff --git a/spec/lib/rspec_flaky/example_spec.rb b/spec/lib/rspec_flaky/example_spec.rb new file mode 100644 index 00000000000..5b4fd5ddf3e --- /dev/null +++ b/spec/lib/rspec_flaky/example_spec.rb @@ -0,0 +1,89 @@ +require 'spec_helper' + +describe RspecFlaky::Example do + let(:example_attrs) do + { + id: 'spec/foo/bar_spec.rb:2', + metadata: { + file_path: 'spec/foo/bar_spec.rb', + line_number: 2, + full_description: 'hello world' + }, + execution_result: double(status: 'passed', exception: 'BOOM!'), + attempts: 1 + } + end + let(:rspec_example) { double(example_attrs) } + + describe '#initialize' do + shared_examples 'a valid Example instance' do + it 'returns valid attributes' do + example = described_class.new(args) + + expect(example.example_id).to eq(example_attrs[:id]) + end + end + + context 'when given an Rspec::Core::Example that responds to #example' do + let(:args) { double(example: rspec_example) } + + it_behaves_like 'a valid Example instance' + end + + context 'when given an Rspec::Core::Example that does not respond to #example' do + let(:args) { rspec_example } + + it_behaves_like 'a valid Example instance' + end + end + + subject { described_class.new(rspec_example) } + + describe '#uid' do + it 'returns a hash of the full description' do + expect(subject.uid).to eq(Digest::MD5.hexdigest("#{subject.description}-#{subject.file}")) + end + end + + describe '#example_id' do + it 'returns the ID of the RSpec::Core::Example' do + expect(subject.example_id).to eq(rspec_example.id) + end + end + + describe '#attempts' do + it 'returns the attempts of the RSpec::Core::Example' do + expect(subject.attempts).to eq(rspec_example.attempts) + end + end + + describe '#file' do + it 'returns the metadata[:file_path] of the RSpec::Core::Example' do + expect(subject.file).to eq(rspec_example.metadata[:file_path]) + end + end + + describe '#line' do + it 'returns the metadata[:line_number] of the RSpec::Core::Example' do + expect(subject.line).to eq(rspec_example.metadata[:line_number]) + end + end + + describe '#description' do + it 'returns the metadata[:full_description] of the RSpec::Core::Example' do + expect(subject.description).to eq(rspec_example.metadata[:full_description]) + end + end + + describe '#status' do + it 'returns the execution_result.status of the RSpec::Core::Example' do + expect(subject.status).to eq(rspec_example.execution_result.status) + end + end + + describe '#exception' do + it 'returns the execution_result.exception of the RSpec::Core::Example' do + expect(subject.exception).to eq(rspec_example.execution_result.exception) + end + end +end diff --git a/spec/lib/rspec_flaky/flaky_example_spec.rb b/spec/lib/rspec_flaky/flaky_example_spec.rb new file mode 100644 index 00000000000..cbfc1e538ab --- /dev/null +++ b/spec/lib/rspec_flaky/flaky_example_spec.rb @@ -0,0 +1,104 @@ +require 'spec_helper' + +describe RspecFlaky::FlakyExample do + let(:flaky_example_attrs) do + { + example_id: 'spec/foo/bar_spec.rb:2', + file: 'spec/foo/bar_spec.rb', + line: 2, + description: 'hello world', + first_flaky_at: 1234, + last_flaky_at: 2345, + last_attempts_count: 2, + flaky_reports: 1 + } + end + let(:example_attrs) do + { + uid: 'abc123', + example_id: flaky_example_attrs[:example_id], + file: flaky_example_attrs[:file], + line: flaky_example_attrs[:line], + description: flaky_example_attrs[:description], + status: 'passed', + exception: 'BOOM!', + attempts: flaky_example_attrs[:last_attempts_count] + } + end + let(:example) { double(example_attrs) } + + describe '#initialize' do + shared_examples 'a valid FlakyExample instance' do + it 'returns valid attributes' do + flaky_example = described_class.new(args) + + expect(flaky_example.uid).to eq(flaky_example_attrs[:uid]) + expect(flaky_example.example_id).to eq(flaky_example_attrs[:example_id]) + end + end + + context 'when given an Rspec::Example' do + let(:args) { example } + + it_behaves_like 'a valid FlakyExample instance' + end + + context 'when given a hash' do + let(:args) { flaky_example_attrs } + + it_behaves_like 'a valid FlakyExample instance' + end + end + + describe '#to_h' do + before do + # Stub these env variables otherwise specs don't behave the same on the CI + stub_env('CI_PROJECT_URL', nil) + stub_env('CI_JOB_ID', nil) + end + + shared_examples 'a valid FlakyExample hash' do + let(:additional_attrs) { {} } + + it 'returns a valid hash' do + flaky_example = described_class.new(args) + final_hash = flaky_example_attrs + .merge(last_flaky_at: instance_of(Time), last_flaky_job: nil) + .merge(additional_attrs) + + expect(flaky_example.to_h).to match(hash_including(final_hash)) + end + end + + context 'when given an Rspec::Example' do + let(:args) { example } + + context 'when run locally' do + it_behaves_like 'a valid FlakyExample hash' do + let(:additional_attrs) do + { first_flaky_at: instance_of(Time) } + end + end + end + + context 'when run on the CI' do + before do + stub_env('CI_PROJECT_URL', 'https://gitlab.com/gitlab-org/gitlab-ce') + stub_env('CI_JOB_ID', 42) + end + + it_behaves_like 'a valid FlakyExample hash' do + let(:additional_attrs) do + { first_flaky_at: instance_of(Time), last_flaky_job: "https://gitlab.com/gitlab-org/gitlab-ce/-/jobs/42" } + end + end + end + end + + context 'when given a hash' do + let(:args) { flaky_example_attrs } + + it_behaves_like 'a valid FlakyExample hash' + end + end +end diff --git a/spec/lib/rspec_flaky/listener_spec.rb b/spec/lib/rspec_flaky/listener_spec.rb new file mode 100644 index 00000000000..0e193bf408b --- /dev/null +++ b/spec/lib/rspec_flaky/listener_spec.rb @@ -0,0 +1,178 @@ +require 'spec_helper' + +describe RspecFlaky::Listener do + let(:flaky_example_report) do + { + 'abc123' => { + example_id: 'spec/foo/bar_spec.rb:2', + file: 'spec/foo/bar_spec.rb', + line: 2, + description: 'hello world', + first_flaky_at: 1234, + last_flaky_at: instance_of(Time), + last_attempts_count: 2, + flaky_reports: 1, + last_flaky_job: nil + } + } + end + let(:example_attrs) do + { + id: 'spec/foo/baz_spec.rb:3', + metadata: { + file_path: 'spec/foo/baz_spec.rb', + line_number: 3, + full_description: 'hello GitLab' + }, + execution_result: double(status: 'passed', exception: nil) + } + end + + before do + # Stub these env variables otherwise specs don't behave the same on the CI + stub_env('CI_PROJECT_URL', nil) + stub_env('CI_JOB_ID', nil) + end + + describe '#initialize' do + shared_examples 'a valid Listener instance' do + let(:expected_all_flaky_examples) { {} } + + it 'returns a valid Listener instance' do + listener = described_class.new + + expect(listener.to_report(listener.all_flaky_examples)) + .to match(hash_including(expected_all_flaky_examples)) + expect(listener.new_flaky_examples).to eq({}) + end + end + + context 'when no report file exists' do + it_behaves_like 'a valid Listener instance' + end + + context 'when a report file exists and set by ALL_FLAKY_RSPEC_REPORT_PATH' do + let(:report_file) do + Tempfile.new(%w[rspec_flaky_report .json]).tap do |f| + f.write(JSON.pretty_generate(flaky_example_report)) + f.rewind + end + end + + before do + stub_env('ALL_FLAKY_RSPEC_REPORT_PATH', report_file.path) + end + + after do + report_file.close + report_file.unlink + end + + it_behaves_like 'a valid Listener instance' do + let(:expected_all_flaky_examples) { flaky_example_report } + end + end + end + + describe '#example_passed' do + let(:rspec_example) { double(example_attrs) } + let(:notification) { double(example: rspec_example) } + + shared_examples 'a non-flaky example' do + it 'does not change the flaky examples hash' do + expect { subject.example_passed(notification) } + .not_to change { subject.all_flaky_examples } + end + end + + describe 'when the RSpec example does not respond to attempts' do + it_behaves_like 'a non-flaky example' + end + + describe 'when the RSpec example has 1 attempt' do + let(:rspec_example) { double(example_attrs.merge(attempts: 1)) } + + it_behaves_like 'a non-flaky example' + end + + describe 'when the RSpec example has 2 attempts' do + let(:rspec_example) { double(example_attrs.merge(attempts: 2)) } + let(:expected_new_flaky_example) do + { + example_id: 'spec/foo/baz_spec.rb:3', + file: 'spec/foo/baz_spec.rb', + line: 3, + description: 'hello GitLab', + first_flaky_at: instance_of(Time), + last_flaky_at: instance_of(Time), + last_attempts_count: 2, + flaky_reports: 1, + last_flaky_job: nil + } + end + + it 'does not change the flaky examples hash' do + expect { subject.example_passed(notification) } + .to change { subject.all_flaky_examples } + + new_example = RspecFlaky::Example.new(rspec_example) + + expect(subject.all_flaky_examples[new_example.uid].to_h) + .to match(hash_including(expected_new_flaky_example)) + end + end + end + + describe '#dump_summary' do + let(:rspec_example) { double(example_attrs) } + let(:notification) { double(example: rspec_example) } + + context 'when a report file path is set by ALL_FLAKY_RSPEC_REPORT_PATH' do + let(:report_file_path) { Rails.root.join('tmp', 'rspec_flaky_report.json') } + + before do + stub_env('ALL_FLAKY_RSPEC_REPORT_PATH', report_file_path) + FileUtils.rm(report_file_path) if File.exist?(report_file_path) + end + + after do + FileUtils.rm(report_file_path) if File.exist?(report_file_path) + end + + context 'when FLAKY_RSPEC_GENERATE_REPORT == "false"' do + before do + stub_env('FLAKY_RSPEC_GENERATE_REPORT', 'false') + end + + it 'does not write the report file' do + subject.example_passed(notification) + + subject.dump_summary(nil) + + expect(File.exist?(report_file_path)).to be(false) + end + end + + context 'when FLAKY_RSPEC_GENERATE_REPORT == "true"' do + before do + stub_env('FLAKY_RSPEC_GENERATE_REPORT', 'true') + end + + it 'writes the report file' do + subject.example_passed(notification) + + subject.dump_summary(nil) + + expect(File.exist?(report_file_path)).to be(true) + end + end + end + end + + describe '#to_report' do + it 'transforms the internal hash to a JSON-ready hash' do + expect(subject.to_report('abc123' => RspecFlaky::FlakyExample.new(flaky_example_report['abc123']))) + .to match(hash_including(flaky_example_report)) + end + end +end diff --git a/spec/models/appearance_spec.rb b/spec/models/appearance_spec.rb index 7cd3a84d592..b5d5d58697b 100644 --- a/spec/models/appearance_spec.rb +++ b/spec/models/appearance_spec.rb @@ -9,4 +9,39 @@ RSpec.describe Appearance do it { is_expected.to validate_presence_of(:description) } it { is_expected.to have_many(:uploads).dependent(:destroy) } + + describe '.current', :use_clean_rails_memory_store_caching do + let!(:appearance) { create(:appearance) } + + it 'returns the current appearance row' do + expect(described_class.current).to eq(appearance) + end + + it 'caches the result' do + expect(described_class).to receive(:first).once + + 2.times { described_class.current } + end + end + + describe '#flush_redis_cache' do + it 'flushes the cache in Redis' do + appearance = create(:appearance) + + expect(Rails.cache).to receive(:delete).with(described_class::CACHE_KEY) + + appearance.flush_redis_cache + end + end + + describe '#single_appearance_row' do + it 'adds an error when more than 1 row exists' do + create(:appearance) + + new_row = build(:appearance) + new_row.save + + expect(new_row.valid?).to eq(false) + end + end end diff --git a/spec/models/broadcast_message_spec.rb b/spec/models/broadcast_message_spec.rb index a8ca1d110e4..3369aef1d3e 100644 --- a/spec/models/broadcast_message_spec.rb +++ b/spec/models/broadcast_message_spec.rb @@ -20,7 +20,7 @@ describe BroadcastMessage do it { is_expected.not_to allow_value('000').for(:font) } end - describe '.current' do + describe '.current', :use_clean_rails_memory_store_caching do it 'returns message if time match' do message = create(:broadcast_message) @@ -45,6 +45,14 @@ describe BroadcastMessage do expect(described_class.current).to be_empty end + + it 'caches the output of the query' do + create(:broadcast_message) + + expect(described_class).to receive(:where).and_call_original.once + + 2.times { described_class.current } + end end describe '#active?' do @@ -102,4 +110,14 @@ describe BroadcastMessage do end end end + + describe '#flush_redis_cache' do + it 'flushes the Redis cache' do + message = create(:broadcast_message) + + expect(Rails.cache).to receive(:delete).with(described_class::CACHE_KEY) + + message.flush_redis_cache + end + end end diff --git a/spec/models/event_collection_spec.rb b/spec/models/event_collection_spec.rb new file mode 100644 index 00000000000..e0a87c18cc7 --- /dev/null +++ b/spec/models/event_collection_spec.rb @@ -0,0 +1,51 @@ +require 'spec_helper' + +describe EventCollection do + describe '#to_a' do + let(:project) { create(:project_empty_repo) } + let(:projects) { Project.where(id: project.id) } + let(:user) { create(:user) } + + before do + 20.times do + event = create(:push_event, project: project, author: user) + + create(:push_event_payload, event: event) + end + + create(:closed_issue_event, project: project, author: user) + end + + it 'returns an Array of events' do + events = described_class.new(projects).to_a + + expect(events).to be_an_instance_of(Array) + end + + it 'applies a limit to the number of events' do + events = described_class.new(projects).to_a + + expect(events.length).to eq(20) + end + + it 'can paginate through events' do + events = described_class.new(projects, offset: 20).to_a + + expect(events.length).to eq(1) + end + + it 'returns an empty Array when crossing the maximum page number' do + events = described_class.new(projects, limit: 1, offset: 15).to_a + + expect(events).to be_empty + end + + it 'allows filtering of events using an EventFilter' do + filter = EventFilter.new(EventFilter.issue) + events = described_class.new(projects, filter: filter).to_a + + expect(events.length).to eq(1) + expect(events[0].action).to eq(Event::CLOSED) + end + end +end diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index d86bf1a90a9..ff3224dd298 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -304,27 +304,15 @@ describe Event do end end - def create_push_event(project, user, attrs = {}) - data = { - before: Gitlab::Git::BLANK_SHA, - after: "0220c11b9a3e6c69dc8fd35321254ca9a7b98f7e", - ref: "refs/heads/master", - user_id: user.id, - user_name: user.name, - repository: { - name: project.name, - url: "localhost/rubinius", - description: "", - homepage: "localhost/rubinius", - private: true - } - } - - described_class.create({ - project: project, - action: described_class::PUSHED, - data: data, - author_id: user.id - }.merge!(attrs)) + def create_push_event(project, user) + event = create(:push_event, project: project, author: user) + + create(:push_event_payload, + event: event, + commit_to: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', + commit_count: 0, + ref: 'master') + + event end end diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index f1d1f37c78a..fa3e80ba062 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -149,7 +149,7 @@ describe ProjectMember do describe 'notifications' do describe '#after_accept_request' do it 'calls NotificationService.new_project_member' do - member = create(:project_member, user: build_stubbed(:user), requested_at: Time.now) + member = create(:project_member, user: create(:user), requested_at: Time.now) expect_any_instance_of(NotificationService).to receive(:new_project_member) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index a28e92446ea..d9ab44dc49f 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -485,7 +485,7 @@ describe Project do describe 'last_activity' do it 'alias last_activity to last_event' do - last_event = create(:event, project: project) + last_event = create(:event, :closed, project: project) expect(project.last_activity).to eq(last_event) end @@ -493,7 +493,7 @@ describe Project do describe 'last_activity_date' do it 'returns the creation date of the project\'s last event if present' do - new_event = create(:event, project: project, created_at: Time.now) + new_event = create(:event, :closed, project: project, created_at: Time.now) project.reload expect(project.last_activity_at.to_i).to eq(new_event.created_at.to_i) diff --git a/spec/models/push_event_payload_spec.rb b/spec/models/push_event_payload_spec.rb new file mode 100644 index 00000000000..a049ad35584 --- /dev/null +++ b/spec/models/push_event_payload_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe PushEventPayload do + describe 'saving payloads' do + it 'does not allow commit messages longer than 70 characters' do + event = create(:push_event) + payload = build(:push_event_payload, event: event) + + expect(payload).to be_valid + + payload.commit_title = 'a' * 100 + + expect(payload).not_to be_valid + end + end +end diff --git a/spec/models/push_event_spec.rb b/spec/models/push_event_spec.rb new file mode 100644 index 00000000000..532fb024261 --- /dev/null +++ b/spec/models/push_event_spec.rb @@ -0,0 +1,202 @@ +require 'spec_helper' + +describe PushEvent do + let(:payload) { PushEventPayload.new } + + let(:event) do + event = described_class.new + + allow(event).to receive(:push_event_payload).and_return(payload) + + event + end + + describe '.sti_name' do + it 'returns Event::PUSHED' do + expect(described_class.sti_name).to eq(Event::PUSHED) + end + end + + describe '#push?' do + it 'returns true' do + expect(event).to be_push + end + end + + describe '#push_with_commits?' do + it 'returns true when both the first and last commit are present' do + allow(event).to receive(:commit_from).and_return('123') + allow(event).to receive(:commit_to).and_return('456') + + expect(event).to be_push_with_commits + end + + it 'returns false when the first commit is missing' do + allow(event).to receive(:commit_to).and_return('456') + + expect(event).not_to be_push_with_commits + end + + it 'returns false when the last commit is missing' do + allow(event).to receive(:commit_from).and_return('123') + + expect(event).not_to be_push_with_commits + end + end + + describe '#tag?' do + it 'returns true when pushing to a tag' do + allow(payload).to receive(:tag?).and_return(true) + + expect(event).to be_tag + end + + it 'returns false when pushing to a branch' do + allow(payload).to receive(:tag?).and_return(false) + + expect(event).not_to be_tag + end + end + + describe '#branch?' do + it 'returns true when pushing to a branch' do + allow(payload).to receive(:branch?).and_return(true) + + expect(event).to be_branch + end + + it 'returns false when pushing to a tag' do + allow(payload).to receive(:branch?).and_return(false) + + expect(event).not_to be_branch + end + end + + describe '#valid_push?' do + it 'returns true if a ref exists' do + allow(payload).to receive(:ref).and_return('master') + + expect(event).to be_valid_push + end + + it 'returns false when no ref is present' do + expect(event).not_to be_valid_push + end + end + + describe '#new_ref?' do + it 'returns true when pushing a new ref' do + allow(payload).to receive(:created?).and_return(true) + + expect(event).to be_new_ref + end + + it 'returns false when pushing to an existing ref' do + allow(payload).to receive(:created?).and_return(false) + + expect(event).not_to be_new_ref + end + end + + describe '#rm_ref?' do + it 'returns true when removing an existing ref' do + allow(payload).to receive(:removed?).and_return(true) + + expect(event).to be_rm_ref + end + + it 'returns false when pushing to an existing ref' do + allow(payload).to receive(:removed?).and_return(false) + + expect(event).not_to be_rm_ref + end + end + + describe '#commit_from' do + it 'returns the first commit SHA' do + allow(payload).to receive(:commit_from).and_return('123') + + expect(event.commit_from).to eq('123') + end + end + + describe '#commit_to' do + it 'returns the last commit SHA' do + allow(payload).to receive(:commit_to).and_return('123') + + expect(event.commit_to).to eq('123') + end + end + + describe '#ref_name' do + it 'returns the name of the ref' do + allow(payload).to receive(:ref).and_return('master') + + expect(event.ref_name).to eq('master') + end + end + + describe '#ref_type' do + it 'returns the type of the ref' do + allow(payload).to receive(:ref_type).and_return('branch') + + expect(event.ref_type).to eq('branch') + end + end + + describe '#branch_name' do + it 'returns the name of the branch' do + allow(payload).to receive(:ref).and_return('master') + + expect(event.branch_name).to eq('master') + end + end + + describe '#tag_name' do + it 'returns the name of the tag' do + allow(payload).to receive(:ref).and_return('1.2') + + expect(event.tag_name).to eq('1.2') + end + end + + describe '#commit_title' do + it 'returns the commit message' do + allow(payload).to receive(:commit_title).and_return('foo') + + expect(event.commit_title).to eq('foo') + end + end + + describe '#commit_id' do + it 'returns the SHA of the last commit if present' do + allow(event).to receive(:commit_to).and_return('123') + + expect(event.commit_id).to eq('123') + end + + it 'returns the SHA of the first commit if the last commit is not present' do + allow(event).to receive(:commit_to).and_return(nil) + allow(event).to receive(:commit_from).and_return('123') + + expect(event.commit_id).to eq('123') + end + end + + describe '#commits_count' do + it 'returns the number of commits' do + allow(payload).to receive(:commit_count).and_return(1) + + expect(event.commits_count).to eq(1) + end + end + + describe '#validate_push_action' do + it 'adds an error when the action is not PUSHED' do + event.action = Event::CREATED + event.validate_push_action + + expect(event.errors.count).to eq(1) + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6c8248eeb40..97bb91a6ac8 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1291,7 +1291,7 @@ describe User do let!(:project2) { create(:project, forked_from_project: project3) } let!(:project3) { create(:project) } let!(:merge_request) { create(:merge_request, source_project: project2, target_project: project3, author: subject) } - let!(:push_event) { create(:event, :pushed, project: project1, target: project1, author: subject) } + let!(:push_event) { create(:push_event, project: project1, author: subject) } let!(:merge_event) { create(:event, :created, project: project3, target: merge_request, author: subject) } before do @@ -1333,10 +1333,18 @@ describe User do subject { create(:user) } let!(:project1) { create(:project, :repository) } let!(:project2) { create(:project, :repository, forked_from_project: project1) } - let!(:push_data) do - Gitlab::DataBuilder::Push.build_sample(project2, subject) + + let!(:push_event) do + event = create(:push_event, project: project2, author: subject) + + create(:push_event_payload, + event: event, + commit_to: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', + commit_count: 0, + ref: 'master') + + event end - let!(:push_event) { create(:event, :pushed, project: project2, target: project1, author: subject, data: push_data) } before do project1.team << [subject, :master] @@ -1363,8 +1371,13 @@ describe User do expect(subject.recent_push(project1)).to eq(nil) expect(subject.recent_push(project2)).to eq(push_event) - push_data1 = Gitlab::DataBuilder::Push.build_sample(project1, subject) - push_event1 = create(:event, :pushed, project: project1, target: project1, author: subject, data: push_data1) + push_event1 = create(:push_event, project: project1, author: subject) + + create(:push_event_payload, + event: push_event1, + commit_to: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', + commit_count: 0, + ref: 'master') expect(subject.recent_push([project1, project2])).to eq(push_event1) # Newest end diff --git a/spec/requests/api/events_spec.rb b/spec/requests/api/events_spec.rb index f1a26b6ce6c..a23d28994ce 100644 --- a/spec/requests/api/events_spec.rb +++ b/spec/requests/api/events_spec.rb @@ -59,6 +59,34 @@ describe API::Events do expect(json_response.size).to eq(1) end + context 'when the list of events includes push events' do + let(:event) do + create(:push_event, author: user, project: private_project) + end + + let!(:payload) { create(:push_event_payload, event: event) } + let(:payload_hash) { json_response[0]['push_data'] } + + before do + get api("/users/#{user.id}/events?action=pushed", user) + end + + it 'responds with HTTP 200 OK' do + expect(response).to have_http_status(200) + end + + it 'includes the push payload as a Hash' do + expect(payload_hash).to be_an_instance_of(Hash) + end + + it 'includes the push payload details' do + expect(payload_hash['commit_count']).to eq(payload.commit_count) + expect(payload_hash['action']).to eq(payload.action) + expect(payload_hash['ref_type']).to eq(payload.ref_type) + expect(payload_hash['commit_to']).to eq(payload.commit_to) + end + end + context 'when there are multiple events from different projects' do let(:second_note) { create(:note_on_issue, project: create(:project)) } diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 2dc7be22f8f..49739a1601a 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -217,9 +217,19 @@ describe API::Users do it "does not return the user's `is_admin` flag" do get api("/users/#{user.id}", user) + expect(response).to have_http_status(200) expect(json_response['is_admin']).to be_nil end + context 'when authenticated as admin' do + it 'includes the `is_admin` field' do + get api("/users/#{user.id}", admin) + + expect(response).to have_http_status(200) + expect(json_response['is_admin']).to be(false) + end + end + context 'for an anonymous user' do it "returns a user by id" do get api("/users/#{user.id}") diff --git a/spec/requests/api/v3/users_spec.rb b/spec/requests/api/v3/users_spec.rb index bc0a4ab20a3..227b8d1b0c1 100644 --- a/spec/requests/api/v3/users_spec.rb +++ b/spec/requests/api/v3/users_spec.rb @@ -252,6 +252,31 @@ describe API::V3::Users do end context "as a user than can see the event's project" do + context 'when the list of events includes push events' do + let(:event) { create(:push_event, author: user, project: project) } + let!(:payload) { create(:push_event_payload, event: event) } + let(:payload_hash) { json_response[0]['push_data'] } + + before do + get api("/users/#{user.id}/events?action=pushed", user) + end + + it 'responds with HTTP 200 OK' do + expect(response).to have_http_status(200) + end + + it 'includes the push payload as a Hash' do + expect(payload_hash).to be_an_instance_of(Hash) + end + + it 'includes the push payload details' do + expect(payload_hash['commit_count']).to eq(payload.commit_count) + expect(payload_hash['action']).to eq(payload.action) + expect(payload_hash['ref_type']).to eq(payload.ref_type) + expect(payload_hash['commit_to']).to eq(payload.commit_to) + end + end + context 'joined event' do it 'returns the "joined" event' do get v3_api("/users/#{user.id}/events", user) diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 42adb044190..02d7ddeb86b 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -117,12 +117,52 @@ describe EventCreateService do let(:project) { create(:project) } let(:user) { create(:user) } + let(:push_data) do + { + commits: [ + { + id: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', + message: 'This is a commit' + } + ], + before: '0000000000000000000000000000000000000000', + after: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', + total_commits_count: 1, + ref: 'refs/heads/my-branch' + } + end + it 'creates a new event' do - expect { service.push(project, user, {}) }.to change { Event.count } + expect { service.push(project, user, push_data) }.to change { Event.count } + end + + it 'creates the push event payload' do + expect(PushEventPayloadService).to receive(:new) + .with(an_instance_of(PushEvent), push_data) + .and_call_original + + service.push(project, user, push_data) end it 'updates user last activity' do - expect { service.push(project, user, {}) }.to change { user_activity(user) } + expect { service.push(project, user, push_data) } + .to change { user_activity(user) } + end + + it 'does not create any event data when an error is raised' do + payload_service = double(:service) + + allow(payload_service).to receive(:execute) + .and_raise(RuntimeError) + + allow(PushEventPayloadService).to receive(:new) + .and_return(payload_service) + + expect { service.push(project, user, push_data) } + .to raise_error(RuntimeError) + + expect(Event.count).to eq(0) + expect(PushEventPayload.count).to eq(0) end end diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index a6449a3c9f5..8485605b398 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -141,10 +141,13 @@ describe GitPushService, services: true do let!(:push_data) { push_data_from_service(project, user, oldrev, newrev, ref) } let(:event) { Event.find_by_action(Event::PUSHED) } - it { expect(event).not_to be_nil } + it { expect(event).to be_an_instance_of(PushEvent) } it { expect(event.project).to eq(project) } it { expect(event.action).to eq(Event::PUSHED) } - it { expect(event.data).to eq(push_data) } + it { expect(event.push_event_payload).to be_an_instance_of(PushEventPayload) } + it { expect(event.push_event_payload.commit_from).to eq(oldrev) } + it { expect(event.push_event_payload.commit_to).to eq(newrev) } + it { expect(event.push_event_payload.ref).to eq('master') } context "Updates merge requests" do it "when pushing a new branch for the first time" do diff --git a/spec/services/push_event_payload_service_spec.rb b/spec/services/push_event_payload_service_spec.rb new file mode 100644 index 00000000000..81956200bff --- /dev/null +++ b/spec/services/push_event_payload_service_spec.rb @@ -0,0 +1,218 @@ +require 'spec_helper' + +describe PushEventPayloadService do + let(:event) { create(:push_event) } + + describe '#execute' do + let(:push_data) do + { + commits: [ + { + id: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', + message: 'This is a commit' + } + ], + before: '0000000000000000000000000000000000000000', + after: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', + total_commits_count: 1, + ref: 'refs/heads/my-branch' + } + end + + it 'creates a new PushEventPayload row' do + payload = described_class.new(event, push_data).execute + + expect(payload.commit_count).to eq(1) + expect(payload.action).to eq('created') + expect(payload.ref_type).to eq('branch') + expect(payload.commit_from).to be_nil + expect(payload.commit_to).to eq(push_data[:after]) + expect(payload.ref).to eq('my-branch') + expect(payload.commit_title).to eq('This is a commit') + expect(payload.event_id).to eq(event.id) + end + + it 'sets the push_event_payload association of the used event' do + payload = described_class.new(event, push_data).execute + + expect(event.push_event_payload).to eq(payload) + end + end + + describe '#commit_title' do + it 'returns nil if no commits were pushed' do + service = described_class.new(event, commits: []) + + expect(service.commit_title).to be_nil + end + + it 'returns a String limited to 70 characters' do + service = described_class.new(event, commits: [{ message: 'a' * 100 }]) + + expect(service.commit_title).to eq(('a' * 67) + '...') + end + + it 'does not truncate the commit message if it is shorter than 70 characters' do + service = described_class.new(event, commits: [{ message: 'Hello' }]) + + expect(service.commit_title).to eq('Hello') + end + + it 'includes the first line of a commit message if the message spans multiple lines' do + service = described_class + .new(event, commits: [{ message: "Hello\n\nworld" }]) + + expect(service.commit_title).to eq('Hello') + end + end + + describe '#commit_from_id' do + it 'returns nil when creating a new ref' do + service = described_class.new(event, before: Gitlab::Git::BLANK_SHA) + + expect(service.commit_from_id).to be_nil + end + + it 'returns the ID of the first commit when pushing to an existing ref' do + service = described_class.new(event, before: '123') + + expect(service.commit_from_id).to eq('123') + end + end + + describe '#commit_to_id' do + it 'returns nil when removing an existing ref' do + service = described_class.new(event, after: Gitlab::Git::BLANK_SHA) + + expect(service.commit_to_id).to be_nil + end + end + + describe '#commit_count' do + it 'returns the number of commits' do + service = described_class.new(event, total_commits_count: 1) + + expect(service.commit_count).to eq(1) + end + + it 'raises when the push data does not contain the commits count' do + service = described_class.new(event, {}) + + expect { service.commit_count }.to raise_error(KeyError) + end + end + + describe '#ref' do + it 'returns the name of the ref' do + service = described_class.new(event, ref: 'refs/heads/foo') + + expect(service.ref).to eq('refs/heads/foo') + end + + it 'raises when the push data does not contain the ref name' do + service = described_class.new(event, {}) + + expect { service.ref }.to raise_error(KeyError) + end + end + + describe '#revision_before' do + it 'returns the revision from before the push' do + service = described_class.new(event, before: 'foo') + + expect(service.revision_before).to eq('foo') + end + + it 'raises when the push data does not contain the before revision' do + service = described_class.new(event, {}) + + expect { service.revision_before }.to raise_error(KeyError) + end + end + + describe '#revision_after' do + it 'returns the revision from after the push' do + service = described_class.new(event, after: 'foo') + + expect(service.revision_after).to eq('foo') + end + + it 'raises when the push data does not contain the after revision' do + service = described_class.new(event, {}) + + expect { service.revision_after }.to raise_error(KeyError) + end + end + + describe '#trimmed_ref' do + it 'returns the ref name without its prefix' do + service = described_class.new(event, ref: 'refs/heads/foo') + + expect(service.trimmed_ref).to eq('foo') + end + end + + describe '#create?' do + it 'returns true when creating a new ref' do + service = described_class.new(event, before: Gitlab::Git::BLANK_SHA) + + expect(service.create?).to eq(true) + end + + it 'returns false when pushing to an existing ref' do + service = described_class.new(event, before: 'foo') + + expect(service.create?).to eq(false) + end + end + + describe '#remove?' do + it 'returns true when removing an existing ref' do + service = described_class.new(event, after: Gitlab::Git::BLANK_SHA) + + expect(service.remove?).to eq(true) + end + + it 'returns false pushing to an existing ref' do + service = described_class.new(event, after: 'foo') + + expect(service.remove?).to eq(false) + end + end + + describe '#action' do + it 'returns :created when creating a ref' do + service = described_class.new(event, before: Gitlab::Git::BLANK_SHA) + + expect(service.action).to eq(:created) + end + + it 'returns :removed when removing an existing ref' do + service = described_class.new(event, + before: '123', + after: Gitlab::Git::BLANK_SHA) + + expect(service.action).to eq(:removed) + end + + it 'returns :pushed when pushing to an existing ref' do + service = described_class.new(event, before: '123', after: '456') + + expect(service.action).to eq(:pushed) + end + end + + describe '#ref_type' do + it 'returns :tag for a tag' do + service = described_class.new(event, ref: 'refs/tags/1.2') + + expect(service.ref_type).to eq(:tag) + end + + it 'returns :branch for a branch' do + service = described_class.new(event, ref: 'refs/heads/master') + + expect(service.ref_type).to eq(:branch) + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 3bece3ab720..0cddbe89833 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -69,6 +69,12 @@ RSpec.configure do |config| config.raise_errors_for_deprecations! + if ENV['CI'] + # This includes the first try, i.e. tests will be run 4 times before failing. + config.default_retry_count = 4 + config.reporter.register_listener(RspecFlaky::Listener.new, :example_passed, :dump_summary) + end + config.before(:suite) do Timecop.safe_mode = true TestEnv.init @@ -98,12 +104,6 @@ RSpec.configure do |config| reset_delivered_emails! end - if ENV['CI'] - config.around(:each) do |ex| - ex.run_with_retry retry: 2 - end - end - config.around(:each, :use_clean_rails_memory_store_caching) do |example| caching_store = Rails.cache Rails.cache = ActiveSupport::Cache::MemoryStore.new diff --git a/spec/workers/prune_old_events_worker_spec.rb b/spec/workers/prune_old_events_worker_spec.rb index 35e1518a35e..ea974355050 100644 --- a/spec/workers/prune_old_events_worker_spec.rb +++ b/spec/workers/prune_old_events_worker_spec.rb @@ -2,9 +2,11 @@ require 'spec_helper' describe PruneOldEventsWorker do describe '#perform' do - let!(:expired_event) { create(:event, author_id: 0, created_at: 13.months.ago) } - let!(:not_expired_event) { create(:event, author_id: 0, created_at: 1.day.ago) } - let!(:exactly_12_months_event) { create(:event, author_id: 0, created_at: 12.months.ago) } + let(:user) { create(:user) } + + let!(:expired_event) { create(:event, :closed, author: user, created_at: 13.months.ago) } + let!(:not_expired_event) { create(:event, :closed, author: user, created_at: 1.day.ago) } + let!(:exactly_12_months_event) { create(:event, :closed, author: user, created_at: 12.months.ago) } it 'prunes events older than 12 months' do expect { subject.perform }.to change { Event.count }.by(-1) |