From c7d945758accc6f80ee63c7c3b25782cf7f6f3b0 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 2 Nov 2017 19:38:25 +0100 Subject: [PATCH] Fix most test failures --- app/models/ci/build.rb | 9 ++++----- app/models/ci/job_artifact.rb | 2 +- app/models/project_statistics.rb | 3 +-- app/uploaders/job_artifact_uploader.rb | 12 ++++-------- db/migrate/20170918072948_create_job_artifacts.rb | 4 +++- db/schema.rb | 5 +++-- spec/factories/ci/builds.rb | 1 + spec/features/projects/pipelines/pipelines_spec.rb | 2 +- spec/serializers/pipeline_serializer_spec.rb | 2 +- spec/services/ci/retry_build_service_spec.rb | 4 ++-- spec/tasks/gitlab/backup_rake_spec.rb | 1 + 11 files changed, 22 insertions(+), 23 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8d0c62fcb49..161800e9061 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -15,10 +15,9 @@ module Ci has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_many :trace_sections, class_name: 'Ci::BuildTraceSection' - # TODO: what to do with dependent destroy - has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id, dependent: :destroy - has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id - has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @@ -39,7 +38,7 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } scope :with_artifacts, ->() do - where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.ci_job_id')) + where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id')) end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 9c709077ac6..cc28092978c 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -3,7 +3,7 @@ module Ci extend Gitlab::Ci::Model belongs_to :project - belongs_to :job, class_name: "Ci::Build", foreign_key: :ci_job_id + belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id before_save :set_size, if: :file_changed? after_commit :remove_file!, on: :destroy diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index a9c22d9cf18..17b9d2cf7b4 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -37,8 +37,7 @@ class ProjectStatistics < ActiveRecord::Base def update_build_artifacts_size self.build_artifacts_size = project.builds.sum(:artifacts_size) + - Ci::JobArtifact.artifacts_size_for(self) + - Ci::JobArtifactMetadata.artifacts_size_for(self) + Ci::JobArtifact.artifacts_size_for(self) end def update_storage_size diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb index cc6e1927f9e..7185e24287f 100644 --- a/app/uploaders/job_artifact_uploader.rb +++ b/app/uploaders/job_artifact_uploader.rb @@ -4,7 +4,7 @@ class JobArtifactUploader < ArtifactUploader end def size - return super unless @artifact.size + return super if @artifact.size.nil? @artifact.size end @@ -12,17 +12,13 @@ class JobArtifactUploader < ArtifactUploader private def disk_hash - @disk_hash ||= Digest::SHA2.hexdigest(job.project_id.to_s) + @disk_hash ||= Digest::SHA2.hexdigest(@artifact.project_id.to_s) end def default_path - creation_date = job.created_at.utc.strftime('%Y_%m_%d') + creation_date = @artifact.created_at.utc.strftime('%Y_%m_%d') File.join(disk_hash[0..1], disk_hash[2..3], disk_hash, - creation_date, job.id.to_s, @artifact.id.to_s) - end - - def job - @artifact.job + creation_date, @artifact.ci_job_id.to_s, @artifact.id.to_s) end end diff --git a/db/migrate/20170918072948_create_job_artifacts.rb b/db/migrate/20170918072948_create_job_artifacts.rb index 6d1756f368c..078e3e9dc4e 100644 --- a/db/migrate/20170918072948_create_job_artifacts.rb +++ b/db/migrate/20170918072948_create_job_artifacts.rb @@ -6,7 +6,7 @@ class CreateJobArtifacts < ActiveRecord::Migration def change create_table :ci_job_artifacts do |t| t.belongs_to :project, null: false, index: true, foreign_key: { on_delete: :cascade } - t.integer :ci_job_id, null: false, index: true + t.integer :job_id, null: false, index: true t.integer :size, limit: 8 t.integer :file_type, null: false, index: true @@ -15,5 +15,7 @@ class CreateJobArtifacts < ActiveRecord::Migration t.string :file end + + add_foreign_key :ci_job_artifacts, :ci_builds, column: :job_id, on_delete: :cascade end end diff --git a/db/schema.rb b/db/schema.rb index 46a3b147198..0f71463acd8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -321,7 +321,7 @@ ActiveRecord::Schema.define(version: 20171124150326) do create_table "ci_job_artifacts", force: :cascade do |t| t.integer "project_id", null: false - t.integer "ci_job_id", null: false + t.integer "job_id", null: false t.integer "size", limit: 8 t.integer "file_type", null: false t.datetime_with_timezone "created_at", null: false @@ -329,8 +329,8 @@ ActiveRecord::Schema.define(version: 20171124150326) do t.string "file" end - add_index "ci_job_artifacts", ["ci_job_id"], name: "index_ci_job_artifacts_on_ci_job_id", using: :btree add_index "ci_job_artifacts", ["file_type"], name: "index_ci_job_artifacts_on_file_type", using: :btree + add_index "ci_job_artifacts", ["job_id"], name: "index_ci_job_artifacts_on_job_id", using: :btree add_index "ci_job_artifacts", ["project_id"], name: "index_ci_job_artifacts_on_project_id", using: :btree create_table "ci_pipeline_schedule_variables", force: :cascade do |t| @@ -1923,6 +1923,7 @@ ActiveRecord::Schema.define(version: 20171124150326) do add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade + add_foreign_key "ci_job_artifacts", "ci_builds", column: "job_id", on_delete: :cascade add_foreign_key "ci_job_artifacts", "projects", on_delete: :cascade add_foreign_key "ci_pipeline_schedule_variables", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_41c35fda51", on_delete: :cascade add_foreign_key "ci_pipeline_schedules", "projects", name: "fk_8ead60fcc4", on_delete: :cascade diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index ee449bfe5db..2ce49ede4fa 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -158,6 +158,7 @@ FactoryGirl.define do after(:create) do |build| create(:ci_job_artifact, job: build) create(:ci_job_metadata, job: build) + build.reload end end diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index a1b1d94ae06..b87b47d0e1a 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -304,7 +304,7 @@ describe 'Pipelines', :js do context 'with artifacts expired' do let!(:with_artifacts_expired) do - create(:ci_build, :artifacts_expired, :success, + create(:ci_build, :expired, :success, pipeline: pipeline, name: 'rspec', stage: 'test') diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 7144b66585c..88d347322a6 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -117,7 +117,7 @@ describe PipelineSerializer do shared_examples 'no N+1 queries' do it 'verifies number of queries', :request_store do recorded = ActiveRecord::QueryRecorder.new { subject } - expect(recorded.count).to be_within(1).of(57) + expect(recorded.count).to be_within(1).of(36) expect(recorded.cached_count).to eq(0) end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index b61d1cb765e..20fe80beade 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -17,7 +17,7 @@ describe Ci::RetryBuildService do %i[id status user token coverage trace runner artifacts_expire_at artifacts_file artifacts_metadata artifacts_size created_at updated_at started_at finished_at queued_at erased_by - erased_at auto_canceled_by].freeze + erased_at auto_canceled_by job_artifacts job_archive job_metadata].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections @@ -34,7 +34,7 @@ describe Ci::RetryBuildService do end let(:build) do - create(:ci_build, :failed, :artifacts_expired, :erased, + create(:ci_build, :failed, :artifacts, :expired, :erased, :queued, :coverage, :tags, :allowed_to_fail, :on_tag, :triggered, :trace, :teardown_environment, description: 'my-job', stage: 'test', pipeline: pipeline, diff --git a/spec/tasks/gitlab/backup_rake_spec.rb b/spec/tasks/gitlab/backup_rake_spec.rb index bf2e11bc360..52f81106829 100644 --- a/spec/tasks/gitlab/backup_rake_spec.rb +++ b/spec/tasks/gitlab/backup_rake_spec.rb @@ -24,6 +24,7 @@ describe 'gitlab:app namespace rake task' do # We need this directory to run `gitlab:backup:create` task FileUtils.mkdir_p('public/uploads') + FileUtils.mkdir_p(Rails.root.join('tmp/tests/artifacts')) end before do