From 1e2e1c0db5412e1aed3bf47562350c20c69dc1a6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 5 Jul 2018 16:31:03 +0900 Subject: Reorganize components --- app/models/ci/build.rb | 2 +- app/models/ci/job_artifact.rb | 6 ++++++ ...20180705160945_add_compression_to_ci_job_artifacts.rb | 6 ++++++ db/schema.rb | 16 ++++++++++++---- lib/api/entities.rb | 2 +- lib/api/runner.rb | 10 +++++++++- lib/uploaded_file.rb | 7 +++++-- 7 files changed, 40 insertions(+), 9 deletions(-) create mode 100644 db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index bf93a2caf72..90e6081e851 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -534,7 +534,7 @@ module Ci end def artifacts - [options[:artifacts]] + [options[:artifacts].merge(compression: 'zip', artifact_type: 'archive')] end def cache diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 3b952391b7e..03b51c1e8a0 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -25,6 +25,12 @@ module Ci trace: 3 } + enum compression: { + raw: 1, + zip: 2, + gzip: 3 + } + def update_file_store # The file.object_store is set during `uploader.store!` # which happens after object is inserted/updated diff --git a/db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb b/db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb new file mode 100644 index 00000000000..b7eb7554a9a --- /dev/null +++ b/db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb @@ -0,0 +1,6 @@ +# rubocop:disable all +class AddCompressionToCiJobArtifacts < ActiveRecord::Migration + def change + add_column :ci_job_artifacts, :compression, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index c9aaf80f059..008743697c9 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: 20180629191052) do +ActiveRecord::Schema.define(version: 20180705160945) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -337,6 +337,8 @@ ActiveRecord::Schema.define(version: 20180629191052) do add_index "ci_builds", ["commit_id", "status", "type"], name: "index_ci_builds_on_commit_id_and_status_and_type", using: :btree add_index "ci_builds", ["commit_id", "type", "name", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_name_and_ref", using: :btree add_index "ci_builds", ["commit_id", "type", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_ref", using: :btree + add_index "ci_builds", ["id"], name: "index_ci_builds_on_id", where: "(artifacts_file <> ''::text)", using: :btree + add_index "ci_builds", ["id"], name: "tmp_partial_index_ci_builds_on_id_with_legacy_artifacts", where: "(artifacts_file <> ''::text)", using: :btree add_index "ci_builds", ["project_id", "id"], name: "index_ci_builds_on_project_id_and_id", using: :btree add_index "ci_builds", ["protected"], name: "index_ci_builds_on_protected", using: :btree add_index "ci_builds", ["runner_id"], name: "index_ci_builds_on_runner_id", using: :btree @@ -392,6 +394,9 @@ ActiveRecord::Schema.define(version: 20180629191052) do t.datetime_with_timezone "expire_at" t.string "file" t.binary "file_sha256" + t.integer "file_format", limit: 2 + t.integer "file_location", limit: 2, default: 2, null: false + t.integer "compression" end add_index "ci_job_artifacts", ["expire_at", "job_id"], name: "index_ci_job_artifacts_on_expire_at_and_job_id", using: :btree @@ -1093,9 +1098,10 @@ ActiveRecord::Schema.define(version: 20180629191052) do t.datetime "created_at" t.datetime "updated_at" t.string "file" - t.integer "file_store" + t.integer "file_store", null: false end + add_index "lfs_objects", ["file_store"], name: "index_lfs_objects_on_file_store", using: :btree add_index "lfs_objects", ["oid"], name: "index_lfs_objects_on_oid", unique: true, using: :btree create_table "lfs_objects_projects", force: :cascade do |t| @@ -1738,6 +1744,7 @@ ActiveRecord::Schema.define(version: 20180629191052) do end add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path", unique: true, using: :btree + add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path_text_pattern_ops", using: :btree, opclasses: {"path"=>"varchar_pattern_ops"} add_index "redirect_routes", ["source_type", "source_id"], name: "index_redirect_routes_on_source_type_and_source_id", using: :btree create_table "releases", force: :cascade do |t| @@ -1992,11 +1999,12 @@ ActiveRecord::Schema.define(version: 20180629191052) do t.datetime "created_at", null: false t.string "mount_point" t.string "secret" - t.integer "store" + t.integer "store", null: false end add_index "uploads", ["checksum"], name: "index_uploads_on_checksum", using: :btree add_index "uploads", ["model_id", "model_type"], name: "index_uploads_on_model_id_and_model_type", using: :btree + add_index "uploads", ["store"], name: "index_uploads_on_store", using: :btree add_index "uploads", ["uploader", "path"], name: "index_uploads_on_uploader_and_path", using: :btree create_table "user_agent_details", force: :cascade do |t| @@ -2327,7 +2335,7 @@ ActiveRecord::Schema.define(version: 20180629191052) do add_foreign_key "term_agreements", "users", on_delete: :cascade add_foreign_key "timelogs", "issues", name: "fk_timelogs_issues_issue_id", on_delete: :cascade add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade - add_foreign_key "todos", "namespaces", column: "group_id", on_delete: :cascade + add_foreign_key "todos", "namespaces", column: "group_id", name: "fk_a27c483435", on_delete: :cascade add_foreign_key "todos", "notes", name: "fk_91d1f47b13", on_delete: :cascade add_foreign_key "todos", "projects", name: "fk_45054f9c45", on_delete: :cascade add_foreign_key "todos", "users", column: "author_id", name: "fk_ccf0373936", on_delete: :cascade diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 3a6e707fd5b..0a185220e59 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1219,7 +1219,7 @@ module API end class Artifacts < Grape::Entity - expose :name, :untracked, :paths, :when, :expire_in + expose :name, :untracked, :paths, :when, :expire_in, :compression, :artifact_type end class Cache < Grape::Entity diff --git a/lib/api/runner.rb b/lib/api/runner.rb index d0cc0945a5f..9d1a51aa8c1 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -233,15 +233,21 @@ module API requires :id, type: Integer, desc: %q(Job's ID) optional :token, type: String, desc: %q(Job's authentication token) optional :expire_in, type: String, desc: %q(Specify when artifacts should expire) + optional :artifact_type, type: String, desc: %q(The type of artifact), + default: 'archive', values: Ci::JobArtifact.file_types.keys optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse)) optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse)) optional 'file.size', type: Integer, desc: %q(real size of file (generated by Workhorse)) optional 'file.sha256', type: String, desc: %q(sha256 checksum of the file (generated by Workhorse)) + optional 'file.compression', type: String, desc: %q(The compression algorithm of the artifact), + default: 'zip', values: Ci::JobArtifact.compressions.keys optional 'metadata.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'metadata.name', type: String, desc: %q(filename (generated by Workhorse)) optional 'metadata.size', type: Integer, desc: %q(real size of metadata (generated by Workhorse)) optional 'metadata.sha256', type: String, desc: %q(sha256 checksum of metadata (generated by Workhorse)) + optional 'metadata.compression', type: String, desc: %q(The compression algorithm of the metadata), + default: 'gzip', values: Ci::JobArtifact.compressions.keys end post '/:id/artifacts' do not_allowed! unless Gitlab.config.artifacts.enabled @@ -261,10 +267,11 @@ module API expire_in = params['expire_in'] || Gitlab::CurrentSettings.current_application_settings.default_artifacts_expire_in - job.build_job_artifacts_archive( + job.job_artifacts.build( project: job.project, file: artifacts, file_type: :archive, + compression: artifacts.compression, file_sha256: artifacts.sha256, expire_in: expire_in) @@ -273,6 +280,7 @@ module API project: job.project, file: metadata, file_type: :metadata, + compression: metadata.compression, file_sha256: metadata.sha256, expire_in: expire_in) end diff --git a/lib/uploaded_file.rb b/lib/uploaded_file.rb index 5dc85b2baea..3ef52a3c9b8 100644 --- a/lib/uploaded_file.rb +++ b/lib/uploaded_file.rb @@ -16,8 +16,9 @@ class UploadedFile attr_reader :remote_id attr_reader :sha256 + attr_reader :compression - def initialize(path, filename: nil, content_type: "application/octet-stream", sha256: nil, remote_id: nil) + def initialize(path, filename: nil, content_type: "application/octet-stream", sha256: nil, remote_id: nil, compression: nil) raise InvalidPathError, "#{path} file does not exist" unless ::File.exist?(path) @content_type = content_type @@ -26,6 +27,7 @@ class UploadedFile @sha256 = sha256 @remote_id = remote_id @tempfile = File.new(path, 'rb') + @compression = compression end def self.from_params(params, field, upload_path) @@ -45,7 +47,8 @@ class UploadedFile filename: params["#{field}.name"], content_type: params["#{field}.type"] || 'application/octet-stream', sha256: params["#{field}.sha256"], - remote_id: params["#{field}.remote_id"]) + remote_id: params["#{field}.remote_id"], + compression: params["#{field}.compression"]) end def self.allowed_path?(file_path, paths) -- cgit v1.2.1 From beb5990e7e3bfbb308245dc97284aaf9700bd982 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 5 Jul 2018 19:06:54 +0900 Subject: Make compression params at the first level --- lib/api/runner.rb | 12 +++++------- lib/gitlab/ci/trace.rb | 3 ++- lib/uploaded_file.rb | 7 ++----- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 9d1a51aa8c1..1c9257a930e 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -235,19 +235,17 @@ module API optional :expire_in, type: String, desc: %q(Specify when artifacts should expire) optional :artifact_type, type: String, desc: %q(The type of artifact), default: 'archive', values: Ci::JobArtifact.file_types.keys + optional :compression, type: String, desc: %q(The compression algorithm of the artifact), + default: 'zip', values: Ci::JobArtifact.compressions.keys optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse)) optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse)) optional 'file.size', type: Integer, desc: %q(real size of file (generated by Workhorse)) optional 'file.sha256', type: String, desc: %q(sha256 checksum of the file (generated by Workhorse)) - optional 'file.compression', type: String, desc: %q(The compression algorithm of the artifact), - default: 'zip', values: Ci::JobArtifact.compressions.keys optional 'metadata.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'metadata.name', type: String, desc: %q(filename (generated by Workhorse)) optional 'metadata.size', type: Integer, desc: %q(real size of metadata (generated by Workhorse)) optional 'metadata.sha256', type: String, desc: %q(sha256 checksum of metadata (generated by Workhorse)) - optional 'metadata.compression', type: String, desc: %q(The compression algorithm of the metadata), - default: 'gzip', values: Ci::JobArtifact.compressions.keys end post '/:id/artifacts' do not_allowed! unless Gitlab.config.artifacts.enabled @@ -270,8 +268,8 @@ module API job.job_artifacts.build( project: job.project, file: artifacts, - file_type: :archive, - compression: artifacts.compression, + file_type: params['artifact_type'], + compression: params['compression'], file_sha256: artifacts.sha256, expire_in: expire_in) @@ -280,7 +278,7 @@ module API project: job.project, file: metadata, file_type: :metadata, - compression: metadata.compression, + compression: :gzip, file_sha256: metadata.sha256, expire_in: expire_in) end diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb index a52d71225bb..91f3fdaf80d 100644 --- a/lib/gitlab/ci/trace.rb +++ b/lib/gitlab/ci/trace.rb @@ -162,7 +162,8 @@ module Gitlab project: job.project, file_type: :trace, file: stream, - file_sha256: Digest::SHA256.file(path).hexdigest) + file_sha256: Digest::SHA256.file(path).hexdigest, + compression: :raw) end end diff --git a/lib/uploaded_file.rb b/lib/uploaded_file.rb index 3ef52a3c9b8..5dc85b2baea 100644 --- a/lib/uploaded_file.rb +++ b/lib/uploaded_file.rb @@ -16,9 +16,8 @@ class UploadedFile attr_reader :remote_id attr_reader :sha256 - attr_reader :compression - def initialize(path, filename: nil, content_type: "application/octet-stream", sha256: nil, remote_id: nil, compression: nil) + def initialize(path, filename: nil, content_type: "application/octet-stream", sha256: nil, remote_id: nil) raise InvalidPathError, "#{path} file does not exist" unless ::File.exist?(path) @content_type = content_type @@ -27,7 +26,6 @@ class UploadedFile @sha256 = sha256 @remote_id = remote_id @tempfile = File.new(path, 'rb') - @compression = compression end def self.from_params(params, field, upload_path) @@ -47,8 +45,7 @@ class UploadedFile filename: params["#{field}.name"], content_type: params["#{field}.type"] || 'application/octet-stream', sha256: params["#{field}.sha256"], - remote_id: params["#{field}.remote_id"], - compression: params["#{field}.compression"]) + remote_id: params["#{field}.remote_id"]) end def self.allowed_path?(file_path, paths) -- cgit v1.2.1 From d7e0709319ab8fe35a2598a3d484eb89b1885934 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 6 Jul 2018 17:47:18 +0900 Subject: Validate compression. Clean up schema --- app/models/ci/job_artifact.rb | 6 +++--- .../20180705160945_add_compression_to_ci_job_artifacts.rb | 2 +- db/schema.rb | 15 ++++----------- 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 03b51c1e8a0..1bbb2e5eebe 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -10,10 +10,10 @@ module Ci mount_uploader :file, JobArtifactUploader before_save :set_size, if: :file_changed? + validates :compression, presence: true after_save :update_project_statistics_after_save, if: :size_changed? - after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed? - after_save :update_file_store, if: :file_changed? + after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed? scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } @@ -26,7 +26,7 @@ module Ci } enum compression: { - raw: 1, + raw: 1, # Equavalant to none zip: 2, gzip: 3 } diff --git a/db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb b/db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb index b7eb7554a9a..df0832883aa 100644 --- a/db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb +++ b/db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb @@ -1,6 +1,6 @@ # rubocop:disable all class AddCompressionToCiJobArtifacts < ActiveRecord::Migration def change - add_column :ci_job_artifacts, :compression, :integer + add_column :ci_job_artifacts, :compression, :integer, limit: 2 end end diff --git a/db/schema.rb b/db/schema.rb index 008743697c9..0cf5a0625cd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -337,8 +337,6 @@ ActiveRecord::Schema.define(version: 20180705160945) do add_index "ci_builds", ["commit_id", "status", "type"], name: "index_ci_builds_on_commit_id_and_status_and_type", using: :btree add_index "ci_builds", ["commit_id", "type", "name", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_name_and_ref", using: :btree add_index "ci_builds", ["commit_id", "type", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_ref", using: :btree - add_index "ci_builds", ["id"], name: "index_ci_builds_on_id", where: "(artifacts_file <> ''::text)", using: :btree - add_index "ci_builds", ["id"], name: "tmp_partial_index_ci_builds_on_id_with_legacy_artifacts", where: "(artifacts_file <> ''::text)", using: :btree add_index "ci_builds", ["project_id", "id"], name: "index_ci_builds_on_project_id_and_id", using: :btree add_index "ci_builds", ["protected"], name: "index_ci_builds_on_protected", using: :btree add_index "ci_builds", ["runner_id"], name: "index_ci_builds_on_runner_id", using: :btree @@ -394,9 +392,7 @@ ActiveRecord::Schema.define(version: 20180705160945) do t.datetime_with_timezone "expire_at" t.string "file" t.binary "file_sha256" - t.integer "file_format", limit: 2 - t.integer "file_location", limit: 2, default: 2, null: false - t.integer "compression" + t.integer "compression", limit: 2 end add_index "ci_job_artifacts", ["expire_at", "job_id"], name: "index_ci_job_artifacts_on_expire_at_and_job_id", using: :btree @@ -1098,10 +1094,9 @@ ActiveRecord::Schema.define(version: 20180705160945) do t.datetime "created_at" t.datetime "updated_at" t.string "file" - t.integer "file_store", null: false + t.integer "file_store" end - add_index "lfs_objects", ["file_store"], name: "index_lfs_objects_on_file_store", using: :btree add_index "lfs_objects", ["oid"], name: "index_lfs_objects_on_oid", unique: true, using: :btree create_table "lfs_objects_projects", force: :cascade do |t| @@ -1744,7 +1739,6 @@ ActiveRecord::Schema.define(version: 20180705160945) do end add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path", unique: true, using: :btree - add_index "redirect_routes", ["path"], name: "index_redirect_routes_on_path_text_pattern_ops", using: :btree, opclasses: {"path"=>"varchar_pattern_ops"} add_index "redirect_routes", ["source_type", "source_id"], name: "index_redirect_routes_on_source_type_and_source_id", using: :btree create_table "releases", force: :cascade do |t| @@ -1999,12 +1993,11 @@ ActiveRecord::Schema.define(version: 20180705160945) do t.datetime "created_at", null: false t.string "mount_point" t.string "secret" - t.integer "store", null: false + t.integer "store" end add_index "uploads", ["checksum"], name: "index_uploads_on_checksum", using: :btree add_index "uploads", ["model_id", "model_type"], name: "index_uploads_on_model_id_and_model_type", using: :btree - add_index "uploads", ["store"], name: "index_uploads_on_store", using: :btree add_index "uploads", ["uploader", "path"], name: "index_uploads_on_uploader_and_path", using: :btree create_table "user_agent_details", force: :cascade do |t| @@ -2335,7 +2328,7 @@ ActiveRecord::Schema.define(version: 20180705160945) do add_foreign_key "term_agreements", "users", on_delete: :cascade add_foreign_key "timelogs", "issues", name: "fk_timelogs_issues_issue_id", on_delete: :cascade add_foreign_key "timelogs", "merge_requests", name: "fk_timelogs_merge_requests_merge_request_id", on_delete: :cascade - add_foreign_key "todos", "namespaces", column: "group_id", name: "fk_a27c483435", on_delete: :cascade + add_foreign_key "todos", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "todos", "notes", name: "fk_91d1f47b13", on_delete: :cascade add_foreign_key "todos", "projects", name: "fk_45054f9c45", on_delete: :cascade add_foreign_key "todos", "users", column: "author_id", name: "fk_ccf0373936", on_delete: :cascade -- cgit v1.2.1 From f3f25d56a7c627f4bb9d91d19de175273a7a6a81 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 6 Jul 2018 18:02:21 +0900 Subject: Rename metadata to archive_metadata, and compress to file_format --- app/models/ci/job_artifact.rb | 6 +++--- lib/api/runner.rb | 14 ++++++-------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 1bbb2e5eebe..6d76abc53cc 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -21,12 +21,12 @@ module Ci enum file_type: { archive: 1, - metadata: 2, + archive_metadata: 2, trace: 3 } - enum compression: { - raw: 1, # Equavalant to none + enum file_format: { + raw: 1, zip: 2, gzip: 3 } diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 1c9257a930e..6d30ec3d33a 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -235,8 +235,8 @@ module API optional :expire_in, type: String, desc: %q(Specify when artifacts should expire) optional :artifact_type, type: String, desc: %q(The type of artifact), default: 'archive', values: Ci::JobArtifact.file_types.keys - optional :compression, type: String, desc: %q(The compression algorithm of the artifact), - default: 'zip', values: Ci::JobArtifact.compressions.keys + optional :artifact_format, type: String, desc: %q(The compression algorithm of the artifact), + default: 'zip', values: Ci::JobArtifact.file_formats.keys optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse)) optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse)) @@ -260,8 +260,6 @@ module API bad_request!('Missing artifacts file!') unless artifacts file_to_large! unless artifacts.size < max_artifacts_size - bad_request!("Already uploaded") if job.job_artifacts_archive - expire_in = params['expire_in'] || Gitlab::CurrentSettings.current_application_settings.default_artifacts_expire_in @@ -269,16 +267,16 @@ module API project: job.project, file: artifacts, file_type: params['artifact_type'], - compression: params['compression'], + file_format: params['artifact_format'], file_sha256: artifacts.sha256, expire_in: expire_in) if metadata - job.build_job_artifacts_metadata( + job.job_artifacts.build( project: job.project, file: metadata, - file_type: :metadata, - compression: :gzip, + file_type: "#{params['artifact_type']}_metadata", + file_format: :gzip, file_sha256: metadata.sha256, expire_in: expire_in) end -- cgit v1.2.1 From e7e37612487b556320d27f4fe0de32cd4ec20720 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 6 Jul 2018 19:25:10 +0900 Subject: Change column name to artifact_format --- .../20180705160945_add_artifact_format_to_ci_job_artifacts.rb | 6 ++++++ db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb | 6 ------ 2 files changed, 6 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20180705160945_add_artifact_format_to_ci_job_artifacts.rb delete mode 100644 db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb diff --git a/db/migrate/20180705160945_add_artifact_format_to_ci_job_artifacts.rb b/db/migrate/20180705160945_add_artifact_format_to_ci_job_artifacts.rb new file mode 100644 index 00000000000..59b6045289b --- /dev/null +++ b/db/migrate/20180705160945_add_artifact_format_to_ci_job_artifacts.rb @@ -0,0 +1,6 @@ +# rubocop:disable all +class AddArtifactFormatToCiJobArtifacts < ActiveRecord::Migration + def change + add_column :ci_job_artifacts, :artifact_format, :integer, limit: 2 + end +end diff --git a/db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb b/db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb deleted file mode 100644 index df0832883aa..00000000000 --- a/db/migrate/20180705160945_add_compression_to_ci_job_artifacts.rb +++ /dev/null @@ -1,6 +0,0 @@ -# rubocop:disable all -class AddCompressionToCiJobArtifacts < ActiveRecord::Migration - def change - add_column :ci_job_artifacts, :compression, :integer, limit: 2 - end -end -- cgit v1.2.1 From b630c670c707548799c6852e4465ef94fb4a0572 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 6 Jul 2018 19:26:03 +0900 Subject: Allow reports type under artifacts. Allow junit keyword in it. --- lib/gitlab/ci/config/entry/artifact/reports.rb | 30 ++++++++++++++++++++++++++ lib/gitlab/ci/config/entry/artifacts.rb | 6 +++++- 2 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 lib/gitlab/ci/config/entry/artifact/reports.rb diff --git a/lib/gitlab/ci/config/entry/artifact/reports.rb b/lib/gitlab/ci/config/entry/artifact/reports.rb new file mode 100644 index 00000000000..b10b19e8111 --- /dev/null +++ b/lib/gitlab/ci/config/entry/artifact/reports.rb @@ -0,0 +1,30 @@ +module Gitlab + module Ci + class Config + module Entry + module Artifact + ## + # Entry that represents a configuration of job artifact reports. + # + class Reports < Node + include Validatable + include Attributable + + ALLOWED_KEYS = %i[junit].freeze + + attributes ALLOWED_KEYS + + validations do + validates :config, type: Hash + validates :config, allowed_keys: ALLOWED_KEYS + + with_options allow_nil: true do + validates :junit, type: String + end + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/entry/artifacts.rb b/lib/gitlab/ci/config/entry/artifacts.rb index 8275aacee9b..0bd5d1e5440 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -6,10 +6,11 @@ module Gitlab # Entry that represents a configuration of job artifacts. # class Artifacts < Node + include Configurable include Validatable include Attributable - ALLOWED_KEYS = %i[name untracked paths when expire_in].freeze + ALLOWED_KEYS = %i[name untracked paths reports when expire_in].freeze attributes ALLOWED_KEYS @@ -28,6 +29,9 @@ module Gitlab validates :expire_in, duration: true end end + + entry :reports, Entry::Artifact::Reports, + description: 'Report-type artifacts.' end end end -- cgit v1.2.1 From a3ccbe4c3a9b7d3095fe1929dee5fd9c57e168e0 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 6 Jul 2018 20:22:52 +0900 Subject: Fix schema.rb --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 0cf5a0625cd..7f34e8a1dd1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -392,7 +392,7 @@ ActiveRecord::Schema.define(version: 20180705160945) do t.datetime_with_timezone "expire_at" t.string "file" t.binary "file_sha256" - t.integer "compression", limit: 2 + t.integer "file_format", limit: 2 end add_index "ci_job_artifacts", ["expire_at", "job_id"], name: "index_ci_job_artifacts_on_expire_at_and_job_id", using: :btree -- cgit v1.2.1 From 7ec81e7c7d115f77d712892dfc79db72b9f5bc7a Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 6 Jul 2018 20:23:54 +0900 Subject: Artifacts presenter (Halfway) --- app/models/ci/build.rb | 31 ++++++++++++++++++++++++++++++- lib/api/entities.rb | 4 +++- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 90e6081e851..714386ce2d9 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -534,7 +534,36 @@ module Ci end def artifacts - [options[:artifacts].merge(compression: 'zip', artifact_type: 'archive')] + list = [] + list.concat([artifacts_archive]) if options.dig(:artifacts, :paths) + list.concat(artifacts_reports) if options.dig(:artifacts, :reports) + list + end + + def artifacts_archive + { + name: options[:artifacts][:name], + paths: options[:artifacts][:paths], + type: 'archive', + format: 'zip', + expire_in: options[:artifacts][:expire_in] + } + end + + def artifacts_reports + reports = [] + reports << artifacts_junit if options.dig(:artifacts, :reports, :junit) + reports + end + + def artifacts_junit + { + name: 'junit.xml', + paths: [options[:artifacts][:reports][:junit]], + type: 'junit', + format: 'gzip', + expire_in: options.dig(:artifacts, :expire_in) + } end def cache diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 0a185220e59..ccdf6532260 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1219,7 +1219,9 @@ module API end class Artifacts < Grape::Entity - expose :name, :untracked, :paths, :when, :expire_in, :compression, :artifact_type + expose :name, :untracked, :paths, :when, :expire_in + expose :type, as: :artifact_type + expose :format, as: :artifact_format end class Cache < Grape::Entity -- cgit v1.2.1 From c0840224bc8789d35da032c2a0ee48aa9f2232aa Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 11 Jul 2018 15:16:19 +0900 Subject: Add format_restriction validation --- app/models/ci/job_artifact.rb | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 6d76abc53cc..32158d4bb7a 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -10,10 +10,9 @@ module Ci mount_uploader :file, JobArtifactUploader before_save :set_size, if: :file_changed? - validates :compression, presence: true + validate :format_restriction after_save :update_project_statistics_after_save, if: :size_changed? after_save :update_file_store, if: :file_changed? - after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed? scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } @@ -22,7 +21,8 @@ module Ci enum file_type: { archive: 1, archive_metadata: 2, - trace: 3 + trace: 3, + junit: 4 } enum file_format: { @@ -58,6 +58,30 @@ module Ci private + # For the backword compatibility + def raw + super || (trace? && file_format.nil?) + end + + # For the backword compatibility + def zip? + super || (archive? && file_format.nil?) + end + + # For the backword compatibility + def gzip? + super || (archive_metadata? && file_format.nil?) + end + + def format_restriction + return if archive? && zip? + return if archive_metadata? && gzip? + return if trace? && raw? + return if junit? && gzip? + + erros.add(:file_type, "cannot save #{file_type} type as #{file_format} format") + end + def set_size self.size = file.size end -- cgit v1.2.1 From fb69ae8349d58499ad21965c0d1cf95e2b79a8e3 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 11 Jul 2018 15:20:10 +0900 Subject: Check the presence of the file_format --- app/models/ci/job_artifact.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 32158d4bb7a..150077f3be8 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -10,6 +10,7 @@ module Ci mount_uploader :file, JobArtifactUploader before_save :set_size, if: :file_changed? + validates :file_format, presence: true, if: :new_record? validate :format_restriction after_save :update_project_statistics_after_save, if: :size_changed? after_save :update_file_store, if: :file_changed? -- cgit v1.2.1 From eb48369b8311b538f46f59a31f4a6d3f8c9e68e1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 11 Jul 2018 15:21:46 +0900 Subject: Use file_format raw for trace --- lib/gitlab/ci/trace.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb index e63b8ccc7cd..769d998227c 100644 --- a/lib/gitlab/ci/trace.rb +++ b/lib/gitlab/ci/trace.rb @@ -167,9 +167,9 @@ module Gitlab job.create_job_artifacts_trace!( project: job.project, file_type: :trace, + file_format: :raw, file: stream, - file_sha256: Digest::SHA256.file(path).hexdigest, - compression: :raw) + file_sha256: Digest::SHA256.file(path).hexdigest) end end -- cgit v1.2.1 From 583165c0349f40e7be16a8039dbffb4139f94921 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 11 Jul 2018 15:27:21 +0900 Subject: Revert unnecessary change --- app/models/ci/job_artifact.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 150077f3be8..13ee35db3cb 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -14,6 +14,7 @@ module Ci validate :format_restriction after_save :update_project_statistics_after_save, if: :size_changed? after_save :update_file_store, if: :file_changed? + after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed? scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } -- cgit v1.2.1 From 31252fe8d751319c5390f898f66f0af4a8581013 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 11 Jul 2018 19:05:51 +0900 Subject: Temporaly use type Hash for reports --- lib/gitlab/ci/config/entry/artifacts.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/ci/config/entry/artifacts.rb b/lib/gitlab/ci/config/entry/artifacts.rb index 0bd5d1e5440..8cb4cd7647a 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -22,6 +22,7 @@ module Gitlab validates :name, type: String validates :untracked, boolean: true validates :paths, array_of_strings: true + validates :reports, type: Hash validates :when, inclusion: { in: %w[on_success on_failure always], message: 'should be on_success, on_failure ' \ @@ -29,9 +30,9 @@ module Gitlab validates :expire_in, duration: true end end - - entry :reports, Entry::Artifact::Reports, - description: 'Report-type artifacts.' + # TODO: Validate correctly + # entry :reports, Entry::Artifact::Reports, + # description: 'Report-type artifacts.' end end end -- cgit v1.2.1 From 5342f07e100253713dbf50eb303da1977484077f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 12 Jul 2018 13:14:03 +0900 Subject: Fix raw to raw? --- app/models/ci/job_artifact.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 13ee35db3cb..e5449b37c5c 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -61,7 +61,7 @@ module Ci private # For the backword compatibility - def raw + def raw? super || (trace? && file_format.nil?) end -- cgit v1.2.1 From 4098a8f10f92a6efa48080f8925809e251066f9d Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 12 Jul 2018 13:23:55 +0900 Subject: Add job_artifacts_junit relation --- app/models/ci/build.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 671cbac942f..5419fbdcef7 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -25,6 +25,7 @@ module Ci has_one :job_artifacts_archive, -> { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id + has_one :job_artifacts_junit, -> { where(file_type: Ci::JobArtifact.file_types[:junit]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :metadata, class_name: 'Ci::BuildMetadata' has_one :runner_session, class_name: 'Ci::BuildRunnerSession', validate: true, inverse_of: :build -- cgit v1.2.1 From da4ca63f25a27a1268317952061c81a28516653f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 12 Jul 2018 13:29:21 +0900 Subject: Refactor job_artifacts_metadata to job_artifacts_archive_metadata --- app/models/ci/build.rb | 2 +- app/models/concerns/artifact_migratable.rb | 6 +++--- spec/requests/api/runner_spec.rb | 2 +- spec/services/ci/retry_build_service_spec.rb | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 5419fbdcef7..8942fbe17f2 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -23,7 +23,7 @@ module Ci has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy, inverse_of: :job # rubocop:disable Cop/ActiveRecordDependent has_one :job_artifacts_archive, -> { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id - has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id + has_one :job_artifacts_archive_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :job_artifacts_junit, -> { where(file_type: Ci::JobArtifact.file_types[:junit]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index ff52ca64459..5ce7d693249 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -7,7 +7,7 @@ module ArtifactMigratable end def artifacts_metadata - job_artifacts_metadata&.file || legacy_artifacts_metadata + job_artifacts_archive_metadata&.file || legacy_artifacts_metadata end def artifacts? @@ -31,8 +31,8 @@ module ArtifactMigratable end def remove_artifacts_metadata! - if job_artifacts_metadata - job_artifacts_metadata.destroy + if job_artifacts_archive_metadata + job_artifacts_archive_metadata.destroy else remove_legacy_artifacts_metadata! end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index d57993ab454..401ba7959bc 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -1380,7 +1380,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do let(:stored_metadata_file) { job.reload.artifacts_metadata.file } let(:stored_artifacts_size) { job.reload.artifacts_size } let(:stored_artifacts_sha256) { job.reload.job_artifacts_archive.file_sha256 } - let(:stored_metadata_sha256) { job.reload.job_artifacts_metadata.file_sha256 } + let(:stored_metadata_sha256) { job.reload.job_artifacts_archive_metadata.file_sha256 } before do post(api("/jobs/#{job.id}/artifacts"), post_data, headers_with_token) diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index ecf5d849d3f..41899740081 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -24,7 +24,7 @@ describe Ci::RetryBuildService do artifacts_file artifacts_metadata artifacts_size created_at updated_at started_at finished_at queued_at erased_by erased_at auto_canceled_by job_artifacts job_artifacts_archive - job_artifacts_metadata job_artifacts_trace].freeze + job_artifacts_archive_metadata job_artifacts_trace].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections -- cgit v1.2.1 From 57d6f21821c8ad934874c1aac3f627335c64c80d Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 12 Jul 2018 13:32:35 +0900 Subject: Use the correct type name --- app/models/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8942fbe17f2..dc7a9cdaa87 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -23,7 +23,7 @@ module Ci has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy, inverse_of: :job # rubocop:disable Cop/ActiveRecordDependent has_one :job_artifacts_archive, -> { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id - has_one :job_artifacts_archive_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id + has_one :job_artifacts_archive_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:archive_metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :job_artifacts_junit, -> { where(file_type: Ci::JobArtifact.file_types[:junit]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id -- cgit v1.2.1 From a531bd7487955143489d286a0fb2e5d0984acc52 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 12 Jul 2018 17:40:35 +0900 Subject: Fix errors typo --- app/models/ci/job_artifact.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index e5449b37c5c..b6e22ed0246 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -81,7 +81,7 @@ module Ci return if trace? && raw? return if junit? && gzip? - erros.add(:file_type, "cannot save #{file_type} type as #{file_format} format") + errors.add(:file_type, "cannot save #{file_type} type as #{file_format} format") end def set_size -- cgit v1.2.1 From da6e141e7b9ff28cc1fb25ab42c979b0dee46277 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 12 Jul 2018 17:41:35 +0900 Subject: Add gzip XML parser --- app/models/ci/build.rb | 6 +++++ lib/gitlab/ci/build/artifacts/junit.rb | 36 +++++++++++++++++++++++++ lib/gitlab/ci/build/artifacts/xml.rb | 49 ++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+) create mode 100644 lib/gitlab/ci/build/artifacts/junit.rb create mode 100644 lib/gitlab/ci/build/artifacts/xml.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index dc7a9cdaa87..33e12a140ff 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -448,6 +448,12 @@ module Ci end end + def artifacts_junit_hash + job_artifacts_junit.open do |stream| + Gitlab::Ci::Build::Artifacts::Junit.new(stream).to_hash + end + end + def erase_artifacts! remove_artifacts_file! remove_artifacts_metadata! diff --git a/lib/gitlab/ci/build/artifacts/junit.rb b/lib/gitlab/ci/build/artifacts/junit.rb new file mode 100644 index 00000000000..da3b8cea0ee --- /dev/null +++ b/lib/gitlab/ci/build/artifacts/junit.rb @@ -0,0 +1,36 @@ +module Gitlab + module Ci + module Build + module Artifacts + class Junit < XML + attr_reader :junit_artifact + + def to_hash + Gitlab::Ci::Build::Artifacts::XML.new(stream).each_hash do |file_name, hash| + hash['testsuite'].each do |testsuite| + # testsuite.name + # testsuite.tests + # testsuite.skipped + # testsuite.failures + testsuite['testcase'].each do |testcase| + # testcase.classname + # testcase.name + # testcase.file + # testcase.time + if testcase.key?('failure') + testcase['failure'] # stack_trace + testcase['system-out'] + elsif testcase.key?('skipped') || testcase.key?('skipped') + # TODO: + else + # passed + end + end + end + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/build/artifacts/xml.rb b/lib/gitlab/ci/build/artifacts/xml.rb new file mode 100644 index 00000000000..5d3a0c62d6f --- /dev/null +++ b/lib/gitlab/ci/build/artifacts/xml.rb @@ -0,0 +1,49 @@ +require 'zlib' +require 'json' + +module Gitlab + module Ci + module Build + module Artifacts + class XML + attr_reader :stream + + def initialize(stream) + @stream = stream + end + + def each_hash + each_raw do |file_name, xml| + yield file_name, Hash.from_xml(xml) + end + rescue => e + raise "Failed to parse xml #{e.message}" + end + + def each_raw + stream.seek(0) + + until stream.eof? + gzip(stream) do |gz| + yield gz.orig_name, gz.read + unused = gz.unused&.length.to_i + stream.seek(-unused, IO::SEEK_CUR) # IO has already reached EOF, so rewind the pos to the unsued file + end + end + end + + def gzip(stream, &block) + raise Metadata::InvalidStreamError, "Invalid stream" unless stream + + gz = Zlib::GzipReader.new(stream) + yield(gz) + rescue Zlib::Error => e + raise Metadata::InvalidStreamError, e.message + ensure + gz&.finish + end + end + end + end + end +end -- cgit v1.2.1 From e79808425eb63c322a997e71d606d97b85e42048 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 12 Jul 2018 18:16:52 +0900 Subject: Remove unnecessary change --- app/models/ci/job_artifact.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index b6e22ed0246..d95f2602ef9 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -13,9 +13,10 @@ module Ci validates :file_format, presence: true, if: :new_record? validate :format_restriction after_save :update_project_statistics_after_save, if: :size_changed? - after_save :update_file_store, if: :file_changed? after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed? + after_save :update_file_store, if: :file_changed? + scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } delegate :exists?, :open, to: :file -- cgit v1.2.1 From 1650249214768c23f6f46ec62c0c54448017eeb5 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 13 Jul 2018 13:25:52 +0900 Subject: Simplified file_type relations --- app/models/ci/build.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index dc7a9cdaa87..a5116690378 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -22,10 +22,10 @@ module Ci has_many :trace_chunks, class_name: 'Ci::BuildTraceChunk', foreign_key: :build_id has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy, inverse_of: :job # rubocop:disable Cop/ActiveRecordDependent - has_one :job_artifacts_archive, -> { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id - has_one :job_artifacts_archive_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:archive_metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id - has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id - has_one :job_artifacts_junit, -> { where(file_type: Ci::JobArtifact.file_types[:junit]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id + + Ci::JobArtifact.file_types.each do |key, value| + has_one :"job_artifacts_#{key}", -> { where(file_type: value) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id + end has_one :metadata, class_name: 'Ci::BuildMetadata' has_one :runner_session, class_name: 'Ci::BuildRunnerSession', validate: true, inverse_of: :build -- cgit v1.2.1 From a79299fdbb0ed74000ca37cff8fef8268cd29b13 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 13 Jul 2018 13:55:02 +0900 Subject: Cleanup API::Entities::JobRequest::Artifacts --- app/models/ci/build.rb | 33 --------------------------------- lib/api/entities.rb | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index a5116690378..2479e1142f3 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -540,39 +540,6 @@ module Ci Gitlab::Ci::Build::Image.from_services(self) end - def artifacts - list = [] - list.concat([artifacts_archive]) if options.dig(:artifacts, :paths) - list.concat(artifacts_reports) if options.dig(:artifacts, :reports) - list - end - - def artifacts_archive - { - name: options[:artifacts][:name], - paths: options[:artifacts][:paths], - type: 'archive', - format: 'zip', - expire_in: options[:artifacts][:expire_in] - } - end - - def artifacts_reports - reports = [] - reports << artifacts_junit if options.dig(:artifacts, :reports, :junit) - reports - end - - def artifacts_junit - { - name: 'junit.xml', - paths: [options[:artifacts][:reports][:junit]], - type: 'junit', - format: 'gzip', - expire_in: options.dig(:artifacts, :expire_in) - } - end - def cache cache = options[:cache] diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 1d6e6da09e1..06cb3d58f22 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1230,6 +1230,27 @@ module API expose :name, :untracked, :paths, :when, :expire_in expose :type, as: :artifact_type expose :format, as: :artifact_format + + def self.archive(artifacts) + { + name: artifacts.dig(:name), + paths: artifacts.dig(:paths), + untracked: artifacts.dig(:untracked), + when: artifacts.dig(:when), + expire_in: artifacts.dig(:expire_in), + type: 'archive', + format: 'zip', + } + end + + def self.junit(artifacts) + { + name: 'junit.xml', + paths: artifacts.dig(:reports, :junit), + type: 'junit', + format: 'gzip', + } + end end class Cache < Grape::Entity @@ -1266,7 +1287,18 @@ module API expose :steps, using: Step expose :image, using: Image expose :services, using: Service - expose :artifacts, using: Artifacts + + expose :artifacts, using: Artifacts do |model| + list = [] + + model.options.dig(:artifacts).try do |artifacts| + list << Artifacts.archive(artifacts) if artifacts.dig(:paths) + list << Artifacts.junit(artifacts) if artifacts.dig(:reports, :junit) + end + + list + end + expose :cache, using: Cache expose :credentials, using: Credentials expose :dependencies, using: Dependency -- cgit v1.2.1 From aac284613b9db43e3021198dc5b43b81806f1bce Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 13 Jul 2018 14:40:20 +0900 Subject: Generalized by DEFAULT_FILE_FORMAT --- app/models/ci/job_artifact.rb | 35 ++++++++++++++++++++++++++--------- lib/api/entities.rb | 8 ++++---- lib/api/runner.rb | 4 ---- lib/gitlab/ci/trace.rb | 1 - 4 files changed, 30 insertions(+), 18 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index d95f2602ef9..90c4bae31ae 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -9,9 +9,9 @@ module Ci mount_uploader :file, JobArtifactUploader + before_validation :set_file_format, if: :file_changed? + validate :file_format_check before_save :set_size, if: :file_changed? - validates :file_format, presence: true, if: :new_record? - validate :format_restriction after_save :update_project_statistics_after_save, if: :size_changed? after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed? @@ -34,6 +34,19 @@ module Ci gzip: 3 } + DEFAULT_FILE_FORMAT = { + archive: :zip, + archive_metadata: :gzip, + trace: :raw, + junit: :gzip + } + + ACCEPT_FILE_FORMAT_ETX = { + raw: %w[.log], + zip: %w[.zip], + gzip: %w[.gz] + } + def update_file_store # The file.object_store is set during `uploader.store!` # which happens after object is inserted/updated @@ -76,19 +89,23 @@ module Ci super || (archive_metadata? && file_format.nil?) end - def format_restriction - return if archive? && zip? - return if archive_metadata? && gzip? - return if trace? && raw? - return if junit? && gzip? - - errors.add(:file_type, "cannot save #{file_type} type as #{file_format} format") + def set_file_format + self.file_format = DEFAULT_FILE_FORMAT[self.file_type.to_sym] end def set_size self.size = file.size end + # Shallow check + def file_format_check + extname = File.extname(self.file.filename) + + unless ACCEPT_FILE_FORMAT_ETX[self.file_format.to_sym].include?(extname) + errors.add(:file_type, "cannot save #{file_type} type as #{file_format} format") + end + end + def update_project_statistics_after_save update_project_statistics(size.to_i - size_was.to_i) end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 06cb3d58f22..67e4353bbc6 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1238,8 +1238,8 @@ module API untracked: artifacts.dig(:untracked), when: artifacts.dig(:when), expire_in: artifacts.dig(:expire_in), - type: 'archive', - format: 'zip', + type: :archive, # Ci::JobArtifact.file_types + format: ::Ci::JobArtifact::DEFAULT_FILE_FORMAT[:archive] } end @@ -1247,8 +1247,8 @@ module API { name: 'junit.xml', paths: artifacts.dig(:reports, :junit), - type: 'junit', - format: 'gzip', + type: :junit, # Ci::JobArtifact.file_types + format: ::Ci::JobArtifact::DEFAULT_FILE_FORMAT[:junit] } end end diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 6d30ec3d33a..39400d77eda 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -235,8 +235,6 @@ module API optional :expire_in, type: String, desc: %q(Specify when artifacts should expire) optional :artifact_type, type: String, desc: %q(The type of artifact), default: 'archive', values: Ci::JobArtifact.file_types.keys - optional :artifact_format, type: String, desc: %q(The compression algorithm of the artifact), - default: 'zip', values: Ci::JobArtifact.file_formats.keys optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse)) optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse)) @@ -267,7 +265,6 @@ module API project: job.project, file: artifacts, file_type: params['artifact_type'], - file_format: params['artifact_format'], file_sha256: artifacts.sha256, expire_in: expire_in) @@ -276,7 +273,6 @@ module API project: job.project, file: metadata, file_type: "#{params['artifact_type']}_metadata", - file_format: :gzip, file_sha256: metadata.sha256, expire_in: expire_in) end diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb index 769d998227c..ee54b893598 100644 --- a/lib/gitlab/ci/trace.rb +++ b/lib/gitlab/ci/trace.rb @@ -167,7 +167,6 @@ module Gitlab job.create_job_artifacts_trace!( project: job.project, file_type: :trace, - file_format: :raw, file: stream, file_sha256: Digest::SHA256.file(path).hexdigest) end -- cgit v1.2.1 From bc96346be6990b75da9a36055814b24b5b805707 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 13 Jul 2018 16:43:02 +0900 Subject: Fix Config::Entry::Artifacts --- lib/api/entities.rb | 2 +- lib/gitlab/ci/config/entry/artifact/reports.rb | 30 ----------------------- lib/gitlab/ci/config/entry/artifacts.rb | 13 +++++++--- lib/gitlab/ci/config/entry/reports.rb | 33 ++++++++++++++++++++++++++ lib/gitlab/ci/config/entry/validators.rb | 14 +++++++++++ 5 files changed, 58 insertions(+), 34 deletions(-) delete mode 100644 lib/gitlab/ci/config/entry/artifact/reports.rb create mode 100644 lib/gitlab/ci/config/entry/reports.rb diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 67e4353bbc6..c2e201867c8 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1242,7 +1242,7 @@ module API format: ::Ci::JobArtifact::DEFAULT_FILE_FORMAT[:archive] } end - + def self.junit(artifacts) { name: 'junit.xml', diff --git a/lib/gitlab/ci/config/entry/artifact/reports.rb b/lib/gitlab/ci/config/entry/artifact/reports.rb deleted file mode 100644 index b10b19e8111..00000000000 --- a/lib/gitlab/ci/config/entry/artifact/reports.rb +++ /dev/null @@ -1,30 +0,0 @@ -module Gitlab - module Ci - class Config - module Entry - module Artifact - ## - # Entry that represents a configuration of job artifact reports. - # - class Reports < Node - include Validatable - include Attributable - - ALLOWED_KEYS = %i[junit].freeze - - attributes ALLOWED_KEYS - - validations do - validates :config, type: Hash - validates :config, allowed_keys: ALLOWED_KEYS - - with_options allow_nil: true do - validates :junit, type: String - end - end - end - end - end - end - end -end diff --git a/lib/gitlab/ci/config/entry/artifacts.rb b/lib/gitlab/ci/config/entry/artifacts.rb index 8cb4cd7647a..ca80dbd85a2 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -14,6 +14,9 @@ module Gitlab attributes ALLOWED_KEYS + entry :reports, Entry::Reports, + description: 'Report-type artifacts.' + validations do validates :config, type: Hash validates :config, allowed_keys: ALLOWED_KEYS @@ -30,9 +33,13 @@ module Gitlab validates :expire_in, duration: true end end - # TODO: Validate correctly - # entry :reports, Entry::Artifact::Reports, - # description: 'Report-type artifacts.' + + helpers :reports + + def value + @config[:reports] = reports_value + @config + end end end end diff --git a/lib/gitlab/ci/config/entry/reports.rb b/lib/gitlab/ci/config/entry/reports.rb new file mode 100644 index 00000000000..76e5542be7e --- /dev/null +++ b/lib/gitlab/ci/config/entry/reports.rb @@ -0,0 +1,33 @@ +module Gitlab + module Ci + class Config + module Entry + ## + # Entry that represents a configuration of job artifacts. + # + class Reports < Node + include Validatable + include Attributable + + ALLOWED_KEYS = %i[junit].freeze + + attributes ALLOWED_KEYS + + validations do + validates :config, type: Hash + validates :config, allowed_keys: ALLOWED_KEYS + + with_options allow_nil: true do + validates :junit, array_of_strings_or_string: true + end + end + + def value + @config[:junit] = Array(@config[:junit]) if @config[:junit].is_a?(String) + @config + end + end + end + end + end +end diff --git a/lib/gitlab/ci/config/entry/validators.rb b/lib/gitlab/ci/config/entry/validators.rb index 55658900628..c6469aff5cb 100644 --- a/lib/gitlab/ci/config/entry/validators.rb +++ b/lib/gitlab/ci/config/entry/validators.rb @@ -130,6 +130,20 @@ module Gitlab end end + class ArrayOfStringsOrStringValidator < RegexpValidator + def validate_each(record, attribute, value) + unless validate_array_of_strings_or_string(value) + record.errors.add(attribute, 'should be an array of strings or string') + end + end + + private + + def validate_array_of_strings_or_string(values) + validate_array_of_strings(values) || validate_string(values) + end + end + class TypeValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) type = options[:with] -- cgit v1.2.1 From 4c87e5b388fb098fb6da71e17a47fa204033e4ac Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 13 Jul 2018 17:33:07 +0900 Subject: Fix static analysis --- app/models/ci/job_artifact.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 90c4bae31ae..193aa87af1d 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -39,13 +39,13 @@ module Ci archive_metadata: :gzip, trace: :raw, junit: :gzip - } + }.freeze ACCEPT_FILE_FORMAT_ETX = { raw: %w[.log], zip: %w[.zip], gzip: %w[.gz] - } + }.freeze def update_file_store # The file.object_store is set during `uploader.store!` -- cgit v1.2.1 From cc81c34acf23323257d190c23030d0a89265bccc Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 14:35:12 +0900 Subject: Add changelog --- changelogs/unreleased/artifact-format-v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/artifact-format-v2.yml diff --git a/changelogs/unreleased/artifact-format-v2.yml b/changelogs/unreleased/artifact-format-v2.yml new file mode 100644 index 00000000000..e264e0a9fa1 --- /dev/null +++ b/changelogs/unreleased/artifact-format-v2.yml @@ -0,0 +1,5 @@ +--- +title: Extend gitlab-ci.yml to request junit.xml test reports +merge_request: 20390 +author: +type: added -- cgit v1.2.1 From e5299526138be90d65cf13368134e734b46f7597 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 14:59:09 +0900 Subject: Support deleting junit artifact. Make wording explicit --- app/models/ci/build.rb | 5 +++-- app/models/concerns/artifact_migratable.rb | 26 +++++--------------------- lib/api/entities.rb | 4 ++-- lib/api/job_artifacts.rb | 8 ++++---- lib/gitlab/workhorse.rb | 2 +- 5 files changed, 15 insertions(+), 30 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 2efb421fff6..ac73aad590a 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -449,8 +449,9 @@ module Ci end def erase_artifacts! - remove_artifacts_file! - remove_artifacts_metadata! + remove_legacy_artifacts_file! if legacy_artifacts_file + remove_legacy_artifacts_metadata! if legacy_artifacts_metadata + job_artifacts.destroy_all save end diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 5ce7d693249..77457b0699e 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -2,42 +2,26 @@ # Ci::Artifact model # Meant to be prepended so the interface can stay the same module ArtifactMigratable - def artifacts_file + def artifacts_archive_file job_artifacts_archive&.file || legacy_artifacts_file end - def artifacts_metadata + def artifacts_archive_metadata job_artifacts_archive_metadata&.file || legacy_artifacts_metadata end - def artifacts? - !artifacts_expired? && artifacts_file.exists? + def artifacts_archive? + !artifacts_expired? && artifacts_archive_file.exists? end def artifacts_metadata? - artifacts? && artifacts_metadata.exists? + artifacts_archive? && artifacts_archive_metadata.exists? end def artifacts_file_changed? job_artifacts_archive&.file_changed? || attribute_changed?(:artifacts_file) end - def remove_artifacts_file! - if job_artifacts_archive - job_artifacts_archive.destroy - else - remove_legacy_artifacts_file! - end - end - - def remove_artifacts_metadata! - if job_artifacts_archive_metadata - job_artifacts_archive_metadata.destroy - else - remove_legacy_artifacts_metadata! - end - end - def artifacts_size read_attribute(:artifacts_size).to_i + job_artifacts.sum(:size).to_i end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index c2e201867c8..c3dbb1e2186 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1072,7 +1072,7 @@ module API end class Job < JobBasic - expose :artifacts_file, using: JobArtifactFile, if: -> (job, opts) { job.artifacts? } + expose :artifacts_file, using: JobArtifactFile, if: -> (job, opts) { job.artifacts_archive? } expose :runner, with: Runner expose :artifacts_expire_at end @@ -1263,7 +1263,7 @@ module API class Dependency < Grape::Entity expose :id, :name, :token - expose :artifacts_file, using: JobArtifactFile, if: ->(job, _) { job.artifacts? } + expose :artifacts_file, using: JobArtifactFile, if: ->(job, _) { job.artifacts_archive? } end class Response < Grape::Entity diff --git a/lib/api/job_artifacts.rb b/lib/api/job_artifacts.rb index 32379d7c8ab..38cf511eb36 100644 --- a/lib/api/job_artifacts.rb +++ b/lib/api/job_artifacts.rb @@ -28,7 +28,7 @@ module API builds = user_project.latest_successful_builds_for(params[:ref_name]) latest_build = builds.find_by!(name: params[:job]) - present_carrierwave_file!(latest_build.artifacts_file) + present_carrierwave_file!(latest_build.artifacts_archive_file) end desc 'Download the artifacts archive from a job' do @@ -43,7 +43,7 @@ module API build = find_build!(params[:job_id]) - present_carrierwave_file!(build.artifacts_file) + present_carrierwave_file!(build.artifacts_archive_file) end desc 'Download a specific file from artifacts archive' do @@ -57,7 +57,7 @@ module API authorize_read_builds! build = find_build!(params[:job_id]) - not_found! unless build.artifacts? + not_found! unless build.artifacts_archive? path = Gitlab::Ci::Build::Artifacts::Path .new(params[:artifact_path]) @@ -77,7 +77,7 @@ module API build = find_build!(params[:job_id]) authorize!(:update_build, build) - break not_found!(build) unless build.artifacts? + break not_found!(build) unless build.artifacts_archive? build.keep_artifacts! diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index a9629a92a50..425d92217a6 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -112,7 +112,7 @@ module Gitlab end def send_artifacts_entry(build, entry) - file = build.artifacts_file + file = build.artifacts_archive_file archive = file.file_storage? ? file.path : file.url params = { -- cgit v1.2.1 From f3dc7a2e02901c79a9e572514a1b731c680e43cc Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 15:47:51 +0900 Subject: Use presenter for presenting artifacts hash to runner --- app/presenters/ci/build_presenter.rb | 27 +++++++++++++++++++++++++++ lib/api/entities.rb | 34 +--------------------------------- lib/api/runner.rb | 2 +- 3 files changed, 29 insertions(+), 34 deletions(-) diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index e0aaa5cb736..690380cbfaf 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -33,6 +33,33 @@ module Ci "#{subject.name} - #{detailed_status.status_tooltip}" end + def config_artifacts + list = [] + + options.dig(:artifacts).try do |artifacts| + list << config_artifacts_reports(artifacts.delete(:reports)) if artifacts.dig(:reports) + list << config_artifacts_archive(artifacts) if artifacts.dig(:paths) + end + + list.flatten + end + + def config_artifacts_archive(artifacts) + artifacts.merge(type: :archive, format: :zip) + end + + def config_artifacts_reports(reports) + list = [] + + list << config_artifacts_reports_junit(reports.dig(:junit)) if reports.dig(:junit) + + list + end + + def config_artifacts_reports_junit(junit) + { paths: junit, type: :junit, format: :gzip } + end + private def tooltip_for_badge diff --git a/lib/api/entities.rb b/lib/api/entities.rb index c3dbb1e2186..b883f1533fc 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1230,27 +1230,6 @@ module API expose :name, :untracked, :paths, :when, :expire_in expose :type, as: :artifact_type expose :format, as: :artifact_format - - def self.archive(artifacts) - { - name: artifacts.dig(:name), - paths: artifacts.dig(:paths), - untracked: artifacts.dig(:untracked), - when: artifacts.dig(:when), - expire_in: artifacts.dig(:expire_in), - type: :archive, # Ci::JobArtifact.file_types - format: ::Ci::JobArtifact::DEFAULT_FILE_FORMAT[:archive] - } - end - - def self.junit(artifacts) - { - name: 'junit.xml', - paths: artifacts.dig(:reports, :junit), - type: :junit, # Ci::JobArtifact.file_types - format: ::Ci::JobArtifact::DEFAULT_FILE_FORMAT[:junit] - } - end end class Cache < Grape::Entity @@ -1287,18 +1266,7 @@ module API expose :steps, using: Step expose :image, using: Image expose :services, using: Service - - expose :artifacts, using: Artifacts do |model| - list = [] - - model.options.dig(:artifacts).try do |artifacts| - list << Artifacts.archive(artifacts) if artifacts.dig(:paths) - list << Artifacts.junit(artifacts) if artifacts.dig(:reports, :junit) - end - - list - end - + expose :config_artifacts, as: :artifacts, using: Artifacts expose :cache, using: Cache expose :credentials, using: Credentials expose :dependencies, using: Dependency diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 39400d77eda..abb351c40a1 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -110,7 +110,7 @@ module API if result.build Gitlab::Metrics.add_event(:build_found, project: result.build.project.full_path) - present result.build, with: Entities::JobRequest::Response + present result.build.present, with: Entities::JobRequest::Response else Gitlab::Metrics.add_event(:build_not_found) header 'X-GitLab-Last-Update', new_update -- cgit v1.2.1 From b0ffa42f6410be4718e7a36cb21f7b585421750e Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 15:50:42 +0900 Subject: Set file_format at callers --- app/models/ci/job_artifact.rb | 44 +------------------------------------------ lib/api/runner.rb | 2 ++ lib/gitlab/ci/trace.rb | 1 + 3 files changed, 4 insertions(+), 43 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 193aa87af1d..2c9f7e98483 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -9,8 +9,7 @@ module Ci mount_uploader :file, JobArtifactUploader - before_validation :set_file_format, if: :file_changed? - validate :file_format_check + validates :file_format, presence: true, on: :create before_save :set_size, if: :file_changed? after_save :update_project_statistics_after_save, if: :size_changed? after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed? @@ -34,19 +33,6 @@ module Ci gzip: 3 } - DEFAULT_FILE_FORMAT = { - archive: :zip, - archive_metadata: :gzip, - trace: :raw, - junit: :gzip - }.freeze - - ACCEPT_FILE_FORMAT_ETX = { - raw: %w[.log], - zip: %w[.zip], - gzip: %w[.gz] - }.freeze - def update_file_store # The file.object_store is set during `uploader.store!` # which happens after object is inserted/updated @@ -74,38 +60,10 @@ module Ci private - # For the backword compatibility - def raw? - super || (trace? && file_format.nil?) - end - - # For the backword compatibility - def zip? - super || (archive? && file_format.nil?) - end - - # For the backword compatibility - def gzip? - super || (archive_metadata? && file_format.nil?) - end - - def set_file_format - self.file_format = DEFAULT_FILE_FORMAT[self.file_type.to_sym] - end - def set_size self.size = file.size end - # Shallow check - def file_format_check - extname = File.extname(self.file.filename) - - unless ACCEPT_FILE_FORMAT_ETX[self.file_format.to_sym].include?(extname) - errors.add(:file_type, "cannot save #{file_type} type as #{file_format} format") - end - end - def update_project_statistics_after_save update_project_statistics(size.to_i - size_was.to_i) end diff --git a/lib/api/runner.rb b/lib/api/runner.rb index abb351c40a1..a549e9c7480 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -265,6 +265,7 @@ module API project: job.project, file: artifacts, file_type: params['artifact_type'], + file_format: :zip, file_sha256: artifacts.sha256, expire_in: expire_in) @@ -273,6 +274,7 @@ module API project: job.project, file: metadata, file_type: "#{params['artifact_type']}_metadata", + file_format: :gzip, file_sha256: metadata.sha256, expire_in: expire_in) end diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb index ee54b893598..769d998227c 100644 --- a/lib/gitlab/ci/trace.rb +++ b/lib/gitlab/ci/trace.rb @@ -167,6 +167,7 @@ module Gitlab job.create_job_artifacts_trace!( project: job.project, file_type: :trace, + file_format: :raw, file: stream, file_sha256: Digest::SHA256.file(path).hexdigest) end -- cgit v1.2.1 From e295e8cbdee065ee3af6dd82f512729554237cad Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 16:03:26 +0900 Subject: Dry up the converion in Entry::Reports --- lib/gitlab/ci/config/entry/reports.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/gitlab/ci/config/entry/reports.rb b/lib/gitlab/ci/config/entry/reports.rb index 76e5542be7e..5963f3eb90c 100644 --- a/lib/gitlab/ci/config/entry/reports.rb +++ b/lib/gitlab/ci/config/entry/reports.rb @@ -23,8 +23,7 @@ module Gitlab end def value - @config[:junit] = Array(@config[:junit]) if @config[:junit].is_a?(String) - @config + @config.transform_values { |v| Array(v) } end end end -- cgit v1.2.1 From f511933b5f618fc47d1512554878913922dfba61 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 17:09:57 +0900 Subject: Revert artifacts_archive_file refactoring --- app/models/concerns/artifact_migratable.rb | 28 ++++++++++++++++++++++------ spec/models/ci/build_spec.rb | 4 ++-- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 77457b0699e..ff52ca64459 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -2,26 +2,42 @@ # Ci::Artifact model # Meant to be prepended so the interface can stay the same module ArtifactMigratable - def artifacts_archive_file + def artifacts_file job_artifacts_archive&.file || legacy_artifacts_file end - def artifacts_archive_metadata - job_artifacts_archive_metadata&.file || legacy_artifacts_metadata + def artifacts_metadata + job_artifacts_metadata&.file || legacy_artifacts_metadata end - def artifacts_archive? - !artifacts_expired? && artifacts_archive_file.exists? + def artifacts? + !artifacts_expired? && artifacts_file.exists? end def artifacts_metadata? - artifacts_archive? && artifacts_archive_metadata.exists? + artifacts? && artifacts_metadata.exists? end def artifacts_file_changed? job_artifacts_archive&.file_changed? || attribute_changed?(:artifacts_file) end + def remove_artifacts_file! + if job_artifacts_archive + job_artifacts_archive.destroy + else + remove_legacy_artifacts_file! + end + end + + def remove_artifacts_metadata! + if job_artifacts_metadata + job_artifacts_metadata.destroy + else + remove_legacy_artifacts_metadata! + end + end + def artifacts_size read_attribute(:artifacts_size).to_i + job_artifacts.sum(:size).to_i end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index ee923374480..7c3ddab1751 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -861,7 +861,7 @@ describe Ci::Build do let!(:build) { create(:ci_build, :success, :artifacts) } before do - build.remove_artifacts_metadata! + build.job_artifacts_metadata.destroy end describe '#erase' do @@ -930,7 +930,7 @@ describe Ci::Build do let!(:build) { create(:ci_build, :success, :legacy_artifacts) } before do - build.remove_artifacts_metadata! + build.remove_legacy_artifacts_metadata! end describe '#erase' do -- cgit v1.2.1 From 3cd0513e254db15141cd748f6209179f462974f2 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 17:12:52 +0900 Subject: Rename migration file properly --- .../20180705160945_add_artifact_format_to_ci_job_artifacts.rb | 6 ------ db/migrate/20180705160945_add_file_format_to_ci_job_artifacts.rb | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) delete mode 100644 db/migrate/20180705160945_add_artifact_format_to_ci_job_artifacts.rb create mode 100644 db/migrate/20180705160945_add_file_format_to_ci_job_artifacts.rb diff --git a/db/migrate/20180705160945_add_artifact_format_to_ci_job_artifacts.rb b/db/migrate/20180705160945_add_artifact_format_to_ci_job_artifacts.rb deleted file mode 100644 index 59b6045289b..00000000000 --- a/db/migrate/20180705160945_add_artifact_format_to_ci_job_artifacts.rb +++ /dev/null @@ -1,6 +0,0 @@ -# rubocop:disable all -class AddArtifactFormatToCiJobArtifacts < ActiveRecord::Migration - def change - add_column :ci_job_artifacts, :artifact_format, :integer, limit: 2 - end -end diff --git a/db/migrate/20180705160945_add_file_format_to_ci_job_artifacts.rb b/db/migrate/20180705160945_add_file_format_to_ci_job_artifacts.rb new file mode 100644 index 00000000000..9867506a925 --- /dev/null +++ b/db/migrate/20180705160945_add_file_format_to_ci_job_artifacts.rb @@ -0,0 +1,6 @@ +# rubocop:disable all +class AddFileFormatToCiJobArtifacts < ActiveRecord::Migration + def change + add_column :ci_job_artifacts, :file_format, :integer, limit: 2 + end +end -- cgit v1.2.1 From 882faeab57ab39d18f72abd9b65d286db92e1011 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 17:20:28 +0900 Subject: Add file_format to factory --- app/models/concerns/artifact_migratable.rb | 2 +- spec/factories/ci/builds.rb | 2 +- spec/factories/ci/job_artifacts.rb | 18 ++++++++++++++++-- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index ff52ca64459..ca9aba5ac3a 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -7,7 +7,7 @@ module ArtifactMigratable end def artifacts_metadata - job_artifacts_metadata&.file || legacy_artifacts_metadata + job_artifacts_archive_metadata&.file || legacy_artifacts_metadata end def artifacts? diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 83cb4750741..99f14a08039 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -182,7 +182,7 @@ FactoryBot.define do trait :artifacts do after(:create) do |build| create(:ci_job_artifact, :archive, job: build, expire_at: build.artifacts_expire_at) - create(:ci_job_artifact, :metadata, job: build, expire_at: build.artifacts_expire_at) + create(:ci_job_artifact, :archive_metadata, job: build, expire_at: build.artifacts_expire_at) build.reload end end diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index 3d3287d8168..631e92f7338 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -4,6 +4,7 @@ FactoryBot.define do factory :ci_job_artifact, class: Ci::JobArtifact do job factory: :ci_build file_type :archive + file_format :zip trait :remote_store do file_store JobArtifactUploader::Store::REMOTE @@ -15,6 +16,7 @@ FactoryBot.define do trait :archive do file_type :archive + file_format :zip after(:build) do |artifact, _| artifact.file = fixture_file_upload( @@ -22,8 +24,9 @@ FactoryBot.define do end end - trait :metadata do - file_type :metadata + trait :archive_metadata do + file_type :archive_metadata + file_format :gzip after(:build) do |artifact, _| artifact.file = fixture_file_upload( @@ -33,6 +36,7 @@ FactoryBot.define do trait :trace do file_type :trace + file_format :raw after(:build) do |artifact, evaluator| artifact.file = fixture_file_upload( @@ -40,6 +44,16 @@ FactoryBot.define do end end + trait :junit do + file_type :junit + file_format :gzip + + after(:build) do |artifact, evaluator| + artifact.file = fixture_file_upload( + Rails.root.join('spec/fixtures/junit.xml.gz'), 'application/x-gzip') + end + end + trait :correct_checksum do after(:build) do |artifact, evaluator| artifact.file_sha256 = Digest::SHA256.file(artifact.file.path).hexdigest -- cgit v1.2.1 From 14821f3babcc210bc52e4e825adc8333752fbc88 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 17:55:41 +0900 Subject: Add spec for file format. Add spec for config_artifacts --- app/presenters/ci/build_presenter.rb | 20 ++++++++--------- spec/models/ci/job_artifact_spec.rb | 24 ++++++++++++++++++++ spec/presenters/ci/build_presenter_spec.rb | 36 ++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 10 deletions(-) diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index 690380cbfaf..a83135ed3f9 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -44,6 +44,16 @@ module Ci list.flatten end + private + + def tooltip_for_badge + detailed_status.badge_tooltip.capitalize + end + + def detailed_status + @detailed_status ||= subject.detailed_status(user) + end + def config_artifacts_archive(artifacts) artifacts.merge(type: :archive, format: :zip) end @@ -59,15 +69,5 @@ module Ci def config_artifacts_reports_junit(junit) { paths: junit, type: :junit, format: :gzip } end - - private - - def tooltip_for_badge - detailed_status.badge_tooltip.capitalize - end - - def detailed_status - @detailed_status ||= subject.detailed_status(user) - end end end diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index 0fd7612c011..594211c288f 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -87,6 +87,30 @@ describe Ci::JobArtifact do end end + describe 'validates file format' do + subject { artifact.valid? } + + let(:artifact) { build(:ci_job_artifact, file_format: file_format) } + + context 'when file format is present' do + let(:file_format) { :raw } + + it { is_expected.to be_truthy } + end + + context 'when file format is not defined' do + let(:file_format) { :new_format } + + it { expect { subject }.to raise_error(ArgumentError) } + end + + context 'when file format is not present' do + let(:file_format) { nil } + + it { is_expected.to be_falsy } + end + end + describe '#file' do subject { artifact.file } diff --git a/spec/presenters/ci/build_presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb index 6dfaa3b72f7..78d00c4e14a 100644 --- a/spec/presenters/ci/build_presenter_spec.rb +++ b/spec/presenters/ci/build_presenter_spec.rb @@ -252,4 +252,40 @@ describe Ci::BuildPresenter do end end end + + describe '#config_artifacts' do + context "when config has 'artifacts' keyword" do + let(:build) { create(:ci_build, options: { artifacts: { paths: ['sample.txt'] } } ) } + + it 'presents artifacts hash' do + expect(presenter.config_artifacts).to include({ type: :archive, format: :zip, paths: ['sample.txt'] }) + end + end + + context "when config has 'junit' keyword" do + let(:build) { create(:ci_build, options: { artifacts: { reports: { junit: ['junit.xml'] } } } ) } + + it 'presents artifacts hash' do + expect(presenter.config_artifacts).to include({ type: :junit, format: :gzip, paths: ['junit.xml'] }) + end + end + + context "when config has all artifact keywords" do + let(:build) { create(:ci_build, options: { script: 'echo', artifacts: { paths: ['sample.txt'], reports: { junit: ['junit.xml'] } } } ) } + + it 'presents artifacts hash' do + expect(presenter.config_artifacts).to include( + { type: :junit, format: :gzip, paths: ['junit.xml'] }, + { type: :archive, format: :zip, paths: ['sample.txt'] }) + end + end + + context "when config has no artifact keywords" do + let(:build) { create(:ci_build, :no_options) } + + it 'presents empty artifacts hash' do + expect(presenter.config_artifacts).to eq([]) + end + end + end end -- cgit v1.2.1 From 15531ba9feff669b2ac05936e0feaee1856c1571 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 17:57:31 +0900 Subject: Revert refactoring --- lib/api/job_artifacts.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/api/job_artifacts.rb b/lib/api/job_artifacts.rb index 38cf511eb36..32379d7c8ab 100644 --- a/lib/api/job_artifacts.rb +++ b/lib/api/job_artifacts.rb @@ -28,7 +28,7 @@ module API builds = user_project.latest_successful_builds_for(params[:ref_name]) latest_build = builds.find_by!(name: params[:job]) - present_carrierwave_file!(latest_build.artifacts_archive_file) + present_carrierwave_file!(latest_build.artifacts_file) end desc 'Download the artifacts archive from a job' do @@ -43,7 +43,7 @@ module API build = find_build!(params[:job_id]) - present_carrierwave_file!(build.artifacts_archive_file) + present_carrierwave_file!(build.artifacts_file) end desc 'Download a specific file from artifacts archive' do @@ -57,7 +57,7 @@ module API authorize_read_builds! build = find_build!(params[:job_id]) - not_found! unless build.artifacts_archive? + not_found! unless build.artifacts? path = Gitlab::Ci::Build::Artifacts::Path .new(params[:artifact_path]) @@ -77,7 +77,7 @@ module API build = find_build!(params[:job_id]) authorize!(:update_build, build) - break not_found!(build) unless build.artifacts_archive? + break not_found!(build) unless build.artifacts? build.keep_artifacts! -- cgit v1.2.1 From ede107caf13fb215045576dcce18e20eec776df1 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 17:58:28 +0900 Subject: Revert refactoring --- lib/gitlab/workhorse.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 425d92217a6..a9629a92a50 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -112,7 +112,7 @@ module Gitlab end def send_artifacts_entry(build, entry) - file = build.artifacts_archive_file + file = build.artifacts_file archive = file.file_storage? ? file.path : file.url params = { -- cgit v1.2.1 From e5ce3668ee65217aba610d5311efd5e82bacddf3 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 19:02:06 +0900 Subject: Add spec for Gitlab::Ci::Config::Entry::Artifacts --- spec/lib/gitlab/ci/config/entry/artifacts_spec.rb | 17 ++++++++ spec/lib/gitlab/ci/config/entry/reports_spec.rb | 53 +++++++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 spec/lib/gitlab/ci/config/entry/reports_spec.rb diff --git a/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb b/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb index 5c31423fdee..d48aac15f28 100644 --- a/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/artifacts_spec.rb @@ -18,6 +18,14 @@ describe Gitlab::Ci::Config::Entry::Artifacts do expect(entry).to be_valid end end + + context "when value includes 'reports' keyword" do + let(:config) { { paths: %w[public/], reports: { junit: 'junit.xml' } } } + + it 'returns general artifact and report-type artifacts configuration' do + expect(entry.value).to eq config + end + end end context 'when entry value is not correct' do @@ -39,6 +47,15 @@ describe Gitlab::Ci::Config::Entry::Artifacts do .to include 'artifacts config contains unknown keys: test' end end + + context "when 'reports' keyword is not hash" do + let(:config) { { paths: %w[public/], reports: 'junit.xml' } } + + it 'reports error' do + expect(entry.errors) + .to include 'artifacts reports should be a hash' + end + end end end end diff --git a/spec/lib/gitlab/ci/config/entry/reports_spec.rb b/spec/lib/gitlab/ci/config/entry/reports_spec.rb new file mode 100644 index 00000000000..0eb4bc4ec1f --- /dev/null +++ b/spec/lib/gitlab/ci/config/entry/reports_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +describe Gitlab::Ci::Config::Entry::Reports do + let(:entry) { described_class.new(config) } + + describe 'validation' do + context 'when entry config value is correct' do + let(:config) { { junit: %w[junit.xml] } } + + describe '#value' do + it 'returns artifacs configuration' do + expect(entry.value).to eq config + end + end + + describe '#valid?' do + it 'is valid' do + expect(entry).to be_valid + end + end + + context 'when value is not array' do + let(:config) { { junit: 'junit.xml' } } + + it 'converts to array' do + expect(entry.value).to eq({ junit: ['junit.xml'] } ) + end + end + end + + context 'when entry value is not correct' do + describe '#errors' do + context 'when value of attribute is invalid' do + let(:config) { { junit: 10 } } + + it 'reports error' do + expect(entry.errors) + .to include 'reports junit should be an array of strings or string' + end + end + + context 'when there is an unknown key present' do + let(:config) { { codeclimate: 'codeclimate.json' } } + + it 'reports error' do + expect(entry.errors) + .to include 'reports config contains unknown keys: codeclimate' + end + end + end + end + end +end -- cgit v1.2.1 From c3ce06aa9bc6481b37a16d175adf0fd1c37a1bc0 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 16 Jul 2018 22:27:46 +0900 Subject: Fix sending junit.xml --- app/presenters/ci/build_presenter.rb | 2 +- lib/api/runner.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index a83135ed3f9..c4e0baa0603 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -67,7 +67,7 @@ module Ci end def config_artifacts_reports_junit(junit) - { paths: junit, type: :junit, format: :gzip } + { name: 'junit.xml', paths: junit, type: :junit, format: :gzip } end end end diff --git a/lib/api/runner.rb b/lib/api/runner.rb index a549e9c7480..255b9114c4d 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -280,7 +280,7 @@ module API end if job.update(artifacts_expire_in: expire_in) - present job, with: Entities::JobRequest::Response + present job.present, with: Entities::JobRequest::Response else render_validation_error!(job) end -- cgit v1.2.1 From 541292c37e5ad24f4d137454743808cfc1f3c216 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 17 Jul 2018 16:26:37 +0900 Subject: Add parser and testresults --- .../projects/merge_requests_controller.rb | 7 +++ app/models/ci/build.rb | 21 +++++-- app/models/ci/job_artifact.rb | 16 +++++ app/models/ci/pipeline.rb | 8 +++ app/models/merge_request.rb | 14 +++++ lib/gitlab/ci/build/artifacts/gzip_file_adapter.rb | 38 ++++++++++++ lib/gitlab/ci/build/artifacts/junit_parser.rb | 68 ++++++++++++++++++++++ lib/gitlab/ci/build/artifacts/test_results.rb | 49 ++++++++++++++++ 8 files changed, 215 insertions(+), 6 deletions(-) create mode 100644 lib/gitlab/ci/build/artifacts/gzip_file_adapter.rb create mode 100644 lib/gitlab/ci/build/artifacts/junit_parser.rb create mode 100644 lib/gitlab/ci/build/artifacts/test_results.rb diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index dc6551fc761..7d20e0494bb 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -224,6 +224,13 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo render json: environments end + def test_results + # we would likely use Grape Entities: so Serializer and Entity + # so this is pseudo-code + render merge_request.test_results, with: MergeRequests::TestResults + end + + def rebase RebaseWorker.perform_async(@merge_request.id, current_user.id) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 7560828ecd0..8a7029fecd7 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -67,6 +67,10 @@ module Ci where('NOT EXISTS (?)', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').trace) end + scope :with_junit_artifacts, ->() do + where('EXISTS (?)', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').junit) + end + scope :with_artifacts_stored_locally, -> { with_artifacts_archive.where(artifacts_file_store: [nil, LegacyArtifactUploader::Store::LOCAL]) } scope :with_artifacts_not_expired, ->() { with_artifacts_archive.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts_archive.where('artifacts_expire_at < ?', Time.now) } @@ -448,12 +452,6 @@ module Ci end end - def artifacts_junit_hash - job_artifacts_junit.open do |stream| - Gitlab::Ci::Build::Artifacts::Junit.new(stream).to_hash - end - end - def erase_artifacts! remove_legacy_artifacts_file! if legacy_artifacts_file remove_legacy_artifacts_metadata! if legacy_artifacts_metadata @@ -608,6 +606,17 @@ module Ci running? && runner_session_url.present? end + # job_artifacts_junit.reader.each_blob do |blob, name| + # Gitlab::Ci::Report::JUnitParser.new(blob).parse!(test_results) + # end + def collect_test_results(test_results) + job_artifacts_junit.open do |stream| + Gitlab::Ci::Build::Artifacts::GzipFileAdapter.new(stream).each_blob do |blob, name| + Gitlab::Ci::Build::Artifacts::JunitParser.new(blob).parse!(test_results) + end + end + end + private def update_artifacts_size diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 2c9f7e98483..d4795549d7e 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -58,6 +58,22 @@ module Ci end end + ## TODO: Improve abstraction + # def file_adapter(stream) + # case file_type + # when :gzip + # Gitlab::Ci::Build::Artifacts::GzipFileAdapter.new(stream) + # else + # raise 'Not supported' + # end + # end + + # def each_blob(&blk) + # file.open do |stream| + # file_adapter(stream).each_blob(&blk) + # end + # end + private def set_size diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index e5caa3ffa41..671b812cbdf 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -603,6 +603,14 @@ module Ci @latest_builds_with_artifacts ||= builds.latest.with_artifacts_archive.to_a end + def test_results + Gitlab::Ci::Build::Artifacts::TestResults.new.tap do |test_results| + builds.with_junit_artifacts.each do |build| + build.collect_test_results(test_results) + end + end + end + private def ci_yaml_from_repo diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b4090fd8baf..717bfb4d578 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1009,6 +1009,20 @@ class MergeRequest < ActiveRecord::Base .order(id: :desc) end + def test_results + TestResultsComparer.new.tap do |test_results_comparer| + test_results_comparer.compare(base_test_results, head_test_results) + end + end + + def base_test_results + base_pipeline&.test_results + end + + def head_test_results + head_pipeline&.test_results + end + def all_commits # MySQL doesn't support LIMIT in a subquery. diffs_relation = if Gitlab::Database.postgresql? diff --git a/lib/gitlab/ci/build/artifacts/gzip_file_adapter.rb b/lib/gitlab/ci/build/artifacts/gzip_file_adapter.rb new file mode 100644 index 00000000000..1687424d0d3 --- /dev/null +++ b/lib/gitlab/ci/build/artifacts/gzip_file_adapter.rb @@ -0,0 +1,38 @@ +module Gitlab + module Ci + module Build + module Artifacts + class GzipFileAdapter + attr_reader :stream + + def initialize(stream) + @stream = stream + end + + def each_blob + stream.seek(0) + + until stream.eof? + gzip(stream) do |gz| + yield gz.read, gz.orig_name + unused = gz.unused&.length.to_i + stream.seek(-unused, IO::SEEK_CUR) # IO has already reached EOF, so rewind the pos to the pointer of unsued file + end + end + end + + def gzip(stream, &block) + raise Metadata::InvalidStreamError, "Invalid stream" unless stream + + gz = Zlib::GzipReader.new(stream) + yield(gz) + rescue Zlib::Error => e + raise Metadata::InvalidStreamError, e.message + ensure + gz&.finish + end + end + end + end + end +end diff --git a/lib/gitlab/ci/build/artifacts/junit_parser.rb b/lib/gitlab/ci/build/artifacts/junit_parser.rb new file mode 100644 index 00000000000..bc760fc1bea --- /dev/null +++ b/lib/gitlab/ci/build/artifacts/junit_parser.rb @@ -0,0 +1,68 @@ +module Gitlab + module Ci + module Build + module Artifacts + class JunitParser + attr_reader :data + + def initialize(xml_data) + @data = Hash.from_xml(xml_data) + end + + def parse!(test_results) + testsuites = data['testsuites'] ? data['testsuites'] : [data['testsuite']] + + testsuites.each do |testsuite| + parse_testsuite(testsuite) do |summary, testsuite_name, testcases| + test_results.add_summary(summary) + + parse_testcases(testsuite_name, testcases) do |testcase| + test_results.add_result(testcase) + end + end + end + end + + private + + def parse_testsuite(testsuite) + sumamry = { + testsuite_tests: testsuite['tests'], + testsuite_errors: testsuite['errors'], + testsuite_failures: testsuite['failures'], + testsuite_skipped: testsuite['skipped'], + testsuite_time: testsuite['time'] + } + + yield sumamry, testsuite['name'], testsuite['testcase'] + end + + def parse_testcases(testsuite_name, testcases) + testcases.each do |testcase| + if testcase['failure'] + testcase_result = TestResults::RESULT_FAILED + testcase_failed_reason = testcase['failure'] + else + testcase_result = TestResults::RESULT_PASS + testcase_failed_reason = nil + end + + ret = { + testsuite_name: testsuite_name, + testcase_classname: testcase['classname'], + testcase_name: testcase['name'], + testcase_file: testcase['file'], + testcase_time: testcase['time'], + testcase_result: testcase_result, + testcase_failed_reason: testcase_failed_reason + } + + yield ret + end + end + end + end + end + end +end + diff --git a/lib/gitlab/ci/build/artifacts/test_results.rb b/lib/gitlab/ci/build/artifacts/test_results.rb new file mode 100644 index 00000000000..abf83dc31e6 --- /dev/null +++ b/lib/gitlab/ci/build/artifacts/test_results.rb @@ -0,0 +1,49 @@ +module Gitlab + module Ci + module Build + module Artifacts + class TestResults + RESULT_PASS = :pass + RESULT_FAILED = :failed + + def initialize + @sumamry = { testsuite_tests: 0, testsuite_errors: 0, testsuite_failures: 0, testsuite_skipped: 0, testsuite_time: 0 } + @results = Hash.new + end + + def add_summary(testsuite_tests:, testsuite_errors:, testsuite_failures:, testsuite_skipped:, testsuite_time:) + @sumamry[:testsuite_tests] += testsuite_tests.to_i + @sumamry[:testsuite_errors] += testsuite_errors.to_i + @sumamry[:testsuite_failures] += testsuite_failures.to_i + @sumamry[:testsuite_skipped] += testsuite_skipped.to_i + @sumamry[:testsuite_time] += testsuite_time.to_i + end + + def add_result(testsuite_name:, testcase_classname:, testcase_name:, testcase_file:, testcase_time:, testcase_result:, testcase_failed_reason:) + raise 'Insufficient data to generate a unique key' unless testsuite_name.present? && testcase_name.present? && testcase_name.present? + + key = sanitize_key_name("#{testsuite_name}_#{testcase_classname}_#{testcase_name}") + + @results[testsuite_name] ||= {} + + @results[testsuite_name][key] = { + testsuite_name: testsuite_name, + testcase_classname: testcase_classname, + testcase_name: testcase_name, + testcase_file: testcase_file, + testcase_time: testcase_time, + testcase_result: testcase_result, + testcase_failed_reason: testcase_failed_reason + } + end + + private + + def sanitize_key_name(key) + key.gsub(/[^0-9A-Za-z]/, '-') + end + end + end + end + end +end -- cgit v1.2.1 From d0ad35fcc4b1d9c58d798775e13623026900ed27 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 17 Jul 2018 16:35:06 +0900 Subject: Remove unnecessary files --- lib/gitlab/ci/build/artifacts/junit.rb | 36 ------------------------- lib/gitlab/ci/build/artifacts/xml.rb | 49 ---------------------------------- 2 files changed, 85 deletions(-) delete mode 100644 lib/gitlab/ci/build/artifacts/junit.rb delete mode 100644 lib/gitlab/ci/build/artifacts/xml.rb diff --git a/lib/gitlab/ci/build/artifacts/junit.rb b/lib/gitlab/ci/build/artifacts/junit.rb deleted file mode 100644 index da3b8cea0ee..00000000000 --- a/lib/gitlab/ci/build/artifacts/junit.rb +++ /dev/null @@ -1,36 +0,0 @@ -module Gitlab - module Ci - module Build - module Artifacts - class Junit < XML - attr_reader :junit_artifact - - def to_hash - Gitlab::Ci::Build::Artifacts::XML.new(stream).each_hash do |file_name, hash| - hash['testsuite'].each do |testsuite| - # testsuite.name - # testsuite.tests - # testsuite.skipped - # testsuite.failures - testsuite['testcase'].each do |testcase| - # testcase.classname - # testcase.name - # testcase.file - # testcase.time - if testcase.key?('failure') - testcase['failure'] # stack_trace - testcase['system-out'] - elsif testcase.key?('skipped') || testcase.key?('skipped') - # TODO: - else - # passed - end - end - end - end - end - end - end - end - end -end diff --git a/lib/gitlab/ci/build/artifacts/xml.rb b/lib/gitlab/ci/build/artifacts/xml.rb deleted file mode 100644 index 5d3a0c62d6f..00000000000 --- a/lib/gitlab/ci/build/artifacts/xml.rb +++ /dev/null @@ -1,49 +0,0 @@ -require 'zlib' -require 'json' - -module Gitlab - module Ci - module Build - module Artifacts - class XML - attr_reader :stream - - def initialize(stream) - @stream = stream - end - - def each_hash - each_raw do |file_name, xml| - yield file_name, Hash.from_xml(xml) - end - rescue => e - raise "Failed to parse xml #{e.message}" - end - - def each_raw - stream.seek(0) - - until stream.eof? - gzip(stream) do |gz| - yield gz.orig_name, gz.read - unused = gz.unused&.length.to_i - stream.seek(-unused, IO::SEEK_CUR) # IO has already reached EOF, so rewind the pos to the unsued file - end - end - end - - def gzip(stream, &block) - raise Metadata::InvalidStreamError, "Invalid stream" unless stream - - gz = Zlib::GzipReader.new(stream) - yield(gz) - rescue Zlib::Error => e - raise Metadata::InvalidStreamError, e.message - ensure - gz&.finish - end - end - end - end - end -end -- cgit v1.2.1 From 598b34072c2c7b417e47945389b88e5103fc4b17 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 17 Jul 2018 17:55:34 +0900 Subject: Pass build.group_name as testsuite name --- app/models/ci/build.rb | 4 ++-- app/models/ci/pipeline.rb | 4 ++-- lib/gitlab/ci/build/artifacts/junit_parser.rb | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8a7029fecd7..e370feed749 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -67,7 +67,7 @@ module Ci where('NOT EXISTS (?)', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').trace) end - scope :with_junit_artifacts, ->() do + scope :with_test_reports, ->() do where('EXISTS (?)', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').junit) end @@ -612,7 +612,7 @@ module Ci def collect_test_results(test_results) job_artifacts_junit.open do |stream| Gitlab::Ci::Build::Artifacts::GzipFileAdapter.new(stream).each_blob do |blob, name| - Gitlab::Ci::Build::Artifacts::JunitParser.new(blob).parse!(test_results) + Gitlab::Ci::Build::Artifacts::JunitParser.new(blob, group_name).parse!(test_results) end end end diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 671b812cbdf..228922ec27d 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -488,7 +488,7 @@ module Ci @ci_yaml_file else self.yaml_errors = "Failed to load CI/CD config file for #{sha}" - nil + nil end end @@ -605,7 +605,7 @@ module Ci def test_results Gitlab::Ci::Build::Artifacts::TestResults.new.tap do |test_results| - builds.with_junit_artifacts.each do |build| + builds.with_test_reports.each do |build| build.collect_test_results(test_results) end end diff --git a/lib/gitlab/ci/build/artifacts/junit_parser.rb b/lib/gitlab/ci/build/artifacts/junit_parser.rb index bc760fc1bea..0aac05fd4eb 100644 --- a/lib/gitlab/ci/build/artifacts/junit_parser.rb +++ b/lib/gitlab/ci/build/artifacts/junit_parser.rb @@ -3,10 +3,11 @@ module Gitlab module Build module Artifacts class JunitParser - attr_reader :data + attr_reader :data, :group_name - def initialize(xml_data) + def initialize(xml_data, group_name) @data = Hash.from_xml(xml_data) + @group_name = group_name end def parse!(test_results) @@ -17,7 +18,7 @@ module Gitlab test_results.add_summary(summary) parse_testcases(testsuite_name, testcases) do |testcase| - test_results.add_result(testcase) + test_results.add_result(testsuite_name: group_name, **testcase) end end end @@ -65,4 +66,3 @@ module Gitlab end end end - -- cgit v1.2.1 From 612c44a39099edfb258cc3c1c8e650ab192d9a8b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 17 Jul 2018 18:50:03 +0900 Subject: Objectize each test suit and summary --- app/models/ci/build.rb | 2 +- lib/gitlab/ci/build/artifacts/junit_parser.rb | 71 +++++++++------------- .../ci/build/artifacts/test_result/test_case.rb | 36 +++++++++++ .../ci/build/artifacts/test_result/test_summary.rb | 17 ++++++ lib/gitlab/ci/build/artifacts/test_results.rb | 58 +++++++----------- 5 files changed, 105 insertions(+), 79 deletions(-) create mode 100644 lib/gitlab/ci/build/artifacts/test_result/test_case.rb create mode 100644 lib/gitlab/ci/build/artifacts/test_result/test_summary.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index e370feed749..062923c89dd 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -612,7 +612,7 @@ module Ci def collect_test_results(test_results) job_artifacts_junit.open do |stream| Gitlab::Ci::Build::Artifacts::GzipFileAdapter.new(stream).each_blob do |blob, name| - Gitlab::Ci::Build::Artifacts::JunitParser.new(blob, group_name).parse!(test_results) + Gitlab::Ci::Build::Artifacts::JunitParser.new(blob).parse!(group_name, test_results) end end end diff --git a/lib/gitlab/ci/build/artifacts/junit_parser.rb b/lib/gitlab/ci/build/artifacts/junit_parser.rb index 0aac05fd4eb..8950dbd1e60 100644 --- a/lib/gitlab/ci/build/artifacts/junit_parser.rb +++ b/lib/gitlab/ci/build/artifacts/junit_parser.rb @@ -3,63 +3,48 @@ module Gitlab module Build module Artifacts class JunitParser - attr_reader :data, :group_name + attr_reader :data - def initialize(xml_data, group_name) + def initialize(xml_data) @data = Hash.from_xml(xml_data) - @group_name = group_name end - def parse!(test_results) - testsuites = data['testsuites'] ? data['testsuites'] : [data['testsuite']] - - testsuites.each do |testsuite| - parse_testsuite(testsuite) do |summary, testsuite_name, testcases| - test_results.add_summary(summary) - - parse_testcases(testsuite_name, testcases) do |testcase| - test_results.add_result(testsuite_name: group_name, **testcase) - end + def parse!(suite, test_results) + each_suite do |test_suite, testcases| + testcases.each do |testcase| + test_case_result = create_test_result(testcase) + test_results.add_result(suite, test_case_result) end end end private - def parse_testsuite(testsuite) - sumamry = { - testsuite_tests: testsuite['tests'], - testsuite_errors: testsuite['errors'], - testsuite_failures: testsuite['failures'], - testsuite_skipped: testsuite['skipped'], - testsuite_time: testsuite['time'] - } + def each_suite + testsuites = data['testsuites'] ? data['testsuites'] : [data['testsuite']] - yield sumamry, testsuite['name'], testsuite['testcase'] + testsuites.each do |testsuite| + yield testsuite['name'], testsuite['testcase'] + end end - def parse_testcases(testsuite_name, testcases) - testcases.each do |testcase| - if testcase['failure'] - testcase_result = TestResults::RESULT_FAILED - testcase_failed_reason = testcase['failure'] - else - testcase_result = TestResults::RESULT_PASS - testcase_failed_reason = nil - end - - ret = { - testsuite_name: testsuite_name, - testcase_classname: testcase['classname'], - testcase_name: testcase['name'], - testcase_file: testcase['file'], - testcase_time: testcase['time'], - testcase_result: testcase_result, - testcase_failed_reason: testcase_failed_reason - } - - yield ret + def create_test_result(testcase) + if testcase['failure'] + result = TestResult::TestCase::RESULT_FAILED + failure_reason = testcase['failure'] + else + result = TestResult::TestCase::RESULT_SUCCESS + failure_reason = nil end + + TestResult::TestCase.new( + classname: testcase['classname'], + name: testcase['name'], + file: testcase['file'], + time: testcase['time'], + result: result, + failure_reason: failure_reason + ) end end end diff --git a/lib/gitlab/ci/build/artifacts/test_result/test_case.rb b/lib/gitlab/ci/build/artifacts/test_result/test_case.rb new file mode 100644 index 00000000000..1834ca173ad --- /dev/null +++ b/lib/gitlab/ci/build/artifacts/test_result/test_case.rb @@ -0,0 +1,36 @@ +module Gitlab + module Ci + module Build + module Artifacts + module TestResult + class TestCase + RESULT_SUCCESS = :success + RESULT_FAILED = :failed + RESULT_SKIPPED = :skipped + RESULT_ERROR = :error + + attr_reader :name, :classname, :file, :time, :result, :failure_reason, :key + + def initialize(name:, classname:, file:, time:, result:, failure_reason:) + raise 'Insufficient data to generate a unique key' unless classname.present? && name.present? + + @name = name + @classname = classname + @file = file + @time = time + @result = result + @failure_reason = failure_reason + @key = sanitize_key_name("#{classname}_#{name}") + end + + private + + def sanitize_key_name(key) + key.gsub(/[^0-9A-Za-z]/, '-') + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/build/artifacts/test_result/test_summary.rb b/lib/gitlab/ci/build/artifacts/test_result/test_summary.rb new file mode 100644 index 00000000000..492d62473ff --- /dev/null +++ b/lib/gitlab/ci/build/artifacts/test_result/test_summary.rb @@ -0,0 +1,17 @@ +module Gitlab + module Ci + module Build + module Artifacts + module TestResult + class TestSummary + attr_accessor :test_count, :success_count, :error_count, :failure_count, :skipped_count, :total_time + + def initialize + @test_count = @success_count = @error_count = @failure_count = @skipped_count = @total_time = 0 + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/build/artifacts/test_results.rb b/lib/gitlab/ci/build/artifacts/test_results.rb index abf83dc31e6..6b24edcc744 100644 --- a/lib/gitlab/ci/build/artifacts/test_results.rb +++ b/lib/gitlab/ci/build/artifacts/test_results.rb @@ -3,44 +3,32 @@ module Gitlab module Build module Artifacts class TestResults - RESULT_PASS = :pass - RESULT_FAILED = :failed - + attr_reader :summary, :test_case_results + def initialize - @sumamry = { testsuite_tests: 0, testsuite_errors: 0, testsuite_failures: 0, testsuite_skipped: 0, testsuite_time: 0 } - @results = Hash.new - end - - def add_summary(testsuite_tests:, testsuite_errors:, testsuite_failures:, testsuite_skipped:, testsuite_time:) - @sumamry[:testsuite_tests] += testsuite_tests.to_i - @sumamry[:testsuite_errors] += testsuite_errors.to_i - @sumamry[:testsuite_failures] += testsuite_failures.to_i - @sumamry[:testsuite_skipped] += testsuite_skipped.to_i - @sumamry[:testsuite_time] += testsuite_time.to_i + @summary = TestResult::TestSummary.new + @test_case_results = {} end - def add_result(testsuite_name:, testcase_classname:, testcase_name:, testcase_file:, testcase_time:, testcase_result:, testcase_failed_reason:) - raise 'Insufficient data to generate a unique key' unless testsuite_name.present? && testcase_name.present? && testcase_name.present? - - key = sanitize_key_name("#{testsuite_name}_#{testcase_classname}_#{testcase_name}") - - @results[testsuite_name] ||= {} - - @results[testsuite_name][key] = { - testsuite_name: testsuite_name, - testcase_classname: testcase_classname, - testcase_name: testcase_name, - testcase_file: testcase_file, - testcase_time: testcase_time, - testcase_result: testcase_result, - testcase_failed_reason: testcase_failed_reason - } - end - - private - - def sanitize_key_name(key) - key.gsub(/[^0-9A-Za-z]/, '-') + def add_result(suite, test_case_result) + test_case_results[suite] ||= [] + test_case_results[suite] << test_case_result + + summary.test_count += 1 + summary.total_time += test_case_result.time.to_f + + case test_case_result.result + when TestResult::TestCase::RESULT_SUCCESS + summary.success_count += 1 + when TestResult::TestCase::RESULT_FAILED + summary.failure_count += 1 + when TestResult::TestCase::RESULT_SKIPPED + summary.skipped_count += 1 + when TestResult::TestCase::RESULT_ERROR + summary.error_count += 1 + else + raise 'Unknown result' + end end end end -- cgit v1.2.1 From 55edde40b15b3499cfc7ecbbe08f15cae9f6661a Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 17 Jul 2018 19:52:01 +0900 Subject: Implement comprare failed tests --- app/models/ci/pipeline.rb | 2 +- lib/gitlab/ci/build/artifacts/test_results.rb | 33 ++++++++++++++++++++++++--- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 228922ec27d..1e8e76c8a9d 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -488,7 +488,7 @@ module Ci @ci_yaml_file else self.yaml_errors = "Failed to load CI/CD config file for #{sha}" - nil + nil end end diff --git a/lib/gitlab/ci/build/artifacts/test_results.rb b/lib/gitlab/ci/build/artifacts/test_results.rb index 6b24edcc744..9a72f5b0f9c 100644 --- a/lib/gitlab/ci/build/artifacts/test_results.rb +++ b/lib/gitlab/ci/build/artifacts/test_results.rb @@ -4,15 +4,20 @@ module Gitlab module Artifacts class TestResults attr_reader :summary, :test_case_results - + def initialize @summary = TestResult::TestSummary.new @test_case_results = {} end def add_result(suite, test_case_result) - test_case_results[suite] ||= [] - test_case_results[suite] << test_case_result + # test_case_results[suite] ||= [] + # test_case_results[suite] << test_case_result + + # Filter by result, this way, we can quickly compare failed results + test_case_results[suite] ||= {} + test_case_results[suite][test_case_result.result] ||= [] + test_case_results[suite][test_case_result.result] << test_case_result summary.test_count += 1 summary.total_time += test_case_result.time.to_f @@ -30,6 +35,28 @@ module Gitlab raise 'Unknown result' end end + + def compare_failed_tests(base_test_results) + head = self.failed_tests.map(&:key) + base = base_test_results.failed_tests.map(&:key) + + new_failures = head - base + resolved_failures = base - head + existing_failures = head - new_failures + + # TODO: Return TestResult::TestCase instead of returning keys + { + new: new_failures, + resolved: resolved_failures, + existing: existing_failures + } + end + + def failed_tests + test_case_results.keys.map do |suite| + test_case_results[suite][TestResult::TestCase::RESULT_FAILED] + end.flatten.compact + end end end end -- cgit v1.2.1 From 0553827fff5e1796bbfcdd7a5daf324a7dd16ea0 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 17 Jul 2018 20:13:31 +0900 Subject: Refactoring suites. Adding gurad clause at collect_test_results --- app/models/ci/build.rb | 2 ++ lib/gitlab/ci/build/artifacts/junit_parser.rb | 4 ++-- lib/gitlab/ci/build/artifacts/test_results.rb | 18 +++++++++--------- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 062923c89dd..4e91251bdcc 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -610,6 +610,8 @@ module Ci # Gitlab::Ci::Report::JUnitParser.new(blob).parse!(test_results) # end def collect_test_results(test_results) + raise 'Junit test report does not exist' unless job_artifacts_junit + job_artifacts_junit.open do |stream| Gitlab::Ci::Build::Artifacts::GzipFileAdapter.new(stream).each_blob do |blob, name| Gitlab::Ci::Build::Artifacts::JunitParser.new(blob).parse!(group_name, test_results) diff --git a/lib/gitlab/ci/build/artifacts/junit_parser.rb b/lib/gitlab/ci/build/artifacts/junit_parser.rb index 8950dbd1e60..bafd8d5e16e 100644 --- a/lib/gitlab/ci/build/artifacts/junit_parser.rb +++ b/lib/gitlab/ci/build/artifacts/junit_parser.rb @@ -10,7 +10,7 @@ module Gitlab end def parse!(suite, test_results) - each_suite do |test_suite, testcases| + each_suite do |testcases, _| testcases.each do |testcase| test_case_result = create_test_result(testcase) test_results.add_result(suite, test_case_result) @@ -24,7 +24,7 @@ module Gitlab testsuites = data['testsuites'] ? data['testsuites'] : [data['testsuite']] testsuites.each do |testsuite| - yield testsuite['name'], testsuite['testcase'] + yield testsuite['testcase'], testsuite['name'] end end diff --git a/lib/gitlab/ci/build/artifacts/test_results.rb b/lib/gitlab/ci/build/artifacts/test_results.rb index 9a72f5b0f9c..01a81476b18 100644 --- a/lib/gitlab/ci/build/artifacts/test_results.rb +++ b/lib/gitlab/ci/build/artifacts/test_results.rb @@ -3,21 +3,21 @@ module Gitlab module Build module Artifacts class TestResults - attr_reader :summary, :test_case_results + attr_reader :summary, :suites def initialize @summary = TestResult::TestSummary.new - @test_case_results = {} + @suites = {} end def add_result(suite, test_case_result) - # test_case_results[suite] ||= [] - # test_case_results[suite] << test_case_result + # suites[suite] ||= [] + # suites[suite] << test_case_result # Filter by result, this way, we can quickly compare failed results - test_case_results[suite] ||= {} - test_case_results[suite][test_case_result.result] ||= [] - test_case_results[suite][test_case_result.result] << test_case_result + suites[suite] ||= {} + suites[suite][test_case_result.result] ||= [] + suites[suite][test_case_result.result] << test_case_result summary.test_count += 1 summary.total_time += test_case_result.time.to_f @@ -53,8 +53,8 @@ module Gitlab end def failed_tests - test_case_results.keys.map do |suite| - test_case_results[suite][TestResult::TestCase::RESULT_FAILED] + suites.keys.map do |suite| + suites[suite][TestResult::TestCase::RESULT_FAILED] end.flatten.compact end end -- cgit v1.2.1 From 5ea46ce5cd6d0f74802216d1c63d274a48d3cd08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Tue, 17 Jul 2018 17:18:19 +0200 Subject: Improve code of JUnit reports --- app/models/ci/build.rb | 11 ++-- app/models/ci/job_artifact.rb | 30 +++++----- db/fixtures/development/14_pipelines.rb | 50 ++++++++++------ lib/gitlab/ci/build/artifacts/junit_parser.rb | 53 ----------------- .../ci/build/artifacts/test_result/test_case.rb | 36 ------------ .../ci/build/artifacts/test_result/test_summary.rb | 17 ------ lib/gitlab/ci/build/artifacts/test_results.rb | 64 --------------------- lib/gitlab/ci/parsers/junit_parser.rb | 51 ++++++++++++++++ lib/gitlab/ci/reports/test_case.rb | 33 +++++++++++ lib/gitlab/ci/reports/test_results.rb | 55 ++++++++++++++++++ lib/gitlab/ci/reports/test_suite.rb | 33 +++++++++++ spec/fixtures/junit/junit_feature.xml.gz | Bin 0 -> 541 bytes spec/fixtures/junit/junit_master.xml.gz | Bin 0 -> 568 bytes 13 files changed, 222 insertions(+), 211 deletions(-) delete mode 100644 lib/gitlab/ci/build/artifacts/junit_parser.rb delete mode 100644 lib/gitlab/ci/build/artifacts/test_result/test_case.rb delete mode 100644 lib/gitlab/ci/build/artifacts/test_result/test_summary.rb delete mode 100644 lib/gitlab/ci/build/artifacts/test_results.rb create mode 100644 lib/gitlab/ci/parsers/junit_parser.rb create mode 100644 lib/gitlab/ci/reports/test_case.rb create mode 100644 lib/gitlab/ci/reports/test_results.rb create mode 100644 lib/gitlab/ci/reports/test_suite.rb create mode 100644 spec/fixtures/junit/junit_feature.xml.gz create mode 100644 spec/fixtures/junit/junit_master.xml.gz diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 4e91251bdcc..0078ae052d2 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -606,15 +606,12 @@ module Ci running? && runner_session_url.present? end - # job_artifacts_junit.reader.each_blob do |blob, name| - # Gitlab::Ci::Report::JUnitParser.new(blob).parse!(test_results) - # end def collect_test_results(test_results) - raise 'Junit test report does not exist' unless job_artifacts_junit + return unless job_artifacts_junit - job_artifacts_junit.open do |stream| - Gitlab::Ci::Build::Artifacts::GzipFileAdapter.new(stream).each_blob do |blob, name| - Gitlab::Ci::Build::Artifacts::JunitParser.new(blob).parse!(group_name, test_results) + test_results.get_suite(group_name).tap do |test_suite| + job_artifacts_junit.each_blob do |blob, name| + Gitlab::Ci::Parsers::JunitParser.new(blob).parse!(test_suite) end end end diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index d4795549d7e..e7ab74cbd3d 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -33,6 +33,10 @@ module Ci gzip: 3 } + FILE_FORMAT_ADAPTERS = { + gzip: Gitlab::Ci::Build::Artifacts::GzipFileAdapter + } + def update_file_store # The file.object_store is set during `uploader.store!` # which happens after object is inserted/updated @@ -58,24 +62,20 @@ module Ci end end - ## TODO: Improve abstraction - # def file_adapter(stream) - # case file_type - # when :gzip - # Gitlab::Ci::Build::Artifacts::GzipFileAdapter.new(stream) - # else - # raise 'Not supported' - # end - # end - - # def each_blob(&blk) - # file.open do |stream| - # file_adapter(stream).each_blob(&blk) - # end - # end + def each_blob(&blk) + raise 'Not supported adapter' unless file_format_adapter_class + + file.open do |stream| + file_format_adapter_class.new(stream).each_blob(&blk) + end + end private + def file_format_adapter_class + FILE_FORMAT_ADAPTERS[file_format.to_sym] + end + def set_size self.size = file.size end diff --git a/db/fixtures/development/14_pipelines.rb b/db/fixtures/development/14_pipelines.rb index d3a63aa2a78..6d938dc2b0e 100644 --- a/db/fixtures/development/14_pipelines.rb +++ b/db/fixtures/development/14_pipelines.rb @@ -54,16 +54,10 @@ class Gitlab::Seeder::Pipelines def seed! pipelines.each do |pipeline| - begin - BUILDS.each { |opts| build_create!(pipeline, opts) } - EXTERNAL_JOBS.each { |opts| commit_status_create!(pipeline, opts) } - print '.' - rescue ActiveRecord::RecordInvalid - print 'F' - ensure - pipeline.update_duration - pipeline.update_status - end + BUILDS.each { |opts| build_create!(pipeline, opts) } + EXTERNAL_JOBS.each { |opts| commit_status_create!(pipeline, opts) } + pipeline.update_duration + pipeline.update_status end end @@ -116,7 +110,7 @@ class Gitlab::Seeder::Pipelines build.project.environments. find_or_create_by(name: build.expanded_environment_name) - build.save + build.save! end end @@ -124,11 +118,21 @@ class Gitlab::Seeder::Pipelines return unless %w[build test].include?(build.stage) artifacts_cache_file(artifacts_archive_path) do |file| - build.job_artifacts.build(project: build.project, file_type: :archive, file: file) + build.job_artifacts.build(project: build.project, file_type: :archive, file_format: :zip, file: file) end artifacts_cache_file(artifacts_metadata_path) do |file| - build.job_artifacts.build(project: build.project, file_type: :metadata, file: file) + build.job_artifacts.build(project: build.project, file_type: :archive_metadata, file_format: :gzip, file: file) + end + + if build.ref == build.project.default_branch + artifacts_cache_file(artifacts_junit_master_path) do |file| + build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) + end + else + artifacts_cache_file(artifacts_junit_feature_path) do |file| + build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) + end end end @@ -171,18 +175,26 @@ class Gitlab::Seeder::Pipelines Rails.root + 'spec/fixtures/ci_build_artifacts_metadata.gz' end + def artifacts_junit_master_path + Rails.root + 'spec/fixtures/junit/junit_master.xml.gz' + end + + def artifacts_junit_feature_path + Rails.root + 'spec/fixtures/junit/junit_feature.xml.gz' + end + def artifacts_cache_file(file_path) - cache_path = file_path.to_s.gsub('ci_', "p#{@project.id}_") + file = Tempfile.new("artifacts") + file.close - FileUtils.copy(file_path, cache_path) - File.open(cache_path) do |file| - yield file - end + FileUtils.copy(file_path, file.path) + + yield(UploadedFile.new(file.path, filename: File.basename(file_path))) end end Gitlab::Seeder.quiet do - Project.all.sample(5).each do |project| + Project.all.sample(1).each do |project| project_builds = Gitlab::Seeder::Pipelines.new(project) project_builds.seed! end diff --git a/lib/gitlab/ci/build/artifacts/junit_parser.rb b/lib/gitlab/ci/build/artifacts/junit_parser.rb deleted file mode 100644 index bafd8d5e16e..00000000000 --- a/lib/gitlab/ci/build/artifacts/junit_parser.rb +++ /dev/null @@ -1,53 +0,0 @@ -module Gitlab - module Ci - module Build - module Artifacts - class JunitParser - attr_reader :data - - def initialize(xml_data) - @data = Hash.from_xml(xml_data) - end - - def parse!(suite, test_results) - each_suite do |testcases, _| - testcases.each do |testcase| - test_case_result = create_test_result(testcase) - test_results.add_result(suite, test_case_result) - end - end - end - - private - - def each_suite - testsuites = data['testsuites'] ? data['testsuites'] : [data['testsuite']] - - testsuites.each do |testsuite| - yield testsuite['testcase'], testsuite['name'] - end - end - - def create_test_result(testcase) - if testcase['failure'] - result = TestResult::TestCase::RESULT_FAILED - failure_reason = testcase['failure'] - else - result = TestResult::TestCase::RESULT_SUCCESS - failure_reason = nil - end - - TestResult::TestCase.new( - classname: testcase['classname'], - name: testcase['name'], - file: testcase['file'], - time: testcase['time'], - result: result, - failure_reason: failure_reason - ) - end - end - end - end - end -end diff --git a/lib/gitlab/ci/build/artifacts/test_result/test_case.rb b/lib/gitlab/ci/build/artifacts/test_result/test_case.rb deleted file mode 100644 index 1834ca173ad..00000000000 --- a/lib/gitlab/ci/build/artifacts/test_result/test_case.rb +++ /dev/null @@ -1,36 +0,0 @@ -module Gitlab - module Ci - module Build - module Artifacts - module TestResult - class TestCase - RESULT_SUCCESS = :success - RESULT_FAILED = :failed - RESULT_SKIPPED = :skipped - RESULT_ERROR = :error - - attr_reader :name, :classname, :file, :time, :result, :failure_reason, :key - - def initialize(name:, classname:, file:, time:, result:, failure_reason:) - raise 'Insufficient data to generate a unique key' unless classname.present? && name.present? - - @name = name - @classname = classname - @file = file - @time = time - @result = result - @failure_reason = failure_reason - @key = sanitize_key_name("#{classname}_#{name}") - end - - private - - def sanitize_key_name(key) - key.gsub(/[^0-9A-Za-z]/, '-') - end - end - end - end - end - end -end diff --git a/lib/gitlab/ci/build/artifacts/test_result/test_summary.rb b/lib/gitlab/ci/build/artifacts/test_result/test_summary.rb deleted file mode 100644 index 492d62473ff..00000000000 --- a/lib/gitlab/ci/build/artifacts/test_result/test_summary.rb +++ /dev/null @@ -1,17 +0,0 @@ -module Gitlab - module Ci - module Build - module Artifacts - module TestResult - class TestSummary - attr_accessor :test_count, :success_count, :error_count, :failure_count, :skipped_count, :total_time - - def initialize - @test_count = @success_count = @error_count = @failure_count = @skipped_count = @total_time = 0 - end - end - end - end - end - end -end diff --git a/lib/gitlab/ci/build/artifacts/test_results.rb b/lib/gitlab/ci/build/artifacts/test_results.rb deleted file mode 100644 index 01a81476b18..00000000000 --- a/lib/gitlab/ci/build/artifacts/test_results.rb +++ /dev/null @@ -1,64 +0,0 @@ -module Gitlab - module Ci - module Build - module Artifacts - class TestResults - attr_reader :summary, :suites - - def initialize - @summary = TestResult::TestSummary.new - @suites = {} - end - - def add_result(suite, test_case_result) - # suites[suite] ||= [] - # suites[suite] << test_case_result - - # Filter by result, this way, we can quickly compare failed results - suites[suite] ||= {} - suites[suite][test_case_result.result] ||= [] - suites[suite][test_case_result.result] << test_case_result - - summary.test_count += 1 - summary.total_time += test_case_result.time.to_f - - case test_case_result.result - when TestResult::TestCase::RESULT_SUCCESS - summary.success_count += 1 - when TestResult::TestCase::RESULT_FAILED - summary.failure_count += 1 - when TestResult::TestCase::RESULT_SKIPPED - summary.skipped_count += 1 - when TestResult::TestCase::RESULT_ERROR - summary.error_count += 1 - else - raise 'Unknown result' - end - end - - def compare_failed_tests(base_test_results) - head = self.failed_tests.map(&:key) - base = base_test_results.failed_tests.map(&:key) - - new_failures = head - base - resolved_failures = base - head - existing_failures = head - new_failures - - # TODO: Return TestResult::TestCase instead of returning keys - { - new: new_failures, - resolved: resolved_failures, - existing: existing_failures - } - end - - def failed_tests - suites.keys.map do |suite| - suites[suite][TestResult::TestCase::RESULT_FAILED] - end.flatten.compact - end - end - end - end - end -end diff --git a/lib/gitlab/ci/parsers/junit_parser.rb b/lib/gitlab/ci/parsers/junit_parser.rb new file mode 100644 index 00000000000..f554ff9a20f --- /dev/null +++ b/lib/gitlab/ci/parsers/junit_parser.rb @@ -0,0 +1,51 @@ +module Gitlab + module Ci + module Parsers + class JunitParser + attr_reader :data + + def initialize(xml_data) + @data = Hash.from_xml(xml_data) + end + + def parse!(test_suite) + each_suite do |testcases, _| + testcases.each do |testcase| + test_case = create_test_case(testcase) + test_suite.add_result(test_case) + end + end + end + + private + + def each_suite + testsuites = data['testsuites'] ? data['testsuites'] : [data['testsuite']] + + testsuites.each do |testsuite| + yield testsuite['testcase'], testsuite['name'] + end + end + + def create_test_case(data) + if data['failure'] + status = ::Gitlab::Ci::Reports::TestCase::FAILED + failure_reason = data['failure'] + else + status = ::Gitlab::Ci::Reports::TestCase::SUCCESS + failure_reason = nil + end + + ::Gitlab::Ci::Reports::TestCase.new( + classname: data['classname'], + name: data['name'], + file: data['file'], + time: data['time'], + status: status, + failure_reason: failure_reason + ) + end + end + end + end +end diff --git a/lib/gitlab/ci/reports/test_case.rb b/lib/gitlab/ci/reports/test_case.rb new file mode 100644 index 00000000000..6c1d6ad95d9 --- /dev/null +++ b/lib/gitlab/ci/reports/test_case.rb @@ -0,0 +1,33 @@ +module Gitlab + module Ci + module Reports + class TestCase + STATUSES = %w(success failed skipped error) + SUCCESS = :success + FAILED = :failed + SKIPPED = :skipped + ERROR = :error + + attr_reader :name, :classname, :file, :time, :status, :failure_reason, :key + + def initialize(name:, classname:, file:, time:, status:, failure_reason:) + raise 'Insufficient data to generate a unique key' unless classname.present? && name.present? + + @name = name + @classname = classname + @file = file + @time = time + @status = status + @failure_reason = failure_reason + @key = sanitize_key_name("#{classname}_#{name}") + end + + private + + def sanitize_key_name(key) + key.gsub(/[^0-9A-Za-z]/, '-') + end + end + end + end +end diff --git a/lib/gitlab/ci/reports/test_results.rb b/lib/gitlab/ci/reports/test_results.rb new file mode 100644 index 00000000000..442f033ea0a --- /dev/null +++ b/lib/gitlab/ci/reports/test_results.rb @@ -0,0 +1,55 @@ +module Gitlab + module Ci + module Reports + class TestResults + attr_reader :suites + + def initialize + @suites = {} + end + + def get_suite(suite_name) + @suites[suite_name] ||= TestSuite.new(suite_name) + end + + def total_time + suites.values.sum(&:total_time) + end + + def total_count + suites.values.sum(&:total_count) + end + + TestCase::STATUSES.each do |result_type| + define_method("#{result_type}_count") do + suites.values.sum { |suite| suite.send("#{result_type}_count") } + end + end + + private + + def compare_failed_tests(base_test_results) + head = self.failed_tests.map(&:key) + base = base_test_results.failed_tests.map(&:key) + + new_failures = head - base + resolved_failures = base - head + existing_failures = head - new_failures + + # TODO: Return TestCase instead of returning keys + { + new: new_failures, + resolved: resolved_failures, + existing: existing_failures + } + end + + def failed_tests + suites.keys.map do |suite| + suites[suite][TestCase::RESULT_FAILED] + end.flatten.compact + end + end + end + end +end diff --git a/lib/gitlab/ci/reports/test_suite.rb b/lib/gitlab/ci/reports/test_suite.rb new file mode 100644 index 00000000000..d2ac4ebb361 --- /dev/null +++ b/lib/gitlab/ci/reports/test_suite.rb @@ -0,0 +1,33 @@ +module Gitlab + module Ci + module Reports + class TestSuite + attr_reader :name + attr_reader :test_statuses + attr_reader :total_time + + def initialize(name) + @name = name + @test_statuses = {} + @total_time = 0.0 + end + + TestCase::STATUSES.each do |result_type| + define_method("#{result_type}_count") do + test_statuses[result_type.to_sym]&.length.to_i + end + end + + def add_result(test_case) + @test_statuses[test_case.status] ||= {} + @test_statuses[test_case.status][test_case.key] = test_case + @total_time += test_case.time.to_f + end + + def total_count + test_statuses.values.sum(&:count) + end + end + end + end +end diff --git a/spec/fixtures/junit/junit_feature.xml.gz b/spec/fixtures/junit/junit_feature.xml.gz new file mode 100644 index 00000000000..802c35f6fcc Binary files /dev/null and b/spec/fixtures/junit/junit_feature.xml.gz differ diff --git a/spec/fixtures/junit/junit_master.xml.gz b/spec/fixtures/junit/junit_master.xml.gz new file mode 100644 index 00000000000..88b7de6fa61 Binary files /dev/null and b/spec/fixtures/junit/junit_master.xml.gz differ -- cgit v1.2.1 From e2670a3c642ba33e79202fc9adb044a78260c515 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Tue, 17 Jul 2018 18:00:59 +0200 Subject: Expose all data with API on Merge Request --- .../projects/merge_requests_controller.rb | 15 ++++---- app/models/ci/pipeline.rb | 8 +++-- app/models/merge_request.rb | 20 ++++++----- app/serializers/merge_request_widget_entity.rb | 6 ++++ app/serializers/test_case_entity.rb | 6 ++++ app/serializers/test_results_comparer_entity.rb | 3 ++ .../test_results_comparer_serializer.rb | 3 ++ app/serializers/test_suite_comparer_entity.rb | 6 ++++ config/routes/project.rb | 1 + lib/gitlab/ci/reports/test_results_comparer.rb | 24 +++++++++++++ lib/gitlab/ci/reports/test_suite.rb | 6 +++- lib/gitlab/ci/reports/test_suite_comparer.rb | 41 ++++++++++++++++++++++ 12 files changed, 120 insertions(+), 19 deletions(-) create mode 100644 app/serializers/test_case_entity.rb create mode 100644 app/serializers/test_results_comparer_entity.rb create mode 100644 app/serializers/test_results_comparer_serializer.rb create mode 100644 app/serializers/test_suite_comparer_entity.rb create mode 100644 lib/gitlab/ci/reports/test_results_comparer.rb create mode 100644 lib/gitlab/ci/reports/test_suite_comparer.rb diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 7d20e0494bb..46d9911546c 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -99,6 +99,14 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo } end + def test_results + test_results = @merge_request.compared_test_results + + render json: TestResultsComparerSerializer + .new(project: @project, current_user: @current_user) + .represent(test_results) + end + def edit define_edit_vars end @@ -224,13 +232,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo render json: environments end - def test_results - # we would likely use Grape Entities: so Serializer and Entity - # so this is pseudo-code - render merge_request.test_results, with: MergeRequests::TestResults - end - - def rebase RebaseWorker.perform_async(@merge_request.id, current_user.id) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 1e8e76c8a9d..0538f6f0ece 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -603,9 +603,13 @@ module Ci @latest_builds_with_artifacts ||= builds.latest.with_artifacts_archive.to_a end + def has_test_results? + builds.with_test_reports.any? + end + def test_results - Gitlab::Ci::Build::Artifacts::TestResults.new.tap do |test_results| - builds.with_test_reports.each do |build| + Gitlab::Ci::Reports::TestResults.new.tap do |test_results| + builds.with_test_reports.includes(:job_artifacts_junit).each do |build| build.collect_test_results(test_results) end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 717bfb4d578..5a3ad3f60ce 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1009,18 +1009,14 @@ class MergeRequest < ActiveRecord::Base .order(id: :desc) end - def test_results - TestResultsComparer.new.tap do |test_results_comparer| - test_results_comparer.compare(base_test_results, head_test_results) - end + def has_test_results? + actual_head_pipeline&.has_test_results? end - def base_test_results - base_pipeline&.test_results - end + def compared_test_results + return unless actual_head_pipeline - def head_test_results - head_pipeline&.test_results + Gitlab::Ci::Reports::TestResultsComparer.new(base_pipeline&.test_results, actual_head_pipeline&.test_results) end def all_commits @@ -1135,6 +1131,12 @@ class MergeRequest < ActiveRecord::Base true end + def base_pipeline + @base_pipeline ||= project.pipelines + .order(id: :desc) + .find_by(sha: diff_base_sha) + end + def discussions_rendered_on_frontend? true end diff --git a/app/serializers/merge_request_widget_entity.rb b/app/serializers/merge_request_widget_entity.rb index a78bd77cf7c..fd9f9dcc9af 100644 --- a/app/serializers/merge_request_widget_entity.rb +++ b/app/serializers/merge_request_widget_entity.rb @@ -225,6 +225,12 @@ class MergeRequestWidgetEntity < IssuableEntity end end + expose :test_results_path do |merge_request| + if merge_request.has_test_results? + test_results_project_merge_request_path(merge_request.project, merge_request, format: :json) + end + end + private delegate :current_user, to: :request diff --git a/app/serializers/test_case_entity.rb b/app/serializers/test_case_entity.rb new file mode 100644 index 00000000000..8213b6d4179 --- /dev/null +++ b/app/serializers/test_case_entity.rb @@ -0,0 +1,6 @@ +class TestCaseEntity < Grape::Entity + expose :name + expose :time, as: :execution_time + expose :failure_reason, as: :system_output + # TODO: stack_trace +end diff --git a/app/serializers/test_results_comparer_entity.rb b/app/serializers/test_results_comparer_entity.rb new file mode 100644 index 00000000000..2e17ecb7ee2 --- /dev/null +++ b/app/serializers/test_results_comparer_entity.rb @@ -0,0 +1,3 @@ +class TestResultsComparerEntity < Grape::Entity + expose :suites, using: TestSuiteComparerEntity +end diff --git a/app/serializers/test_results_comparer_serializer.rb b/app/serializers/test_results_comparer_serializer.rb new file mode 100644 index 00000000000..53983d63b0d --- /dev/null +++ b/app/serializers/test_results_comparer_serializer.rb @@ -0,0 +1,3 @@ +class TestResultsComparerSerializer < BaseSerializer + entity TestResultsComparerEntity +end diff --git a/app/serializers/test_suite_comparer_entity.rb b/app/serializers/test_suite_comparer_entity.rb new file mode 100644 index 00000000000..af4ef90786d --- /dev/null +++ b/app/serializers/test_suite_comparer_entity.rb @@ -0,0 +1,6 @@ +class TestSuiteComparerEntity < Grape::Entity + expose :name + expose :new_failures, using: TestCaseEntity + expose :resolved_failures, using: TestCaseEntity + expose :existing_failures, using: TestCaseEntity +end diff --git a/config/routes/project.rb b/config/routes/project.rb index 5057e937941..be0c1feca62 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -109,6 +109,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do post :assign_related_issues get :discussions, format: :json post :rebase + get :test_results scope constraints: { format: nil }, action: :show do get :commits, defaults: { tab: 'commits' } diff --git a/lib/gitlab/ci/reports/test_results_comparer.rb b/lib/gitlab/ci/reports/test_results_comparer.rb new file mode 100644 index 00000000000..a60917b30f7 --- /dev/null +++ b/lib/gitlab/ci/reports/test_results_comparer.rb @@ -0,0 +1,24 @@ +module Gitlab + module Ci + module Reports + class TestResultsComparer + include Gitlab::Utils::StrongMemoize + + attr_reader :base_results, :head_results + + def initialize(base_results, head_results) + @base_results = base_results || TestResults.new + @head_results = head_results + end + + def suites + strong_memoize(:suites) do + head_results.suites.map do |name, test_suite| + TestSuiteComparer.new(name, base_results.suites[name], test_suite) + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/reports/test_suite.rb b/lib/gitlab/ci/reports/test_suite.rb index d2ac4ebb361..e51f526a489 100644 --- a/lib/gitlab/ci/reports/test_suite.rb +++ b/lib/gitlab/ci/reports/test_suite.rb @@ -6,13 +6,17 @@ module Gitlab attr_reader :test_statuses attr_reader :total_time - def initialize(name) + def initialize(name = nil) @name = name @test_statuses = {} @total_time = 0.0 end TestCase::STATUSES.each do |result_type| + define_method("#{result_type}") do + test_statuses[result_type.to_sym] || {} + end + define_method("#{result_type}_count") do test_statuses[result_type.to_sym]&.length.to_i end diff --git a/lib/gitlab/ci/reports/test_suite_comparer.rb b/lib/gitlab/ci/reports/test_suite_comparer.rb new file mode 100644 index 00000000000..fc25b7a57f4 --- /dev/null +++ b/lib/gitlab/ci/reports/test_suite_comparer.rb @@ -0,0 +1,41 @@ +module Gitlab + module Ci + module Reports + class TestSuiteComparer + include Gitlab::Utils::StrongMemoize + + attr_reader :name, :base_suite, :head_suite + + def initialize(name, base_suite, head_suite) + @name = name + @base_suite = base_suite || TestSuite.new + @head_suite = head_suite + end + + def new_failures + strong_memoize(:new_failures) do + head_suite.failed.reject do |key, _| + base_suite.failed.include?(key) + end.values + end + end + + def existing_failures + strong_memoize(:existing_failures) do + head_suite.failed.select do |key, _| + base_suite.failed.include?(key) + end.values + end + end + + def resolved_failures + strong_memoize(:resolved_failures) do + head_suite.success.select do |key, _| + base_suite.failed.include?(key) + end.values + end + end + end + end + end +end -- cgit v1.2.1 From 5a8d4930e0127aae311bfa3da70d9ab9637791e3 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 18 Jul 2018 15:43:28 +0900 Subject: Evaluate artifact_format --- lib/api/runner.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 255b9114c4d..03bfcd50be0 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -235,6 +235,8 @@ module API optional :expire_in, type: String, desc: %q(Specify when artifacts should expire) optional :artifact_type, type: String, desc: %q(The type of artifact), default: 'archive', values: Ci::JobArtifact.file_types.keys + optional :artifact_format, type: String, desc: %q(The format of artifact), + default: 'zip', values: Ci::JobArtifact.file_formats.keys optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse)) optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse)) @@ -265,7 +267,7 @@ module API project: job.project, file: artifacts, file_type: params['artifact_type'], - file_format: :zip, + file_format: params['artifact_format'], file_sha256: artifacts.sha256, expire_in: expire_in) -- cgit v1.2.1 From 47dfdc732e850fd1390d25d50b1ef7a99491770a Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 18 Jul 2018 16:43:20 +0900 Subject: Fix fixtures for development --- db/fixtures/development/14_pipelines.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/db/fixtures/development/14_pipelines.rb b/db/fixtures/development/14_pipelines.rb index 6d938dc2b0e..7aa87babaa8 100644 --- a/db/fixtures/development/14_pipelines.rb +++ b/db/fixtures/development/14_pipelines.rb @@ -81,7 +81,9 @@ class Gitlab::Seeder::Pipelines branch = merge_request.source_branch merge_request.commits.last(4).map do |commit| - create_pipeline!(project, branch, commit) + create_pipeline!(project, branch, commit).tap do |pipeline| + merge_request.update!(head_pipeline_id: pipeline.id) + end end end @@ -92,7 +94,7 @@ class Gitlab::Seeder::Pipelines def create_pipeline!(project, ref, commit) - project.pipelines.create(sha: commit.id, ref: ref, source: :push) + project.pipelines.create!(sha: commit.id, ref: ref, source: :push) end def build_create!(pipeline, opts = {}) @@ -125,6 +127,8 @@ class Gitlab::Seeder::Pipelines build.job_artifacts.build(project: build.project, file_type: :archive_metadata, file_format: :gzip, file: file) end + return unless %w[test].include?(build.stage) + if build.ref == build.project.default_branch artifacts_cache_file(artifacts_junit_master_path) do |file| build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) -- cgit v1.2.1 From 1859a0f9e0c0f8f10ba640a8826b9ccade9fd15f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Wed, 18 Jul 2018 12:17:49 +0200 Subject: Add summary to test results json --- app/serializers/test_results_comparer_entity.rb | 6 ++++++ app/serializers/test_suite_comparer_entity.rb | 7 +++++++ lib/gitlab/ci/reports/test_results_comparer.rb | 8 ++++++++ lib/gitlab/ci/reports/test_suite_comparer.rb | 18 ++++++++++++++++++ 4 files changed, 39 insertions(+) diff --git a/app/serializers/test_results_comparer_entity.rb b/app/serializers/test_results_comparer_entity.rb index 2e17ecb7ee2..395dc689dda 100644 --- a/app/serializers/test_results_comparer_entity.rb +++ b/app/serializers/test_results_comparer_entity.rb @@ -1,3 +1,9 @@ class TestResultsComparerEntity < Grape::Entity + expose :summary do + expose :total_count, as: :total + expose :resolved_count, as: :resolved + expose :failed_count, as: :failed + end + expose :suites, using: TestSuiteComparerEntity end diff --git a/app/serializers/test_suite_comparer_entity.rb b/app/serializers/test_suite_comparer_entity.rb index af4ef90786d..ed6c2581ce0 100644 --- a/app/serializers/test_suite_comparer_entity.rb +++ b/app/serializers/test_suite_comparer_entity.rb @@ -1,5 +1,12 @@ class TestSuiteComparerEntity < Grape::Entity expose :name + + expose :summary do + expose :total_count, as: :total + expose :resolved_count, as: :resolved + expose :failed_count, as: :failed + end + expose :new_failures, using: TestCaseEntity expose :resolved_failures, using: TestCaseEntity expose :existing_failures, using: TestCaseEntity diff --git a/lib/gitlab/ci/reports/test_results_comparer.rb b/lib/gitlab/ci/reports/test_results_comparer.rb index a60917b30f7..752bb9f9914 100644 --- a/lib/gitlab/ci/reports/test_results_comparer.rb +++ b/lib/gitlab/ci/reports/test_results_comparer.rb @@ -18,6 +18,14 @@ module Gitlab end end end + + %w(total_count resolved_count failed_count).each do |method| + define_method(method) do + strong_memoize(method) do + suites.sum { |suite| suite.public_send(method) } + end + end + end end end end diff --git a/lib/gitlab/ci/reports/test_suite_comparer.rb b/lib/gitlab/ci/reports/test_suite_comparer.rb index fc25b7a57f4..64d29d3a5fe 100644 --- a/lib/gitlab/ci/reports/test_suite_comparer.rb +++ b/lib/gitlab/ci/reports/test_suite_comparer.rb @@ -35,6 +35,24 @@ module Gitlab end.values end end + + def total_count + strong_memoize(:total_count) do + head_suite.total_count + end + end + + def resolved_count + strong_memoize(:resolved_count) do + resolved_failures.count + end + end + + def failed_count + strong_memoize(:failed) do + new_failures.count + existing_failures.count + end + end end end end -- cgit v1.2.1 From 26578902d097979ca32a6453a33d699209077aa5 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 18 Jul 2018 20:59:01 +0900 Subject: Add fixtures which can export all kind of reports (new/resolved/exisiting) --- app/models/ci/job_artifact.rb | 2 +- app/presenters/ci/build_presenter.rb | 2 +- db/fixtures/development/14_pipelines.rb | 37 +++++++++++++++++---------- spec/fixtures/junit/junit_feature_0_3.xml.gz | Bin 0 -> 541 bytes spec/fixtures/junit/junit_feature_1_3.xml.gz | Bin 0 -> 522 bytes spec/fixtures/junit/junit_feature_2_3.xml.gz | Bin 0 -> 359 bytes spec/fixtures/junit/junit_master_0_3.xml.gz | Bin 0 -> 592 bytes spec/fixtures/junit/junit_master_1_3.xml.gz | Bin 0 -> 367 bytes spec/fixtures/junit/junit_master_2_3.xml.gz | Bin 0 -> 360 bytes 9 files changed, 25 insertions(+), 16 deletions(-) create mode 100644 spec/fixtures/junit/junit_feature_0_3.xml.gz create mode 100644 spec/fixtures/junit/junit_feature_1_3.xml.gz create mode 100644 spec/fixtures/junit/junit_feature_2_3.xml.gz create mode 100644 spec/fixtures/junit/junit_master_0_3.xml.gz create mode 100644 spec/fixtures/junit/junit_master_1_3.xml.gz create mode 100644 spec/fixtures/junit/junit_master_2_3.xml.gz diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index e7ab74cbd3d..414b6c4be89 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -69,7 +69,7 @@ module Ci file_format_adapter_class.new(stream).each_blob(&blk) end end - + private def file_format_adapter_class diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index c4e0baa0603..6b52c91749f 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -67,7 +67,7 @@ module Ci end def config_artifacts_reports_junit(junit) - { name: 'junit.xml', paths: junit, type: :junit, format: :gzip } + { name: 'junit.xml', paths: junit, type: :junit, format: :gzip, when: :always } end end end diff --git a/db/fixtures/development/14_pipelines.rb b/db/fixtures/development/14_pipelines.rb index 7aa87babaa8..d602b0309e8 100644 --- a/db/fixtures/development/14_pipelines.rb +++ b/db/fixtures/development/14_pipelines.rb @@ -106,7 +106,8 @@ class Gitlab::Seeder::Pipelines # (id required), that is why we need `#tap` method instead of passing # block directly to `Ci::Build#create!`. - setup_artifacts(build) + setup_artifacts(build) if %w[build test].include?(build.stage) + setup_test_reports(build) if %w[test].include?(build.stage) setup_build_log(build) build.project.environments. @@ -117,8 +118,6 @@ class Gitlab::Seeder::Pipelines end def setup_artifacts(build) - return unless %w[build test].include?(build.stage) - artifacts_cache_file(artifacts_archive_path) do |file| build.job_artifacts.build(project: build.project, file_type: :archive, file_format: :zip, file: file) end @@ -126,16 +125,20 @@ class Gitlab::Seeder::Pipelines artifacts_cache_file(artifacts_metadata_path) do |file| build.job_artifacts.build(project: build.project, file_type: :archive_metadata, file_format: :gzip, file: file) end + end - return unless %w[test].include?(build.stage) - + def setup_test_reports(build) if build.ref == build.project.default_branch - artifacts_cache_file(artifacts_junit_master_path) do |file| - build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) + artifacts_junit_master_path(build.name).try do |path| + artifacts_cache_file(path) do |file| + build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) + end end else - artifacts_cache_file(artifacts_junit_feature_path) do |file| - build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) + artifacts_junit_feature_path(build.name).try do |path| + artifacts_cache_file(path) do |file| + build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) + end end end end @@ -179,12 +182,18 @@ class Gitlab::Seeder::Pipelines Rails.root + 'spec/fixtures/ci_build_artifacts_metadata.gz' end - def artifacts_junit_master_path - Rails.root + 'spec/fixtures/junit/junit_master.xml.gz' + def artifacts_junit_master_path(build_name) + index, total = build_name.scan(/ (\d) (\d)/).first + return unless index && total + + Rails.root + "spec/fixtures/junit/junit_master_#{index}_#{total}.xml.gz" end - def artifacts_junit_feature_path - Rails.root + 'spec/fixtures/junit/junit_feature.xml.gz' + def artifacts_junit_feature_path(build_name) + index, total = build_name.scan(/ (\d) (\d)/).first + return unless index && total + + Rails.root + "spec/fixtures/junit/junit_feature_#{index}_#{total}.xml.gz" end def artifacts_cache_file(file_path) @@ -198,7 +207,7 @@ class Gitlab::Seeder::Pipelines end Gitlab::Seeder.quiet do - Project.all.sample(1).each do |project| + Project.all.sample(5).each do |project| project_builds = Gitlab::Seeder::Pipelines.new(project) project_builds.seed! end diff --git a/spec/fixtures/junit/junit_feature_0_3.xml.gz b/spec/fixtures/junit/junit_feature_0_3.xml.gz new file mode 100644 index 00000000000..a41d4f3b9e2 Binary files /dev/null and b/spec/fixtures/junit/junit_feature_0_3.xml.gz differ diff --git a/spec/fixtures/junit/junit_feature_1_3.xml.gz b/spec/fixtures/junit/junit_feature_1_3.xml.gz new file mode 100644 index 00000000000..68091f62a05 Binary files /dev/null and b/spec/fixtures/junit/junit_feature_1_3.xml.gz differ diff --git a/spec/fixtures/junit/junit_feature_2_3.xml.gz b/spec/fixtures/junit/junit_feature_2_3.xml.gz new file mode 100644 index 00000000000..ca740a35084 Binary files /dev/null and b/spec/fixtures/junit/junit_feature_2_3.xml.gz differ diff --git a/spec/fixtures/junit/junit_master_0_3.xml.gz b/spec/fixtures/junit/junit_master_0_3.xml.gz new file mode 100644 index 00000000000..187f2cba63b Binary files /dev/null and b/spec/fixtures/junit/junit_master_0_3.xml.gz differ diff --git a/spec/fixtures/junit/junit_master_1_3.xml.gz b/spec/fixtures/junit/junit_master_1_3.xml.gz new file mode 100644 index 00000000000..405c695cea0 Binary files /dev/null and b/spec/fixtures/junit/junit_master_1_3.xml.gz differ diff --git a/spec/fixtures/junit/junit_master_2_3.xml.gz b/spec/fixtures/junit/junit_master_2_3.xml.gz new file mode 100644 index 00000000000..aca6bcb8e18 Binary files /dev/null and b/spec/fixtures/junit/junit_master_2_3.xml.gz differ -- cgit v1.2.1 From be2083ff9cceacd6bdd64a9521081278111400a7 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 15:31:42 +0900 Subject: Add java ant Junit test report in fixtures --- db/fixtures/development/14_pipelines.rb | 38 +++++++++++++++------ lib/gitlab/ci/parsers/junit_parser.rb | 2 ++ spec/fixtures/junit/junit_feature_0_3.xml.gz | Bin 541 -> 0 bytes spec/fixtures/junit/junit_feature_1_3.xml.gz | Bin 522 -> 0 bytes spec/fixtures/junit/junit_feature_2_3.xml.gz | Bin 359 -> 0 bytes spec/fixtures/junit/junit_feature_ant.xml.gz | Bin 0 -> 1823 bytes spec/fixtures/junit/junit_feature_rspec_0_3.xml.gz | Bin 0 -> 541 bytes spec/fixtures/junit/junit_feature_rspec_1_3.xml.gz | Bin 0 -> 522 bytes spec/fixtures/junit/junit_feature_rspec_2_3.xml.gz | Bin 0 -> 359 bytes spec/fixtures/junit/junit_master_0_3.xml.gz | Bin 592 -> 0 bytes spec/fixtures/junit/junit_master_1_3.xml.gz | Bin 367 -> 0 bytes spec/fixtures/junit/junit_master_2_3.xml.gz | Bin 360 -> 0 bytes spec/fixtures/junit/junit_master_ant.xml.gz | Bin 0 -> 1975 bytes spec/fixtures/junit/junit_master_rspec_0_3.xml.gz | Bin 0 -> 592 bytes spec/fixtures/junit/junit_master_rspec_1_3.xml.gz | Bin 0 -> 367 bytes spec/fixtures/junit/junit_master_rspec_2_3.xml.gz | Bin 0 -> 360 bytes 16 files changed, 29 insertions(+), 11 deletions(-) delete mode 100644 spec/fixtures/junit/junit_feature_0_3.xml.gz delete mode 100644 spec/fixtures/junit/junit_feature_1_3.xml.gz delete mode 100644 spec/fixtures/junit/junit_feature_2_3.xml.gz create mode 100644 spec/fixtures/junit/junit_feature_ant.xml.gz create mode 100644 spec/fixtures/junit/junit_feature_rspec_0_3.xml.gz create mode 100644 spec/fixtures/junit/junit_feature_rspec_1_3.xml.gz create mode 100644 spec/fixtures/junit/junit_feature_rspec_2_3.xml.gz delete mode 100644 spec/fixtures/junit/junit_master_0_3.xml.gz delete mode 100644 spec/fixtures/junit/junit_master_1_3.xml.gz delete mode 100644 spec/fixtures/junit/junit_master_2_3.xml.gz create mode 100644 spec/fixtures/junit/junit_master_ant.xml.gz create mode 100644 spec/fixtures/junit/junit_master_rspec_0_3.xml.gz create mode 100644 spec/fixtures/junit/junit_master_rspec_1_3.xml.gz create mode 100644 spec/fixtures/junit/junit_master_rspec_2_3.xml.gz diff --git a/db/fixtures/development/14_pipelines.rb b/db/fixtures/development/14_pipelines.rb index d602b0309e8..e80a32903be 100644 --- a/db/fixtures/development/14_pipelines.rb +++ b/db/fixtures/development/14_pipelines.rb @@ -30,6 +30,8 @@ class Gitlab::Seeder::Pipelines queued_at: 8.hour.ago, started_at: 8.hour.ago, finished_at: 7.hour.ago }, { name: 'spinach:osx', stage: 'test', status: :failed, allow_failure: true, queued_at: 8.hour.ago, started_at: 8.hour.ago, finished_at: 7.hour.ago }, + { name: 'java ant', stage: 'test', status: :failed, allow_failure: true, + queued_at: 8.hour.ago, started_at: 8.hour.ago, finished_at: 7.hour.ago }, # deploy stage { name: 'staging', stage: 'deploy', environment: 'staging', status_event: :success, @@ -129,14 +131,24 @@ class Gitlab::Seeder::Pipelines def setup_test_reports(build) if build.ref == build.project.default_branch - artifacts_junit_master_path(build.name).try do |path| - artifacts_cache_file(path) do |file| + if build.name.include?('rspec:linux') + artifacts_cache_file(artifacts_rspec_junit_master_path(build.name)) do |file| + build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) + end + elsif build.name.include?('java ant') + artifacts_cache_file(artifacts_ant_junit_master_path) do |file| build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) end end else - artifacts_junit_feature_path(build.name).try do |path| - artifacts_cache_file(path) do |file| + if build.name.include?('rspec:linux') + artifacts_rspec_junit_feature_path(build.name).try do |path| + artifacts_cache_file(path) do |file| + build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) + end + end + elsif build.name.include?('java ant') + artifacts_cache_file(artifacts_ant_junit_feature_path) do |file| build.job_artifacts.build(project: build.project, file_type: :junit, file_format: :gzip, file: file) end end @@ -182,18 +194,22 @@ class Gitlab::Seeder::Pipelines Rails.root + 'spec/fixtures/ci_build_artifacts_metadata.gz' end - def artifacts_junit_master_path(build_name) + def artifacts_rspec_junit_master_path(build_name) index, total = build_name.scan(/ (\d) (\d)/).first - return unless index && total - - Rails.root + "spec/fixtures/junit/junit_master_#{index}_#{total}.xml.gz" + Rails.root + "spec/fixtures/junit/junit_master_rspec_#{index}_#{total}.xml.gz" end - def artifacts_junit_feature_path(build_name) + def artifacts_rspec_junit_feature_path(build_name) index, total = build_name.scan(/ (\d) (\d)/).first - return unless index && total + Rails.root + "spec/fixtures/junit/junit_feature_rspec_#{index}_#{total}.xml.gz" + end + + def artifacts_ant_junit_master_path + Rails.root + "spec/fixtures/junit/junit_master_ant.xml.gz" + end - Rails.root + "spec/fixtures/junit/junit_feature_#{index}_#{total}.xml.gz" + def artifacts_ant_junit_feature_path + Rails.root + "spec/fixtures/junit/junit_feature_ant.xml.gz" end def artifacts_cache_file(file_path) diff --git a/lib/gitlab/ci/parsers/junit_parser.rb b/lib/gitlab/ci/parsers/junit_parser.rb index f554ff9a20f..0ead2121aab 100644 --- a/lib/gitlab/ci/parsers/junit_parser.rb +++ b/lib/gitlab/ci/parsers/junit_parser.rb @@ -15,6 +15,8 @@ module Gitlab test_suite.add_result(test_case) end end + rescue + Rails.logger.error "Failed to parse Junit file" # Since xml_data is user-generated contents, parser could fail if they include corrupted-data end private diff --git a/spec/fixtures/junit/junit_feature_0_3.xml.gz b/spec/fixtures/junit/junit_feature_0_3.xml.gz deleted file mode 100644 index a41d4f3b9e2..00000000000 Binary files a/spec/fixtures/junit/junit_feature_0_3.xml.gz and /dev/null differ diff --git a/spec/fixtures/junit/junit_feature_1_3.xml.gz b/spec/fixtures/junit/junit_feature_1_3.xml.gz deleted file mode 100644 index 68091f62a05..00000000000 Binary files a/spec/fixtures/junit/junit_feature_1_3.xml.gz and /dev/null differ diff --git a/spec/fixtures/junit/junit_feature_2_3.xml.gz b/spec/fixtures/junit/junit_feature_2_3.xml.gz deleted file mode 100644 index ca740a35084..00000000000 Binary files a/spec/fixtures/junit/junit_feature_2_3.xml.gz and /dev/null differ diff --git a/spec/fixtures/junit/junit_feature_ant.xml.gz b/spec/fixtures/junit/junit_feature_ant.xml.gz new file mode 100644 index 00000000000..33c8b0cc240 Binary files /dev/null and b/spec/fixtures/junit/junit_feature_ant.xml.gz differ diff --git a/spec/fixtures/junit/junit_feature_rspec_0_3.xml.gz b/spec/fixtures/junit/junit_feature_rspec_0_3.xml.gz new file mode 100644 index 00000000000..a41d4f3b9e2 Binary files /dev/null and b/spec/fixtures/junit/junit_feature_rspec_0_3.xml.gz differ diff --git a/spec/fixtures/junit/junit_feature_rspec_1_3.xml.gz b/spec/fixtures/junit/junit_feature_rspec_1_3.xml.gz new file mode 100644 index 00000000000..68091f62a05 Binary files /dev/null and b/spec/fixtures/junit/junit_feature_rspec_1_3.xml.gz differ diff --git a/spec/fixtures/junit/junit_feature_rspec_2_3.xml.gz b/spec/fixtures/junit/junit_feature_rspec_2_3.xml.gz new file mode 100644 index 00000000000..ca740a35084 Binary files /dev/null and b/spec/fixtures/junit/junit_feature_rspec_2_3.xml.gz differ diff --git a/spec/fixtures/junit/junit_master_0_3.xml.gz b/spec/fixtures/junit/junit_master_0_3.xml.gz deleted file mode 100644 index 187f2cba63b..00000000000 Binary files a/spec/fixtures/junit/junit_master_0_3.xml.gz and /dev/null differ diff --git a/spec/fixtures/junit/junit_master_1_3.xml.gz b/spec/fixtures/junit/junit_master_1_3.xml.gz deleted file mode 100644 index 405c695cea0..00000000000 Binary files a/spec/fixtures/junit/junit_master_1_3.xml.gz and /dev/null differ diff --git a/spec/fixtures/junit/junit_master_2_3.xml.gz b/spec/fixtures/junit/junit_master_2_3.xml.gz deleted file mode 100644 index aca6bcb8e18..00000000000 Binary files a/spec/fixtures/junit/junit_master_2_3.xml.gz and /dev/null differ diff --git a/spec/fixtures/junit/junit_master_ant.xml.gz b/spec/fixtures/junit/junit_master_ant.xml.gz new file mode 100644 index 00000000000..0741dc6526b Binary files /dev/null and b/spec/fixtures/junit/junit_master_ant.xml.gz differ diff --git a/spec/fixtures/junit/junit_master_rspec_0_3.xml.gz b/spec/fixtures/junit/junit_master_rspec_0_3.xml.gz new file mode 100644 index 00000000000..187f2cba63b Binary files /dev/null and b/spec/fixtures/junit/junit_master_rspec_0_3.xml.gz differ diff --git a/spec/fixtures/junit/junit_master_rspec_1_3.xml.gz b/spec/fixtures/junit/junit_master_rspec_1_3.xml.gz new file mode 100644 index 00000000000..405c695cea0 Binary files /dev/null and b/spec/fixtures/junit/junit_master_rspec_1_3.xml.gz differ diff --git a/spec/fixtures/junit/junit_master_rspec_2_3.xml.gz b/spec/fixtures/junit/junit_master_rspec_2_3.xml.gz new file mode 100644 index 00000000000..aca6bcb8e18 Binary files /dev/null and b/spec/fixtures/junit/junit_master_rspec_2_3.xml.gz differ -- cgit v1.2.1 From 20e95824341af1ebc5877d28dc5eba26f73eddf9 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 16:28:00 +0900 Subject: Fix spec --- lib/api/entities.rb | 4 ++-- lib/api/runner.rb | 2 +- spec/factories/ci/builds.rb | 7 +++++++ spec/services/ci/retry_build_service_spec.rb | 5 +++-- spec/services/projects/update_pages_service_spec.rb | 2 +- 5 files changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 8c671e7badd..84453666606 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1072,7 +1072,7 @@ module API end class Job < JobBasic - expose :artifacts_file, using: JobArtifactFile, if: -> (job, opts) { job.artifacts_archive? } + expose :artifacts_file, using: JobArtifactFile, if: -> (job, opts) { job.artifacts? } expose :runner, with: Runner expose :artifacts_expire_at end @@ -1242,7 +1242,7 @@ module API class Dependency < Grape::Entity expose :id, :name, :token - expose :artifacts_file, using: JobArtifactFile, if: ->(job, _) { job.artifacts_archive? } + expose :artifacts_file, using: JobArtifactFile, if: ->(job, _) { job.artifacts? } end class Response < Grape::Entity diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 03bfcd50be0..80a12788714 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -236,7 +236,7 @@ module API optional :artifact_type, type: String, desc: %q(The type of artifact), default: 'archive', values: Ci::JobArtifact.file_types.keys optional :artifact_format, type: String, desc: %q(The format of artifact), - default: 'zip', values: Ci::JobArtifact.file_formats.keys + default: 'zip', values: Ci::JobArtifact.file_formats.keys optional 'file.path', type: String, desc: %q(path to locally stored body (generated by Workhorse)) optional 'file.name', type: String, desc: %q(real filename as send in Content-Disposition (generated by Workhorse)) optional 'file.type', type: String, desc: %q(real content type as send in Content-Type (generated by Workhorse)) diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 99f14a08039..db280bded6c 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -187,6 +187,13 @@ FactoryBot.define do end end + trait :test_reports do + after(:create) do |build| + create(:ci_job_artifact, :junit, job: build) + build.reload + end + end + trait :expired do artifacts_expire_at 1.minute.ago end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 41899740081..a69032507dd 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -24,7 +24,7 @@ describe Ci::RetryBuildService do artifacts_file artifacts_metadata artifacts_size created_at updated_at started_at finished_at queued_at erased_by erased_at auto_canceled_by job_artifacts job_artifacts_archive - job_artifacts_archive_metadata job_artifacts_trace].freeze + job_artifacts_archive_metadata job_artifacts_trace job_artifacts_junit].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections @@ -38,7 +38,7 @@ describe Ci::RetryBuildService do let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } let(:build) do - create(:ci_build, :failed, :artifacts, :expired, :erased, + create(:ci_build, :failed, :artifacts, :test_reports, :expired, :erased, :queued, :coverage, :tags, :allowed_to_fail, :on_tag, :triggered, :trace_artifact, :teardown_environment, description: 'my-job', stage: 'test', stage_id: stage.id, @@ -76,6 +76,7 @@ describe Ci::RetryBuildService do describe 'reject acessors' do REJECT_ACCESSORS.each do |attribute| it "does not clone #{attribute} build attribute" do + binding.pry expect(new_build.send(attribute)).not_to eq build.send(attribute) end end diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index a4c103e6f30..d9174e6846c 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -79,7 +79,7 @@ describe Projects::UpdatePagesService do context "for a valid job" do before do create(:ci_job_artifact, file: file, job: build) - create(:ci_job_artifact, file_type: :metadata, file: metadata, job: build) + create(:ci_job_artifact, file_type: :archive_metadata, file: metadata, job: build) build.reload end -- cgit v1.2.1 From e54707fdf97392839cb2c4711160bd3bc89da196 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 16:49:27 +0900 Subject: Fix erase method --- app/models/ci/build.rb | 16 ++++++++++++---- app/models/ci/job_artifact.rb | 3 +++ app/workers/expire_build_instance_artifacts_worker.rb | 1 + spec/fixtures/junit.xml.gz | Bin 0 -> 568 bytes spec/services/ci/retry_build_service_spec.rb | 1 - 5 files changed, 16 insertions(+), 5 deletions(-) create mode 100644 spec/fixtures/junit.xml.gz diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index ac73aad590a..0e5dfb1bd02 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -387,6 +387,10 @@ module Ci trace.exist? end + def has_test_reports? + job_artifacts.test_reports.any? + end + def has_old_trace? old_trace.present? end @@ -449,22 +453,26 @@ module Ci end def erase_artifacts! - remove_legacy_artifacts_file! if legacy_artifacts_file - remove_legacy_artifacts_metadata! if legacy_artifacts_metadata - job_artifacts.destroy_all + remove_artifacts_file! + remove_artifacts_metadata! save end + def erase_test_reports! + job_artifacts.test_reports.destroy_all + end + def erase(opts = {}) return false unless erasable? erase_artifacts! + erase_test_reports! erase_trace! update_erased!(opts[:erased_by]) end def erasable? - complete? && (artifacts? || has_trace?) + complete? && (artifacts? || has_test_reports? || has_trace?) end def erased? diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 2c9f7e98483..e8fc24da9f6 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -17,6 +17,7 @@ module Ci after_save :update_file_store, if: :file_changed? scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } + scope :test_reports, -> { where(file_type: TEST_REPORTS_FILE_TYPES) } delegate :exists?, :open, to: :file @@ -27,6 +28,8 @@ module Ci junit: 4 } + TEST_REPORTS_FILE_TYPES = [4].freeze # junit + enum file_format: { raw: 1, zip: 2, diff --git a/app/workers/expire_build_instance_artifacts_worker.rb b/app/workers/expire_build_instance_artifacts_worker.rb index 3b57ecb36e3..b6d5c98fa70 100644 --- a/app/workers/expire_build_instance_artifacts_worker.rb +++ b/app/workers/expire_build_instance_artifacts_worker.rb @@ -13,5 +13,6 @@ class ExpireBuildInstanceArtifactsWorker Rails.logger.info "Removing artifacts for build #{build.id}..." build.erase_artifacts! + build.erase_test_reports! end end diff --git a/spec/fixtures/junit.xml.gz b/spec/fixtures/junit.xml.gz new file mode 100644 index 00000000000..88b7de6fa61 Binary files /dev/null and b/spec/fixtures/junit.xml.gz differ diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index a69032507dd..fc66288e3cf 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -76,7 +76,6 @@ describe Ci::RetryBuildService do describe 'reject acessors' do REJECT_ACCESSORS.each do |attribute| it "does not clone #{attribute} build attribute" do - binding.pry expect(new_build.send(attribute)).not_to eq build.send(attribute) end end -- cgit v1.2.1 From e31121cb5e617b0f05e375c2150ece0e38e5e0d6 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 17:57:15 +0900 Subject: Impolement job_artifact.test_reports method --- app/models/ci/job_artifact.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index e8fc24da9f6..d0aba6f6652 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -17,7 +17,12 @@ module Ci after_save :update_file_store, if: :file_changed? scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } - scope :test_reports, -> { where(file_type: TEST_REPORTS_FILE_TYPES) } + + scope :test_reports, -> do + types = self.file_types.select { |k, v| TEST_REPORTS_FILE_TYPES.include?(k) }.values + + where(file_type: types) + end delegate :exists?, :open, to: :file @@ -28,7 +33,7 @@ module Ci junit: 4 } - TEST_REPORTS_FILE_TYPES = [4].freeze # junit + TEST_REPORTS_FILE_TYPES = %w[junit].freeze enum file_format: { raw: 1, -- cgit v1.2.1 From a3930853c93862007ba6814511bc32042c7f4986 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 18:15:41 +0900 Subject: Increment migration version to use `file_format` when archiving traces --- spec/lib/gitlab/background_migration/archive_legacy_traces_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/background_migration/archive_legacy_traces_spec.rb b/spec/lib/gitlab/background_migration/archive_legacy_traces_spec.rb index 877c061d11b..a9b07eb3122 100644 --- a/spec/lib/gitlab/background_migration/archive_legacy_traces_spec.rb +++ b/spec/lib/gitlab/background_migration/archive_legacy_traces_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::BackgroundMigration::ArchiveLegacyTraces, :migration, schema: 20180529152628 do +describe Gitlab::BackgroundMigration::ArchiveLegacyTraces, :migration, schema: 20180705160945 do include TraceHelpers let(:namespaces) { table(:namespaces) } -- cgit v1.2.1 From 95502e605af9bcf1a61dbeb26f9be4d181f8a7ba Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 18:23:41 +0900 Subject: Fix artifact migratable --- app/models/concerns/artifact_migratable.rb | 4 ++-- spec/requests/api/jobs_spec.rb | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index ca9aba5ac3a..5ce7d693249 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -31,8 +31,8 @@ module ArtifactMigratable end def remove_artifacts_metadata! - if job_artifacts_metadata - job_artifacts_metadata.destroy + if job_artifacts_archive_metadata + job_artifacts_archive_metadata.destroy else remove_legacy_artifacts_metadata! end diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index 7d1a5c12805..46d9a363b02 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -654,13 +654,15 @@ describe API::Jobs do end context 'job is erasable' do - let(:job) { create(:ci_build, :trace_artifact, :artifacts, :success, project: project, pipeline: pipeline) } + let(:job) { create(:ci_build, :trace_artifact, :artifacts, :test_reports, :success, project: project, pipeline: pipeline) } it 'erases job content' do expect(response).to have_gitlab_http_status(201) + expect(job.job_artifacts.count).to eq(0) expect(job.trace.exist?).to be_falsy expect(job.artifacts_file.exists?).to be_falsy expect(job.artifacts_metadata.exists?).to be_falsy + expect(job.has_test_reports?).to be_falsy end it 'updates job' do -- cgit v1.2.1 From a2cda62fb922184aaf0e78699e06846c96565e0d Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 18:27:11 +0900 Subject: Fix presenter spec --- app/presenters/ci/build_presenter.rb | 4 ++-- spec/presenters/ci/build_presenter_spec.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index c4e0baa0603..d9650589f4c 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -56,7 +56,7 @@ module Ci def config_artifacts_archive(artifacts) artifacts.merge(type: :archive, format: :zip) - end + end def config_artifacts_reports(reports) list = [] @@ -67,7 +67,7 @@ module Ci end def config_artifacts_reports_junit(junit) - { name: 'junit.xml', paths: junit, type: :junit, format: :gzip } + { name: 'junit.xml', paths: junit, type: :junit, format: :gzip, when: :always } end end end diff --git a/spec/presenters/ci/build_presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb index 78d00c4e14a..08fd35ccf4f 100644 --- a/spec/presenters/ci/build_presenter_spec.rb +++ b/spec/presenters/ci/build_presenter_spec.rb @@ -266,7 +266,7 @@ describe Ci::BuildPresenter do let(:build) { create(:ci_build, options: { artifacts: { reports: { junit: ['junit.xml'] } } } ) } it 'presents artifacts hash' do - expect(presenter.config_artifacts).to include({ type: :junit, format: :gzip, paths: ['junit.xml'] }) + expect(presenter.config_artifacts).to include({ name: 'junit.xml', type: :junit, format: :gzip, paths: ['junit.xml'], when: :always }) end end @@ -275,7 +275,7 @@ describe Ci::BuildPresenter do it 'presents artifacts hash' do expect(presenter.config_artifacts).to include( - { type: :junit, format: :gzip, paths: ['junit.xml'] }, + { name: 'junit.xml', type: :junit, format: :gzip, paths: ['junit.xml'], when: :always }, { type: :archive, format: :zip, paths: ['sample.txt'] }) end end -- cgit v1.2.1 From 8a576b18c8ab8ead2344e2885aaf2fde11af0328 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 20:40:06 +0900 Subject: Fix spec --- app/presenters/ci/build_presenter.rb | 2 +- lib/gitlab/ci/config/entry/artifacts.rb | 2 +- spec/models/ci/build_spec.rb | 6 +++--- spec/requests/api/runner_spec.rb | 4 +++- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index d9650589f4c..6b52c91749f 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -56,7 +56,7 @@ module Ci def config_artifacts_archive(artifacts) artifacts.merge(type: :archive, format: :zip) - end + end def config_artifacts_reports(reports) list = [] diff --git a/lib/gitlab/ci/config/entry/artifacts.rb b/lib/gitlab/ci/config/entry/artifacts.rb index ca80dbd85a2..05788f62c72 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -37,7 +37,7 @@ module Gitlab helpers :reports def value - @config[:reports] = reports_value + @config[:reports] = reports_value if @config.key?(:reports) @config end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 7c3ddab1751..3c722148f66 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -861,7 +861,7 @@ describe Ci::Build do let!(:build) { create(:ci_build, :success, :artifacts) } before do - build.job_artifacts_metadata.destroy + build.job_artifacts_archive_metadata.destroy end describe '#erase' do @@ -2700,7 +2700,7 @@ describe Ci::Build do subject { build.artifacts_metadata_entry(path) } context 'when using local storage' do - let!(:metadata) { create(:ci_job_artifact, :metadata, job: build) } + let!(:metadata) { create(:ci_job_artifact, :archive_metadata, job: build) } context 'for existing file' do it 'does exist' do @@ -2720,7 +2720,7 @@ describe Ci::Build do context 'when using remote storage' do include HttpIOHelpers - let!(:metadata) { create(:ci_job_artifact, :remote_store, :metadata, job: build) } + let!(:metadata) { create(:ci_job_artifact, :remote_store, :archive_metadata, job: build) } let(:file_path) { expand_fixture_path('ci_build_artifacts_metadata.gz') } before do diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 401ba7959bc..1c8b09529b0 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -424,7 +424,9 @@ describe API::Runner, :clean_gitlab_redis_shared_state do 'untracked' => false, 'paths' => %w(out/), 'when' => 'always', - 'expire_in' => '7d' }] + 'expire_in' => '7d', + "artifact_type" => "archive", + "artifact_format" => "zip" }] end let(:expected_cache) do -- cgit v1.2.1 From 55bc71a404d8cf5fa87e187f6e88da92ab95afa9 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 20:45:19 +0900 Subject: Use array_of_strings_or_string in Command --- lib/gitlab/ci/config/entry/commands.rb | 13 +------------ spec/lib/gitlab/ci/config/entry/commands_spec.rb | 3 +-- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/lib/gitlab/ci/config/entry/commands.rb b/lib/gitlab/ci/config/entry/commands.rb index 65d19db249c..9f66f11be9b 100644 --- a/lib/gitlab/ci/config/entry/commands.rb +++ b/lib/gitlab/ci/config/entry/commands.rb @@ -9,18 +9,7 @@ module Gitlab include Validatable validations do - include LegacyValidationHelpers - - validate do - unless string_or_array_of_strings?(config) - errors.add(:config, - 'should be a string or an array of strings') - end - end - - def string_or_array_of_strings?(field) - validate_string(field) || validate_array_of_strings(field) - end + validates :config, array_of_strings_or_string: true end def value diff --git a/spec/lib/gitlab/ci/config/entry/commands_spec.rb b/spec/lib/gitlab/ci/config/entry/commands_spec.rb index afa4a089418..84734b06caa 100644 --- a/spec/lib/gitlab/ci/config/entry/commands_spec.rb +++ b/spec/lib/gitlab/ci/config/entry/commands_spec.rb @@ -41,8 +41,7 @@ describe Gitlab::Ci::Config::Entry::Commands do describe '#errors' do it 'saves errors' do expect(entry.errors) - .to include 'commands config should be a ' \ - 'string or an array of strings' + .to include 'commands config should be an array of strings or string' end end end -- cgit v1.2.1 From c79f361ca01f8dbc0d395edee5fab7f5a0697934 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 20:53:00 +0900 Subject: Rever archive_metadata refactoring (For simplifying) --- app/models/ci/job_artifact.rb | 2 +- app/models/concerns/artifact_migratable.rb | 6 +++--- spec/factories/ci/builds.rb | 2 +- spec/factories/ci/job_artifacts.rb | 4 ++-- spec/models/ci/build_spec.rb | 6 +++--- spec/requests/api/runner_spec.rb | 2 +- spec/services/ci/retry_build_service_spec.rb | 2 +- spec/services/projects/update_pages_service_spec.rb | 2 +- 8 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index d0aba6f6652..c9ce042a1c5 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -28,7 +28,7 @@ module Ci enum file_type: { archive: 1, - archive_metadata: 2, + metadata: 2, trace: 3, junit: 4 } diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 5ce7d693249..ff52ca64459 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -7,7 +7,7 @@ module ArtifactMigratable end def artifacts_metadata - job_artifacts_archive_metadata&.file || legacy_artifacts_metadata + job_artifacts_metadata&.file || legacy_artifacts_metadata end def artifacts? @@ -31,8 +31,8 @@ module ArtifactMigratable end def remove_artifacts_metadata! - if job_artifacts_archive_metadata - job_artifacts_archive_metadata.destroy + if job_artifacts_metadata + job_artifacts_metadata.destroy else remove_legacy_artifacts_metadata! end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index db280bded6c..8bd1f1ae4e0 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -182,7 +182,7 @@ FactoryBot.define do trait :artifacts do after(:create) do |build| create(:ci_job_artifact, :archive, job: build, expire_at: build.artifacts_expire_at) - create(:ci_job_artifact, :archive_metadata, job: build, expire_at: build.artifacts_expire_at) + create(:ci_job_artifact, :metadata, job: build, expire_at: build.artifacts_expire_at) build.reload end end diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb index 631e92f7338..a6a87782fe7 100644 --- a/spec/factories/ci/job_artifacts.rb +++ b/spec/factories/ci/job_artifacts.rb @@ -24,8 +24,8 @@ FactoryBot.define do end end - trait :archive_metadata do - file_type :archive_metadata + trait :metadata do + file_type :metadata file_format :gzip after(:build) do |artifact, _| diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 3c722148f66..7c3ddab1751 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -861,7 +861,7 @@ describe Ci::Build do let!(:build) { create(:ci_build, :success, :artifacts) } before do - build.job_artifacts_archive_metadata.destroy + build.job_artifacts_metadata.destroy end describe '#erase' do @@ -2700,7 +2700,7 @@ describe Ci::Build do subject { build.artifacts_metadata_entry(path) } context 'when using local storage' do - let!(:metadata) { create(:ci_job_artifact, :archive_metadata, job: build) } + let!(:metadata) { create(:ci_job_artifact, :metadata, job: build) } context 'for existing file' do it 'does exist' do @@ -2720,7 +2720,7 @@ describe Ci::Build do context 'when using remote storage' do include HttpIOHelpers - let!(:metadata) { create(:ci_job_artifact, :remote_store, :archive_metadata, job: build) } + let!(:metadata) { create(:ci_job_artifact, :remote_store, :metadata, job: build) } let(:file_path) { expand_fixture_path('ci_build_artifacts_metadata.gz') } before do diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 1c8b09529b0..77efb877662 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -1382,7 +1382,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do let(:stored_metadata_file) { job.reload.artifacts_metadata.file } let(:stored_artifacts_size) { job.reload.artifacts_size } let(:stored_artifacts_sha256) { job.reload.job_artifacts_archive.file_sha256 } - let(:stored_metadata_sha256) { job.reload.job_artifacts_archive_metadata.file_sha256 } + let(:stored_metadata_sha256) { job.reload.job_artifacts_metadata.file_sha256 } before do post(api("/jobs/#{job.id}/artifacts"), post_data, headers_with_token) diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index fc66288e3cf..18d52082399 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -24,7 +24,7 @@ describe Ci::RetryBuildService do artifacts_file artifacts_metadata artifacts_size created_at updated_at started_at finished_at queued_at erased_by erased_at auto_canceled_by job_artifacts job_artifacts_archive - job_artifacts_archive_metadata job_artifacts_trace job_artifacts_junit].freeze + job_artifacts_metadata job_artifacts_trace job_artifacts_junit].freeze IGNORE_ACCESSORS = %i[type lock_version target_url base_tags trace_sections diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index d9174e6846c..a4c103e6f30 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -79,7 +79,7 @@ describe Projects::UpdatePagesService do context "for a valid job" do before do create(:ci_job_artifact, file: file, job: build) - create(:ci_job_artifact, file_type: :archive_metadata, file: metadata, job: build) + create(:ci_job_artifact, file_type: :metadata, file: metadata, job: build) build.reload end -- cgit v1.2.1 From d9beb10ede5e4e8abe388fadbd6412640293917a Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 20:57:03 +0900 Subject: Remove scattering around erase_test_reports! --- app/models/ci/build.rb | 2 +- app/workers/expire_build_instance_artifacts_worker.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 0e5dfb1bd02..4b6c65f4223 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -455,6 +455,7 @@ module Ci def erase_artifacts! remove_artifacts_file! remove_artifacts_metadata! + erase_test_reports! save end @@ -466,7 +467,6 @@ module Ci return false unless erasable? erase_artifacts! - erase_test_reports! erase_trace! update_erased!(opts[:erased_by]) end diff --git a/app/workers/expire_build_instance_artifacts_worker.rb b/app/workers/expire_build_instance_artifacts_worker.rb index b6d5c98fa70..3b57ecb36e3 100644 --- a/app/workers/expire_build_instance_artifacts_worker.rb +++ b/app/workers/expire_build_instance_artifacts_worker.rb @@ -13,6 +13,5 @@ class ExpireBuildInstanceArtifactsWorker Rails.logger.info "Removing artifacts for build #{build.id}..." build.erase_artifacts! - build.erase_test_reports! end end -- cgit v1.2.1 From d88523ca88420354f61bd36f533c62a6ca474423 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 19 Jul 2018 21:00:40 +0900 Subject: Revert unnecessary change --- spec/models/ci/build_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 7c3ddab1751..ee923374480 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -861,7 +861,7 @@ describe Ci::Build do let!(:build) { create(:ci_build, :success, :artifacts) } before do - build.job_artifacts_metadata.destroy + build.remove_artifacts_metadata! end describe '#erase' do @@ -930,7 +930,7 @@ describe Ci::Build do let!(:build) { create(:ci_build, :success, :legacy_artifacts) } before do - build.remove_legacy_artifacts_metadata! + build.remove_artifacts_metadata! end describe '#erase' do -- cgit v1.2.1 From 9ecaee914defba5f12a7a06375ea2876b4328d7f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 20 Jul 2018 14:27:54 +0900 Subject: Introduce ARCHIVE_LEGACY_TRACES_MIGRATION_VERSION check --- app/models/ci/job_artifact.rb | 6 +++++- lib/api/runner.rb | 2 +- lib/gitlab/ci/trace.rb | 30 ++++++++++++++++++++++++------ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index c9ce042a1c5..e31662efae3 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -9,7 +9,7 @@ module Ci mount_uploader :file, JobArtifactUploader - validates :file_format, presence: true, on: :create + validates :file_format, presence: true, unless: :ignore_schema, on: :create before_save :set_size, if: :file_changed? after_save :update_project_statistics_after_save, if: :size_changed? after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed? @@ -41,6 +41,10 @@ module Ci gzip: 3 } + def ignore_schema + ActiveRecord::Migrator.current_version <= ::Gitlab::Ci::Trace::ARCHIVE_LEGACY_TRACES_MIGRATION_VERSION + end + def update_file_store # The file.object_store is set during `uploader.store!` # which happens after object is inserted/updated diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 80a12788714..a7d3db57bac 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -275,7 +275,7 @@ module API job.job_artifacts.build( project: job.project, file: metadata, - file_type: "#{params['artifact_type']}_metadata", + file_type: :metadata, file_format: :gzip, file_sha256: metadata.sha256, expire_in: expire_in) diff --git a/lib/gitlab/ci/trace.rb b/lib/gitlab/ci/trace.rb index 769d998227c..ee0adc790d8 100644 --- a/lib/gitlab/ci/trace.rb +++ b/lib/gitlab/ci/trace.rb @@ -162,14 +162,32 @@ module Gitlab end end + ## + # NOTE: + # In 11.0, we shipped a post migration to archive legacy traces. In the migration script, + # `Trace#archive!` method was directly used for simplying the migration logic. + # In 11.2, we created a new column in `ci_job_artifacts` table and started saving a value to the column, + # however, this brought up a problem that if users bump their GitLab instance version from 10.7 to 11.2, + # then their legacy trace migrations are going to require the new column to be present, even though 11.2's migration has not run yet. + # https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/20390#note_89084352 + ARCHIVE_LEGACY_TRACES_MIGRATION_VERSION = 20180529152628 + def create_build_trace!(job, path) File.open(path) do |stream| - job.create_job_artifacts_trace!( - project: job.project, - file_type: :trace, - file_format: :raw, - file: stream, - file_sha256: Digest::SHA256.file(path).hexdigest) + if ActiveRecord::Migrator.current_version <= ARCHIVE_LEGACY_TRACES_MIGRATION_VERSION + job.create_job_artifacts_trace!( + project: job.project, + file_type: :trace, + file: stream, + file_sha256: Digest::SHA256.file(path).hexdigest) + else + job.create_job_artifacts_trace!( + project: job.project, + file_type: :trace, + file_format: :raw, + file: stream, + file_sha256: Digest::SHA256.file(path).hexdigest) + end end end -- cgit v1.2.1 From 75f75b3f5988398fff0660ca5f04aec756ab03bb Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 20 Jul 2018 15:42:16 +0900 Subject: Implement config artifact presenter --- app/models/ci/job_artifact.rb | 2 ++ app/presenters/ci/build_presenter.rb | 41 ++++++++++++++--------------- lib/api/entities.rb | 4 +-- lib/gitlab/ci/config/entry/artifacts.rb | 46 +++++++++++++++++++++++++++++++++ lib/gitlab/ci/config/entry/reports.rb | 12 +++++++++ 5 files changed, 81 insertions(+), 24 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index e31662efae3..c4f1e900615 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -33,7 +33,9 @@ module Ci junit: 4 } + GENERAL_FILE_TYPES = %w[archive].freeze TEST_REPORTS_FILE_TYPES = %w[junit].freeze + DEFAULT_FILE_NAMES = { junit: 'junit.xml' }.freeze enum file_format: { raw: 1, diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index 6b52c91749f..cca05771a08 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -34,14 +34,27 @@ module Ci end def config_artifacts - list = [] + [].tap do |list| + next unless options[:artifacts] - options.dig(:artifacts).try do |artifacts| - list << config_artifacts_reports(artifacts.delete(:reports)) if artifacts.dig(:reports) - list << config_artifacts_archive(artifacts) if artifacts.dig(:paths) - end + options[:artifacts].except(:reports).try do |archive| + ::Gitlab::Ci::Config::Entry::Artifacts.new(archive).present_for_runner.tap do |runner_config| + raise ArgumentError, 'General artifact config for runner is not valid' unless ::Gitlab::Ci::Config::Entry::ArtifactsForRunner.new(runner_config).valid? + + list << runner_config + end + end + + options[:artifacts].dig(:reports).try do |reports| + ::Gitlab::Ci::Config::Entry::Reports.new(reports).present_for_runner.tap do |runner_configs| + list << runner_configs.map do |runner_config| + raise ArgumentError, 'Report-type artifact config for runner is not valid' unless ::Gitlab::Ci::Config::Entry::ArtifactsForRunner.new(runner_config).valid? - list.flatten + runner_config + end + end + end + end.flatten end private @@ -53,21 +66,5 @@ module Ci def detailed_status @detailed_status ||= subject.detailed_status(user) end - - def config_artifacts_archive(artifacts) - artifacts.merge(type: :archive, format: :zip) - end - - def config_artifacts_reports(reports) - list = [] - - list << config_artifacts_reports_junit(reports.dig(:junit)) if reports.dig(:junit) - - list - end - - def config_artifacts_reports_junit(junit) - { name: 'junit.xml', paths: junit, type: :junit, format: :gzip, when: :always } - end end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 84453666606..f5db70a9466 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1228,8 +1228,8 @@ module API class Artifacts < Grape::Entity expose :name, :untracked, :paths, :when, :expire_in - expose :type, as: :artifact_type - expose :format, as: :artifact_format + expose :artifact_type + expose :artifact_format end class Cache < Grape::Entity diff --git a/lib/gitlab/ci/config/entry/artifacts.rb b/lib/gitlab/ci/config/entry/artifacts.rb index 05788f62c72..bc5013dba6a 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -40,6 +40,52 @@ module Gitlab @config[:reports] = reports_value if @config.key?(:reports) @config end + + def present_for_runner + @config.merge(artifact_type: :archive, artifact_format: :zip) + end + end + + class ArtifactsForRunner < Node + include Validatable + include Attributable + + ALLOWED_KEYS = %i[name untracked paths when expire_in artifact_type artifact_format].freeze + + attributes ALLOWED_KEYS + + validations do + validates :config, type: Hash + validates :config, allowed_keys: ALLOWED_KEYS + validates :artifact_type, type: Symbol + validates :artifact_format, type: Symbol + + with_options if: :general_artifact? do + with_options allow_nil: true do + validates :name, type: String + validates :untracked, boolean: true + validates :paths, array_of_strings: true + validates :when, inclusion: { in: %w[on_success on_failure always] } + validates :expire_in, duration: true + end + end + + with_options if: :report_artifact? do + validates :name, type: String + validates :untracked, presence: false + validates :paths, array_of_strings: true + validates :when, inclusion: { in: %w[always] } + validates :expire_in, presence: false + end + end + + def general_artifact? + ::Ci::JobArtifact::GENERAL_FILE_TYPES.include?(@config[:artifact_type]) + end + + def report_artifact? + ::Ci::JobArtifact::TEST_REPORTS_FILE_TYPES.include?(@config[:artifact_type]) + end end end end diff --git a/lib/gitlab/ci/config/entry/reports.rb b/lib/gitlab/ci/config/entry/reports.rb index 5963f3eb90c..fcc28a42521 100644 --- a/lib/gitlab/ci/config/entry/reports.rb +++ b/lib/gitlab/ci/config/entry/reports.rb @@ -25,6 +25,18 @@ module Gitlab def value @config.transform_values { |v| Array(v) } end + + def present_for_runner + @config.map do |k, v| + { + name: ::Ci::JobArtifact::DEFAULT_FILE_NAMES[k.to_sym], + paths: v, + artifact_type: k.to_sym, + artifact_format: :gzip, + when: 'always' + } + end + end end end end -- cgit v1.2.1 From 402ae97ecf7f9e3fe541f2d6abef6e47ab740452 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 20 Jul 2018 15:46:56 +0900 Subject: Make GENERAL_ARCHIVE_FILE_TYPE as a single entry --- app/models/ci/job_artifact.rb | 6 +++--- lib/gitlab/ci/config/entry/artifacts.rb | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index c4f1e900615..6fc2d9e8282 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -19,7 +19,7 @@ module Ci scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) } scope :test_reports, -> do - types = self.file_types.select { |k, v| TEST_REPORTS_FILE_TYPES.include?(k) }.values + types = self.file_types.select { |k, _| TEST_REPORT_FILE_TYPES.include?(k) }.values where(file_type: types) end @@ -33,8 +33,8 @@ module Ci junit: 4 } - GENERAL_FILE_TYPES = %w[archive].freeze - TEST_REPORTS_FILE_TYPES = %w[junit].freeze + GENERAL_ARCHIVE_FILE_TYPE = 'archive'.freeze + TEST_REPORT_FILE_TYPES = %w[junit].freeze DEFAULT_FILE_NAMES = { junit: 'junit.xml' }.freeze enum file_format: { diff --git a/lib/gitlab/ci/config/entry/artifacts.rb b/lib/gitlab/ci/config/entry/artifacts.rb index bc5013dba6a..b276f7ec8d1 100644 --- a/lib/gitlab/ci/config/entry/artifacts.rb +++ b/lib/gitlab/ci/config/entry/artifacts.rb @@ -80,11 +80,11 @@ module Gitlab end def general_artifact? - ::Ci::JobArtifact::GENERAL_FILE_TYPES.include?(@config[:artifact_type]) + ::Ci::JobArtifact::GENERAL_ARCHIVE_FILE_TYPE == @config[:artifact_type] end def report_artifact? - ::Ci::JobArtifact::TEST_REPORTS_FILE_TYPES.include?(@config[:artifact_type]) + ::Ci::JobArtifact::TEST_REPORT_FILE_TYPES.include?(@config[:artifact_type]) end end end -- cgit v1.2.1 From 36e69897b0524cdee6060c928c03af734afae664 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 20 Jul 2018 16:02:09 +0900 Subject: Erase test reports at the proper timing --- app/models/ci/build.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 4b6c65f4223..0e5dfb1bd02 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -455,7 +455,6 @@ module Ci def erase_artifacts! remove_artifacts_file! remove_artifacts_metadata! - erase_test_reports! save end @@ -467,6 +466,7 @@ module Ci return false unless erasable? erase_artifacts! + erase_test_reports! erase_trace! update_erased!(opts[:erased_by]) end -- cgit v1.2.1 From 3c92a22faf6278e7a2d1ee13bd978bc659b72452 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 20 Jul 2018 16:07:21 +0900 Subject: Fix build presenter spec --- spec/presenters/ci/build_presenter_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/presenters/ci/build_presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb index 08fd35ccf4f..8f1763193d7 100644 --- a/spec/presenters/ci/build_presenter_spec.rb +++ b/spec/presenters/ci/build_presenter_spec.rb @@ -258,7 +258,7 @@ describe Ci::BuildPresenter do let(:build) { create(:ci_build, options: { artifacts: { paths: ['sample.txt'] } } ) } it 'presents artifacts hash' do - expect(presenter.config_artifacts).to include({ type: :archive, format: :zip, paths: ['sample.txt'] }) + expect(presenter.config_artifacts).to include({ artifact_type: :archive, artifact_format: :zip, paths: ['sample.txt'] }) end end @@ -266,7 +266,7 @@ describe Ci::BuildPresenter do let(:build) { create(:ci_build, options: { artifacts: { reports: { junit: ['junit.xml'] } } } ) } it 'presents artifacts hash' do - expect(presenter.config_artifacts).to include({ name: 'junit.xml', type: :junit, format: :gzip, paths: ['junit.xml'], when: :always }) + expect(presenter.config_artifacts).to include({ name: 'junit.xml', artifact_type: :junit, artifact_format: :gzip, paths: ['junit.xml'], when: 'always' }) end end @@ -275,8 +275,8 @@ describe Ci::BuildPresenter do it 'presents artifacts hash' do expect(presenter.config_artifacts).to include( - { name: 'junit.xml', type: :junit, format: :gzip, paths: ['junit.xml'], when: :always }, - { type: :archive, format: :zip, paths: ['sample.txt'] }) + { name: 'junit.xml', artifact_type: :junit, artifact_format: :gzip, paths: ['junit.xml'], when: 'always' }, + { artifact_type: :archive, artifact_format: :zip, paths: ['sample.txt'] }) end end -- cgit v1.2.1 From 3d85788edbe73fc74c72854508e47fe259d99236 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 20 Jul 2018 18:05:31 +0900 Subject: Checking filr_format and file_type paring --- app/models/ci/job_artifact.rb | 8 ++++++ spec/models/ci/job_artifact_spec.rb | 28 ++++++++++++++------- spec/requests/api/runner_spec.rb | 50 +++++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 6fc2d9e8282..a736e2b0e22 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -10,6 +10,7 @@ module Ci mount_uploader :file, JobArtifactUploader validates :file_format, presence: true, unless: :ignore_schema, on: :create + validate :valid_file_format?, unless: :ignore_schema, on: :create before_save :set_size, if: :file_changed? after_save :update_project_statistics_after_save, if: :size_changed? after_destroy :update_project_statistics_after_destroy, unless: :project_destroyed? @@ -36,6 +37,7 @@ module Ci GENERAL_ARCHIVE_FILE_TYPE = 'archive'.freeze TEST_REPORT_FILE_TYPES = %w[junit].freeze DEFAULT_FILE_NAMES = { junit: 'junit.xml' }.freeze + TYPE_AND_FORMAT_PAIRS = { archive: :zip, metadata: :gzip, trace: :raw, junit: :gzip }.freeze enum file_format: { raw: 1, @@ -43,6 +45,12 @@ module Ci gzip: 3 } + def valid_file_format? + unless TYPE_AND_FORMAT_PAIRS[self.file_type&.to_sym] == self.file_format&.to_sym + errors.add(:file_format, 'Invalid file format with specified file type') + end + end + def ignore_schema ActiveRecord::Migrator.current_version <= ::Gitlab::Ci::Trace::ARCHIVE_LEGACY_TRACES_MIGRATION_VERSION end diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb index 594211c288f..4474df73434 100644 --- a/spec/models/ci/job_artifact_spec.rb +++ b/spec/models/ci/job_artifact_spec.rb @@ -90,25 +90,35 @@ describe Ci::JobArtifact do describe 'validates file format' do subject { artifact.valid? } - let(:artifact) { build(:ci_job_artifact, file_format: file_format) } - - context 'when file format is present' do - let(:file_format) { :raw } + context 'when archive type with zip format' do + let(:artifact) { build(:ci_job_artifact, :archive, file_format: :zip) } it { is_expected.to be_truthy } end - context 'when file format is not defined' do - let(:file_format) { :new_format } + context 'when archive type with gzip format' do + let(:artifact) { build(:ci_job_artifact, :archive, file_format: :gzip) } + + it { is_expected.to be_falsy } + end + + context 'when archive type without format specification' do + let(:artifact) { build(:ci_job_artifact, :archive, file_format: nil) } - it { expect { subject }.to raise_error(ArgumentError) } + it { is_expected.to be_falsy } end - context 'when file format is not present' do - let(:file_format) { nil } + context 'when junit type with zip format' do + let(:artifact) { build(:ci_job_artifact, :junit, file_format: :zip) } it { is_expected.to be_falsy } end + + context 'when junit type with gzip format' do + let(:artifact) { build(:ci_job_artifact, :junit, file_format: :gzip) } + + it { is_expected.to be_truthy } + end end describe '#file' do diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 77efb877662..0f41e46cdd5 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -1422,6 +1422,56 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end end end + + context 'when artifact_type is archive' do + context 'when artifact_format is zip' do + let(:params) { { artifact_type: :archive, artifact_format: :zip } } + + it 'stores junit test report' do + upload_artifacts(file_upload, headers_with_token, params) + + expect(response).to have_gitlab_http_status(201) + expect(job.reload.job_artifacts_archive).not_to be_nil + end + end + + context 'when artifact_format is gzip' do + let(:params) { { artifact_type: :archive, artifact_format: :gzip } } + + it 'returns an error' do + upload_artifacts(file_upload, headers_with_token, params) + + expect(response).to have_gitlab_http_status(400) + expect(job.reload.job_artifacts_archive).to be_nil + end + end + end + + context 'when artifact_type is junit' do + context 'when artifact_format is gzip' do + let(:file_upload) { fixture_file_upload('spec/fixtures/junit.xml.gz') } + let(:params) { { artifact_type: :junit, artifact_format: :gzip } } + + it 'stores junit test report' do + upload_artifacts(file_upload, headers_with_token, params) + + expect(response).to have_gitlab_http_status(201) + expect(job.reload.job_artifacts_junit).not_to be_nil + end + end + + context 'when artifact_format is raw' do + let(:file_upload) { fixture_file_upload('spec/fixtures/junit.xml.gz') } + let(:params) { { artifact_type: :junit, artifact_format: :raw } } + + it 'returns an error' do + upload_artifacts(file_upload, headers_with_token, params) + + expect(response).to have_gitlab_http_status(400) + expect(job.reload.job_artifacts_junit).to be_nil + end + end + end end context 'when artifacts are being stored outside of tmp path' do -- cgit v1.2.1 From 59c4e31390e0d616d69babf8ac857e98f2dc774e Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 20 Jul 2018 18:14:44 +0900 Subject: Wrap long lines --- app/presenters/ci/build_presenter.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/presenters/ci/build_presenter.rb b/app/presenters/ci/build_presenter.rb index cca05771a08..5e3829693d1 100644 --- a/app/presenters/ci/build_presenter.rb +++ b/app/presenters/ci/build_presenter.rb @@ -39,7 +39,9 @@ module Ci options[:artifacts].except(:reports).try do |archive| ::Gitlab::Ci::Config::Entry::Artifacts.new(archive).present_for_runner.tap do |runner_config| - raise ArgumentError, 'General artifact config for runner is not valid' unless ::Gitlab::Ci::Config::Entry::ArtifactsForRunner.new(runner_config).valid? + unless ::Gitlab::Ci::Config::Entry::ArtifactsForRunner.new(runner_config).valid? + raise ArgumentError, 'General artifact config for runner is not valid' + end list << runner_config end @@ -48,7 +50,9 @@ module Ci options[:artifacts].dig(:reports).try do |reports| ::Gitlab::Ci::Config::Entry::Reports.new(reports).present_for_runner.tap do |runner_configs| list << runner_configs.map do |runner_config| - raise ArgumentError, 'Report-type artifact config for runner is not valid' unless ::Gitlab::Ci::Config::Entry::ArtifactsForRunner.new(runner_config).valid? + unless ::Gitlab::Ci::Config::Entry::ArtifactsForRunner.new(runner_config).valid? + raise ArgumentError, 'Report-type artifact config for runner is not valid' + end runner_config end -- cgit v1.2.1 From 681bd6a878ad2a77c278f5619b51c542d7382aa2 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 20 Jul 2018 18:19:46 +0900 Subject: Specify DOWNTIME=false --- db/migrate/20180705160945_add_file_format_to_ci_job_artifacts.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/db/migrate/20180705160945_add_file_format_to_ci_job_artifacts.rb b/db/migrate/20180705160945_add_file_format_to_ci_job_artifacts.rb index 9867506a925..2f0be9fa5db 100644 --- a/db/migrate/20180705160945_add_file_format_to_ci_job_artifacts.rb +++ b/db/migrate/20180705160945_add_file_format_to_ci_job_artifacts.rb @@ -1,5 +1,7 @@ # rubocop:disable all class AddFileFormatToCiJobArtifacts < ActiveRecord::Migration + DOWNTIME = false + def change add_column :ci_job_artifacts, :file_format, :integer, limit: 2 end -- cgit v1.2.1 From b2183151e6a7344a327883a2658030920e256e47 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 20 Jul 2018 19:06:46 +0900 Subject: Check pipeline status at has_test_results? --- app/models/ci/pipeline.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 0538f6f0ece..25393221005 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -604,7 +604,7 @@ module Ci end def has_test_results? - builds.with_test_reports.any? + complete? && builds.with_test_reports.any? end def test_results -- cgit v1.2.1 From ebebf4042bbcdcc32d5aa65b0da470ddc52f0f8e Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 20 Jul 2018 12:44:16 +0100 Subject: Initial structure for reports app --- .../components/grouped_test_reports_app.vue | 63 +++++++++++++++++ .../javascripts/reports/components/modal.vue | 80 ++++++++++++++++++++++ app/assets/javascripts/reports/constants.js | 60 ++++++++++++++++ 3 files changed, 203 insertions(+) create mode 100644 app/assets/javascripts/reports/components/grouped_test_reports_app.vue create mode 100644 app/assets/javascripts/reports/components/modal.vue create mode 100644 app/assets/javascripts/reports/constants.js diff --git a/app/assets/javascripts/reports/components/grouped_test_reports_app.vue b/app/assets/javascripts/reports/components/grouped_test_reports_app.vue new file mode 100644 index 00000000000..016806b84c3 --- /dev/null +++ b/app/assets/javascripts/reports/components/grouped_test_reports_app.vue @@ -0,0 +1,63 @@ + + + diff --git a/app/assets/javascripts/reports/components/modal.vue b/app/assets/javascripts/reports/components/modal.vue new file mode 100644 index 00000000000..61004399aa4 --- /dev/null +++ b/app/assets/javascripts/reports/components/modal.vue @@ -0,0 +1,80 @@ + + diff --git a/app/assets/javascripts/reports/constants.js b/app/assets/javascripts/reports/constants.js new file mode 100644 index 00000000000..36306f33153 --- /dev/null +++ b/app/assets/javascripts/reports/constants.js @@ -0,0 +1,60 @@ +import { s__ } from '~/locale'; + +export const fieldTypes = { + codeBock: 'codeBlock', + link: 'link', + miliseconds: 'miliseconds', + text: 'text', +}; + +/** + * Map of data show in modalbox + * + * Each key will only be rendered if `value` is present. + * + * Keys in this map have the same format as the backend json response. + * + * When the user clicks on the issue name, in modal_open_name, + * the store will set the modal data based on this map + * and the keys available in the store. + */ +export const modalData = { + class: { + value: null, + text: s__('Reports|Class'), + type: fieldTypes.link, + }, + execution_time: { + value: null, + text: s__('Reports|Execution time'), + type: fieldTypes.miliseconds, + }, + failure: { + value: null, + text: s__('Reports|Failure'), + type: fieldTypes.codeBock, + }, + system_output: { + value: null, + text: s__('Reports|System output'), + type: fieldTypes.codeBock, + }, +}; + +export const modalActions = [ + { + title: s__('Reports|Cancel'), + attributes: { + type: 'button', + class: 'btn btn-default', + 'data-dismiss': 'modal', + }, + }, + { + title: s__('Reports|Create Issue'), + attributes: { + type: 'button', + class: 'btn btn-success btn-inverted', + }, + } +]; -- cgit v1.2.1 From 920b74f519091000dc73a3be02c472ea226aa451 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 19 Jul 2018 12:49:44 +0100 Subject: Adds Vuex store for reports section in MR widget --- app/assets/javascripts/reports/store/actions.js | 68 ++++++++++ app/assets/javascripts/reports/store/index.js | 13 ++ .../javascripts/reports/store/mutation_types.js | 5 + app/assets/javascripts/reports/store/mutations.js | 28 ++++ app/assets/javascripts/reports/store/state.js | 28 ++++ changelogs/unreleased/45318-vuex-store.yml | 5 + spec/javascripts/reports/store/actions_spec.js | 149 +++++++++++++++++++++ spec/javascripts/reports/store/mutations_spec.js | 105 +++++++++++++++ 8 files changed, 401 insertions(+) create mode 100644 app/assets/javascripts/reports/store/actions.js create mode 100644 app/assets/javascripts/reports/store/index.js create mode 100644 app/assets/javascripts/reports/store/mutation_types.js create mode 100644 app/assets/javascripts/reports/store/mutations.js create mode 100644 app/assets/javascripts/reports/store/state.js create mode 100644 changelogs/unreleased/45318-vuex-store.yml create mode 100644 spec/javascripts/reports/store/actions_spec.js create mode 100644 spec/javascripts/reports/store/mutations_spec.js diff --git a/app/assets/javascripts/reports/store/actions.js b/app/assets/javascripts/reports/store/actions.js new file mode 100644 index 00000000000..72069489d20 --- /dev/null +++ b/app/assets/javascripts/reports/store/actions.js @@ -0,0 +1,68 @@ +import Visibility from 'visibilityjs'; +import axios from '../../lib/utils/axios_utils'; +import Poll from '../../lib/utils/poll'; +import * as types from './mutation_types'; + +export const setEndpoint = ({ commit }, endpoint) => commit(types.SET_ENDPOINT, endpoint); + +export const requestReports = ({ commit }) => commit(types.REQUEST_REPORTS); + +let eTagPoll; + +export const clearEtagPoll = () => { + eTagPoll = null; +}; + +export const stopPolling = () => { + if (eTagPoll) eTagPoll.stop(); +}; +export const restartPolling = () => { + if (eTagPoll) eTagPoll.restart(); +}; + +/** + * We need to poll the reports endpoint while they are being parsed in the Backend. + * This can take up to one minute. + * + * Poll.js will handle etag response. + * While http status code is 204, it means it's parsing, and we'll keep polling + * When http status code is 200, it means parsing is done, we can show the results & stop polling + * When http status code is 500, it means parsins went wrong and we stop polling + */ +export const fetchReports = ({ state, dispatch }) => { + dispatch('requestReports'); + + eTagPoll = new Poll({ + resource: { + getReports(endpoint) { + return axios.get(endpoint); + }, + }, + data: state.endpoint, + method: 'getReports', + successCallback: ({ data }) => dispatch('receiveReportsSuccess', data), + errorCallback: () => dispatch('receiveReportsError'), + }); + + if (!Visibility.hidden()) { + eTagPoll.makeRequest(); + } + + Visibility.change(() => { + if (!Visibility.hidden()) { + dispatch('restartPolling'); + } else { + dispatch('stopPolling'); + } + }); +}; + +export const receiveReportsSuccess = ({ commit }, response) => + commit(types.RECEIVE_REPORTS_SUCCESS, response); + +export const receiveReportsError = ({ commit }) => { + commit(types.RECEIVE_REPORTS_ERROR); +}; + +// prevent babel-plugin-rewire from generating an invalid default during karma tests +export default () => {}; diff --git a/app/assets/javascripts/reports/store/index.js b/app/assets/javascripts/reports/store/index.js new file mode 100644 index 00000000000..af4f9688fb4 --- /dev/null +++ b/app/assets/javascripts/reports/store/index.js @@ -0,0 +1,13 @@ +import Vue from 'vue'; +import Vuex from 'vuex'; +import * as actions from './actions'; +import mutations from './mutations'; +import state from './state'; + +Vue.use(Vuex); + +export default () => new Vuex.Store({ + actions, + mutations, + state: state(), +}); diff --git a/app/assets/javascripts/reports/store/mutation_types.js b/app/assets/javascripts/reports/store/mutation_types.js new file mode 100644 index 00000000000..77722974c45 --- /dev/null +++ b/app/assets/javascripts/reports/store/mutation_types.js @@ -0,0 +1,5 @@ +export const SET_ENDPOINT = 'SET_ENDPOINT'; + +export const REQUEST_REPORTS = 'REQUEST_REPORTS'; +export const RECEIVE_REPORTS_SUCCESS = 'RECEIVE_REPORTS_SUCCESS'; +export const RECEIVE_REPORTS_ERROR = 'RECEIVE_REPORTS_ERROR'; diff --git a/app/assets/javascripts/reports/store/mutations.js b/app/assets/javascripts/reports/store/mutations.js new file mode 100644 index 00000000000..9487b8d073a --- /dev/null +++ b/app/assets/javascripts/reports/store/mutations.js @@ -0,0 +1,28 @@ +/* eslint-disable no-param-reassign */ +import Vue from 'vue'; +import * as types from './mutation_types'; + +export default { + [types.SET_ENDPOINT](state, endpoint) { + state.endpoint = endpoint; + }, + [types.REQUEST_REPORTS](state) { + state.isLoading = true; + }, + [types.RECEIVE_REPORTS_SUCCESS](state, response) { + + state.isLoading = false; + state.hasError = false; + + Vue.set(state.summary, 'total', response.summary.total); + Vue.set(state.summary, 'resolved', response.summary.resolved); + Vue.set(state.summary, 'failed', response.summary.failed); + + state.reports = response.suites; + + }, + [types.RECEIVE_REPORTS_ERROR](state) { + state.isLoading = false; + state.hasError = true; + }, +}; diff --git a/app/assets/javascripts/reports/store/state.js b/app/assets/javascripts/reports/store/state.js new file mode 100644 index 00000000000..d4d50c1beab --- /dev/null +++ b/app/assets/javascripts/reports/store/state.js @@ -0,0 +1,28 @@ +export default () => ({ + endpoint: null, + + isLoading: false, + hasError: false, + + summary: { + total: 0, + resolved: 0, + failed: 0, + }, + + /** + * Each report will have the following format: + * { + * name: {String}, + * summary: { + * total: {Number}, + * resolved: {Number}, + * failed: {Number}, + * }, + * newFailures: {Array.}, + * resolvedFailures: {Array.}, + * existingFailures: {Array.}, + * } + */ + reports: [], +}); diff --git a/changelogs/unreleased/45318-vuex-store.yml b/changelogs/unreleased/45318-vuex-store.yml new file mode 100644 index 00000000000..5ea89034bce --- /dev/null +++ b/changelogs/unreleased/45318-vuex-store.yml @@ -0,0 +1,5 @@ +--- +title: Adds Vuex store for reports section in MR widget +merge_request: 20709 +author: +type: added diff --git a/spec/javascripts/reports/store/actions_spec.js b/spec/javascripts/reports/store/actions_spec.js new file mode 100644 index 00000000000..8bbb6f06182 --- /dev/null +++ b/spec/javascripts/reports/store/actions_spec.js @@ -0,0 +1,149 @@ +import MockAdapter from 'axios-mock-adapter'; +import axios from '~/lib/utils/axios_utils'; +import { + setEndpoint, + requestReports, + fetchReports, + stopPolling, + clearEtagPoll, + receiveReportsSuccess, + receiveReportsError, +} from '~/reports/store/actions'; +import state from '~/reports/store/state'; +import * as types from '~/reports/store/mutation_types'; +import testAction from 'spec/helpers/vuex_action_helper'; + +describe('Reports Store Actions', () => { + let mockedState; + + beforeEach(() => { + mockedState = state(); + }); + + describe('setEndpoint', () => { + it('should commit SET_ENDPOINT mutation', done => { + testAction( + setEndpoint, + 'endpoint.json', + mockedState, + [{ type: types.SET_ENDPOINT, payload: 'endpoint.json' }], + [], + done, + ); + }); + }); + + describe('requestReports', () => { + it('should commit REQUEST_REPORTS mutation', done => { + testAction(requestReports, null, mockedState, [{ type: types.REQUEST_REPORTS }], [], done); + }); + }); + + describe('fetchReports', () => { + let mock; + + beforeEach(() => { + mockedState.endpoint = 'endpoint.json'; + mock = new MockAdapter(axios); + }); + + afterEach(() => { + mock.restore(); + stopPolling(); + clearEtagPoll(); + }); + + describe('success', () => { + it('dispatches requestReports and receiveReportsSuccess ', done => { + mock.onGet('endpoint.json').reply(200, { summary: {}, suites: [{ name: 'rspec' }] }); + + testAction( + fetchReports, + null, + mockedState, + [], + [ + { + type: 'requestReports', + }, + { + type: 'receiveReportsSuccess', + }, + ], + done, + ); + }); + }); + + describe('error', () => { + beforeEach(() => { + mock.onGet(mockedState.endpoint).reply(500); + }); + + it('dispatches requestReports and receiveReportsError ', done => { + testAction( + fetchReports, + null, + mockedState, + [], + [ + { + type: 'requestReports', + }, + { + type: 'receiveReportsError', + }, + ], + done, + ); + }); + }); + + describe('no content', () => { + beforeEach(() => { + mock.onGet(mockedState.endpoint).reply(200); + }); + + it('dispatches requestReports and keeps polling ', done => { + testAction( + fetchReports, + null, + mockedState, + [], + [ + { + type: 'requestReports', + }, + ], + done, + ); + }); + }); + }); + + describe('receiveReportsSuccess', () => { + it('should commit RECEIVE_REPORTS_SUCCESS mutation', done => { + testAction( + receiveReportsSuccess, + { summary: {} }, + mockedState, + [{ type: types.RECEIVE_REPORTS_SUCCESS, payload: { summary: {} } }], + [], + done, + ); + }); + }); + + describe('receiveReportsError', () => { + it('should commit RECEIVE_REPORTS_ERROR mutation', done => { + testAction( + receiveReportsError, + null, + mockedState, + [{ type: types.RECEIVE_REPORTS_ERROR }], + [], + done, + ); + }); + }); +}); diff --git a/spec/javascripts/reports/store/mutations_spec.js b/spec/javascripts/reports/store/mutations_spec.js new file mode 100644 index 00000000000..90d2203cc66 --- /dev/null +++ b/spec/javascripts/reports/store/mutations_spec.js @@ -0,0 +1,105 @@ +import state from '~/reports/store/state'; +import mutations from '~/reports/store/mutations'; +import * as types from '~/reports/store/mutation_types'; + +describe('Reports Store Mutations', () => { + let stateCopy; + + beforeEach(() => { + stateCopy = state(); + }); + + describe('SET_ENDPOINT', () => { + it('should set endpoint', () => { + mutations[types.SET_ENDPOINT](stateCopy, 'endpoint.json'); + expect(stateCopy.endpoint).toEqual('endpoint.json'); + }); + }); + + describe('REQUEST_REPORTS', () => { + it('should set isLoading to true', () => { + mutations[types.REQUEST_REPORTS](stateCopy); + expect(stateCopy.isLoading).toEqual(true); + }); + }); + + describe('RECEIVE_REPORTS_SUCCESS', () => { + const mockedResponse = { + summary: { + total: 14, + resolved: 0, + failed: 7, + }, + suites: [ + { + name: 'build:linux', + summary: { + total: 2, + resolved: 0, + failed: 1, + }, + new_failures: [ + { + name: 'StringHelper#concatenate when a is git and b is lab returns summary', + execution_time: 0.0092435, + system_output: + 'Failure/Error: is_expected.to eq(\'gitlab\')', + }, + ], + resolved_failures: [ + { + name: 'StringHelper#concatenate when a is git and b is lab returns summary', + execution_time: 0.009235, + system_output: + 'Failure/Error: is_expected.to eq(\'gitlab\')', + }, + ], + existing_failures: [ + { + name: 'StringHelper#concatenate when a is git and b is lab returns summary', + execution_time: 1232.08, + system_output: + 'Failure/Error: is_expected.to eq(\'gitlab\')', + }, + ], + }, + ], + }; + + beforeEach(() => { + mutations[types.RECEIVE_REPORTS_SUCCESS](stateCopy, mockedResponse); + }); + + it('should reset isLoading', () => { + expect(stateCopy.isLoading).toEqual(false); + }); + + it('should reset hasError', () => { + expect(stateCopy.hasError).toEqual(false); + }); + + it('should set summary counts', () => { + expect(stateCopy.summary.total).toEqual(mockedResponse.summary.total); + expect(stateCopy.summary.resolved).toEqual(mockedResponse.summary.resolved); + expect(stateCopy.summary.failed).toEqual(mockedResponse.summary.failed); + }); + + it('should set reports', () => { + expect(stateCopy.reports).toEqual(mockedResponse.suites); + }); + }); + + describe('RECEIVE_REPORTS_ERROR', () => { + beforeEach(() => { + mutations[types.RECEIVE_REPORTS_ERROR](stateCopy); + }); + it('should reset isLoading', () => { + expect(stateCopy.isLoading).toEqual(false); + }); + + it('should set hasError to true', () => { + expect(stateCopy.hasError).toEqual(true); + }); + + }); +}); -- cgit v1.2.1 From a3eb4001181d6d0ae8f3a62d0452a06f67500cc6 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 19 Jul 2018 14:48:54 +0100 Subject: Updates documentation and test mock endpoint --- app/assets/javascripts/reports/store/state.js | 6 +++--- spec/javascripts/reports/store/actions_spec.js | 9 +++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/reports/store/state.js b/app/assets/javascripts/reports/store/state.js index d4d50c1beab..97f9d0a6859 100644 --- a/app/assets/javascripts/reports/store/state.js +++ b/app/assets/javascripts/reports/store/state.js @@ -19,9 +19,9 @@ export default () => ({ * resolved: {Number}, * failed: {Number}, * }, - * newFailures: {Array.}, - * resolvedFailures: {Array.}, - * existingFailures: {Array.}, + * new_failures: {Array.}, + * resolved_failures: {Array.}, + * existing_failures: {Array.}, * } */ reports: [], diff --git a/spec/javascripts/reports/store/actions_spec.js b/spec/javascripts/reports/store/actions_spec.js index 8bbb6f06182..887b1cc8b84 100644 --- a/spec/javascripts/reports/store/actions_spec.js +++ b/spec/javascripts/reports/store/actions_spec.js @@ -12,6 +12,7 @@ import { import state from '~/reports/store/state'; import * as types from '~/reports/store/mutation_types'; import testAction from 'spec/helpers/vuex_action_helper'; +import { TEST_HOST } from 'spec/test_constants'; describe('Reports Store Actions', () => { let mockedState; @@ -43,7 +44,7 @@ describe('Reports Store Actions', () => { let mock; beforeEach(() => { - mockedState.endpoint = 'endpoint.json'; + mockedState.endpoint = `${TEST_HOST}/endpoint.json`; mock = new MockAdapter(axios); }); @@ -55,7 +56,7 @@ describe('Reports Store Actions', () => { describe('success', () => { it('dispatches requestReports and receiveReportsSuccess ', done => { - mock.onGet('endpoint.json').reply(200, { summary: {}, suites: [{ name: 'rspec' }] }); + mock.onGet(`${TEST_HOST}/endpoint.json`).reply(200, { summary: {}, suites: [{ name: 'rspec' }] }); testAction( fetchReports, @@ -77,7 +78,7 @@ describe('Reports Store Actions', () => { describe('error', () => { beforeEach(() => { - mock.onGet(mockedState.endpoint).reply(500); + mock.onGet(`${TEST_HOST}/endpoint.json`).reply(500); }); it('dispatches requestReports and receiveReportsError ', done => { @@ -101,7 +102,7 @@ describe('Reports Store Actions', () => { describe('no content', () => { beforeEach(() => { - mock.onGet(mockedState.endpoint).reply(200); + mock.onGet(`${TEST_HOST}/endpoint.json`).reply(204); }); it('dispatches requestReports and keeps polling ', done => { -- cgit v1.2.1 From 04b473b68969a57d7c5fa33fe46c18bd9ee6bddf Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 19 Jul 2018 16:10:25 +0100 Subject: Adds payload to the fetchReports success spec --- spec/javascripts/reports/store/actions_spec.js | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/spec/javascripts/reports/store/actions_spec.js b/spec/javascripts/reports/store/actions_spec.js index 887b1cc8b84..b558a308133 100644 --- a/spec/javascripts/reports/store/actions_spec.js +++ b/spec/javascripts/reports/store/actions_spec.js @@ -65,6 +65,7 @@ describe('Reports Store Actions', () => { [], [ { + payload: { summary: {}, suites: [{ name: 'rspec' }] }, type: 'requestReports', }, { @@ -99,27 +100,6 @@ describe('Reports Store Actions', () => { ); }); }); - - describe('no content', () => { - beforeEach(() => { - mock.onGet(`${TEST_HOST}/endpoint.json`).reply(204); - }); - - it('dispatches requestReports and keeps polling ', done => { - testAction( - fetchReports, - null, - mockedState, - [], - [ - { - type: 'requestReports', - }, - ], - done, - ); - }); - }); }); describe('receiveReportsSuccess', () => { -- cgit v1.2.1 From 570e7713c76b247c6da886dc60edce10657558b1 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 20 Jul 2018 09:37:45 +0100 Subject: Moves payload to the correct action --- spec/javascripts/reports/store/actions_spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/javascripts/reports/store/actions_spec.js b/spec/javascripts/reports/store/actions_spec.js index b558a308133..fc2973de5bd 100644 --- a/spec/javascripts/reports/store/actions_spec.js +++ b/spec/javascripts/reports/store/actions_spec.js @@ -65,10 +65,10 @@ describe('Reports Store Actions', () => { [], [ { - payload: { summary: {}, suites: [{ name: 'rspec' }] }, type: 'requestReports', }, { + payload: { summary: {}, suites: [{ name: 'rspec' }] }, type: 'receiveReportsSuccess', }, ], -- cgit v1.2.1 From 8e74f14c26b9df6a4fdc4fb79322b13b06c3c509 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 20 Jul 2018 13:15:35 +0100 Subject: Follow up after review: Clear Vuex actions and mutations --- app/assets/javascripts/reports/store/actions.js | 5 ++--- app/assets/javascripts/reports/store/mutations.js | 1 - spec/javascripts/reports/store/mutations_spec.js | 4 ---- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/reports/store/actions.js b/app/assets/javascripts/reports/store/actions.js index 72069489d20..6ce0a00a03e 100644 --- a/app/assets/javascripts/reports/store/actions.js +++ b/app/assets/javascripts/reports/store/actions.js @@ -16,6 +16,7 @@ export const clearEtagPoll = () => { export const stopPolling = () => { if (eTagPoll) eTagPoll.stop(); }; + export const restartPolling = () => { if (eTagPoll) eTagPoll.restart(); }; @@ -60,9 +61,7 @@ export const fetchReports = ({ state, dispatch }) => { export const receiveReportsSuccess = ({ commit }, response) => commit(types.RECEIVE_REPORTS_SUCCESS, response); -export const receiveReportsError = ({ commit }) => { - commit(types.RECEIVE_REPORTS_ERROR); -}; +export const receiveReportsError = ({ commit }) => commit(types.RECEIVE_REPORTS_ERROR); // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/reports/store/mutations.js b/app/assets/javascripts/reports/store/mutations.js index 9487b8d073a..3851f2f5799 100644 --- a/app/assets/javascripts/reports/store/mutations.js +++ b/app/assets/javascripts/reports/store/mutations.js @@ -12,7 +12,6 @@ export default { [types.RECEIVE_REPORTS_SUCCESS](state, response) { state.isLoading = false; - state.hasError = false; Vue.set(state.summary, 'total', response.summary.total); Vue.set(state.summary, 'resolved', response.summary.resolved); diff --git a/spec/javascripts/reports/store/mutations_spec.js b/spec/javascripts/reports/store/mutations_spec.js index 90d2203cc66..3e0b15438c3 100644 --- a/spec/javascripts/reports/store/mutations_spec.js +++ b/spec/javascripts/reports/store/mutations_spec.js @@ -74,10 +74,6 @@ describe('Reports Store Mutations', () => { expect(stateCopy.isLoading).toEqual(false); }); - it('should reset hasError', () => { - expect(stateCopy.hasError).toEqual(false); - }); - it('should set summary counts', () => { expect(stateCopy.summary.total).toEqual(mockedResponse.summary.total); expect(stateCopy.summary.resolved).toEqual(mockedResponse.summary.resolved); -- cgit v1.2.1 From 5cfe99006ed539fbc9e0a184b09af4be63e6d91b Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 20 Jul 2018 18:54:06 +0100 Subject: Render grouped test report in MR widget --- .../components/grouped_test_reports_app.vue | 120 ++++++++++++++------- .../javascripts/reports/components/modal.vue | 27 ++--- .../reports/components/test_issue_body.vue | 40 +++++++ app/assets/javascripts/reports/constants.js | 56 +--------- app/assets/javascripts/reports/store/actions.js | 9 ++ app/assets/javascripts/reports/store/getters.js | 20 ++++ app/assets/javascripts/reports/store/index.js | 3 + .../javascripts/reports/store/mutation_types.js | 1 + app/assets/javascripts/reports/store/mutations.js | 10 ++ app/assets/javascripts/reports/store/state.js | 31 ++++++ app/assets/javascripts/reports/store/utils.js | 44 ++++++++ .../vue_merge_request_widget/mr_widget_options.vue | 8 ++ .../stores/mr_widget_store.js | 2 + .../components/reports/report_issues.vue | 12 +++ .../vue_shared/components/reports/summary_row.vue | 8 +- app/assets/stylesheets/pages/merge_requests.scss | 12 +++ 16 files changed, 292 insertions(+), 111 deletions(-) create mode 100644 app/assets/javascripts/reports/components/test_issue_body.vue create mode 100644 app/assets/javascripts/reports/store/getters.js create mode 100644 app/assets/javascripts/reports/store/utils.js diff --git a/app/assets/javascripts/reports/components/grouped_test_reports_app.vue b/app/assets/javascripts/reports/components/grouped_test_reports_app.vue index 016806b84c3..c3f775013eb 100644 --- a/app/assets/javascripts/reports/components/grouped_test_reports_app.vue +++ b/app/assets/javascripts/reports/components/grouped_test_reports_app.vue @@ -1,40 +1,77 @@ diff --git a/app/assets/javascripts/reports/components/modal.vue b/app/assets/javascripts/reports/components/modal.vue index 61004399aa4..3eb7094f166 100644 --- a/app/assets/javascripts/reports/components/modal.vue +++ b/app/assets/javascripts/reports/components/modal.vue @@ -20,24 +20,15 @@ type: Object, required: true, }, - actions: { - type: Array, - required: true, - }, - }, - - computed: { - shouldRenderFooterSection() { - return this.actions.length > 0; - }, }, + fieldTypes, };