From 93ea3234dfaa43204c5f24d4010bbe5070d75c72 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Wed, 17 Jan 2018 11:30:25 +0000 Subject: Use the DatabaseCleaner 'deletion' strategy instead of 'truncation' --- spec/uploaders/job_artifact_uploader_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'spec/uploaders') diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index 14fd5f3600f..98a4373e9d0 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -8,7 +8,7 @@ describe JobArtifactUploader do describe '#store_dir' do subject { uploader.store_dir } - let(:path) { "#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/#{job_artifact.project_id}/#{job_artifact.id}" } + let(:path) { "#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/#{job_artifact.job_id}/#{job_artifact.id}" } context 'when using local storage' do it { is_expected.to start_with(local_path) } @@ -45,7 +45,7 @@ describe JobArtifactUploader do it { is_expected.to start_with(local_path) } it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") } - it { is_expected.to include("/#{job_artifact.project_id}/") } + it { is_expected.to include("/#{job_artifact.job_id}/#{job_artifact.id}/") } it { is_expected.to end_with("ci_build_artifacts.zip") } end end -- cgit v1.2.1 From 2b6307f6ad9d09156c42befe4babbfea40dad052 Mon Sep 17 00:00:00 2001 From: Takuya Noguchi Date: Sat, 27 Jan 2018 14:35:53 +0900 Subject: Enable RuboCop Style/RegexpLiteral --- spec/uploaders/file_uploader_spec.rb | 2 +- spec/uploaders/job_artifact_uploader_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'spec/uploaders') diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index fd195d6f9b8..845516e5004 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -112,7 +112,7 @@ describe FileUploader do fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') uploader.store!(fixture_file_upload(fixture)) - expect(uploader.relative_path).to match(/\A\h{32}\/rails_sample.jpg\z/) + expect(uploader.relative_path).to match(%r{\A\h{32}/rails_sample.jpg\z}) end end end diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index 98a4373e9d0..a067c3e75f4 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -12,7 +12,7 @@ describe JobArtifactUploader do context 'when using local storage' do it { is_expected.to start_with(local_path) } - it { is_expected.to match(/\h{2}\/\h{2}\/\h{64}\/\d{4}_\d{1,2}_\d{1,2}\/\d+\/\d+\z/) } + it { is_expected.to match(%r{\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z}) } it { is_expected.to end_with(path) } end end -- cgit v1.2.1 From 2057a6acdee7c1f6824ff6289b0d979e8cb15f35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Mon, 29 Jan 2018 12:57:34 -0500 Subject: port of 594e6a0a625^..f74c90f68c6 --- spec/uploaders/attachment_uploader_spec.rb | 18 ++-- spec/uploaders/avatar_uploader_spec.rb | 18 ++-- spec/uploaders/file_mover_spec.rb | 18 ++-- spec/uploaders/file_uploader_spec.rb | 121 ++++++------------------ spec/uploaders/job_artifact_uploader_spec.rb | 32 ++----- spec/uploaders/legacy_artifact_uploader_spec.rb | 49 +++------- spec/uploaders/lfs_object_uploader_spec.rb | 38 ++------ spec/uploaders/namespace_file_uploader_spec.rb | 21 ++-- spec/uploaders/personal_file_uploader_spec.rb | 28 +++--- spec/uploaders/records_uploads_spec.rb | 73 +++++++------- 10 files changed, 134 insertions(+), 282 deletions(-) (limited to 'spec/uploaders') diff --git a/spec/uploaders/attachment_uploader_spec.rb b/spec/uploaders/attachment_uploader_spec.rb index 04ee6e9bfad..db378dab97d 100644 --- a/spec/uploaders/attachment_uploader_spec.rb +++ b/spec/uploaders/attachment_uploader_spec.rb @@ -1,18 +1,16 @@ require 'spec_helper' describe AttachmentUploader do - let(:uploader) { described_class.new(build_stubbed(:user)) } + let(:note) { create(:note, :with_attachment) } + let(:uploader) { note.attachment } + let(:upload) { create(:upload, :attachment_upload, model: uploader.model) } - describe "#store_dir" do - it "stores in the system dir" do - expect(uploader.store_dir).to start_with("uploads/-/system/user") - end + subject { uploader } - it "uses the old path when using object storage" do - expect(described_class).to receive(:file_storage?).and_return(false) - expect(uploader.store_dir).to start_with("uploads/user") - end - end + it_behaves_like 'builds correct paths', + store_dir: %r[uploads/-/system/note/attachment/], + upload_path: %r[uploads/-/system/note/attachment/], + absolute_path: %r[#{CarrierWave.root}/uploads/-/system/note/attachment/] describe '#move_to_cache' do it 'is true' do diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb index 1dc574699d8..bf9028c9260 100644 --- a/spec/uploaders/avatar_uploader_spec.rb +++ b/spec/uploaders/avatar_uploader_spec.rb @@ -1,18 +1,16 @@ require 'spec_helper' describe AvatarUploader do - let(:uploader) { described_class.new(build_stubbed(:user)) } + let(:model) { create(:user, :with_avatar) } + let(:uploader) { described_class.new(model, :avatar) } + let(:upload) { create(:upload, model: model) } - describe "#store_dir" do - it "stores in the system dir" do - expect(uploader.store_dir).to start_with("uploads/-/system/user") - end + subject { uploader } - it "uses the old path when using object storage" do - expect(described_class).to receive(:file_storage?).and_return(false) - expect(uploader.store_dir).to start_with("uploads/user") - end - end + it_behaves_like 'builds correct paths', + store_dir: %r[uploads/-/system/user/avatar/], + upload_path: %r[uploads/-/system/user/avatar/], + absolute_path: %r[#{CarrierWave.root}/uploads/-/system/user/avatar/] describe '#move_to_cache' do it 'is false' do diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index 0cf462e9553..bc024cd307c 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -3,13 +3,13 @@ require 'spec_helper' describe FileMover do let(:filename) { 'banana_sample.gif' } let(:file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', filename)) } + let(:temp_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) } + let(:temp_description) do - 'test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) same ![banana_sample]'\ - '(/uploads/-/system/temp/secret55/banana_sample.gif)' + "test ![banana_sample](/#{temp_file_path}) "\ + "same ![banana_sample](/#{temp_file_path}) " end - let(:temp_file_path) { File.join('secret55', filename).to_s } - let(:file_path) { File.join('uploads', '-', 'system', 'personal_snippet', snippet.id.to_s, 'secret55', filename).to_s } - + let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) } let(:snippet) { create(:personal_snippet, description: temp_description) } subject { described_class.new(file_path, snippet).execute } @@ -28,8 +28,8 @@ describe FileMover do expect(snippet.reload.description) .to eq( - "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"\ - " same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)" + "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\ + "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) " ) end @@ -50,8 +50,8 @@ describe FileMover do expect(snippet.reload.description) .to eq( - "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif)"\ - " same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif)" + "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "\ + "same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) " ) end diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index 845516e5004..a72f853df75 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -1,118 +1,57 @@ require 'spec_helper' describe FileUploader do - let(:uploader) { described_class.new(build_stubbed(:project)) } + let(:group) { create(:group, name: 'awesome') } + let(:project) { create(:project, namespace: group, name: 'project') } + let(:uploader) { described_class.new(project) } + let(:upload) { double(model: project, path: 'secret/foo.jpg') } - context 'legacy storage' do - let(:project) { build_stubbed(:project) } - - describe '.absolute_path' do - it 'returns the correct absolute path by building it dynamically' do - upload = double(model: project, path: 'secret/foo.jpg') - - dynamic_segment = project.full_path + subject { uploader } - expect(described_class.absolute_path(upload)) - .to end_with("#{dynamic_segment}/secret/foo.jpg") - end - end - - describe "#store_dir" do - it "stores in the namespace path" do - uploader = described_class.new(project) - - expect(uploader.store_dir).to include(project.full_path) - expect(uploader.store_dir).not_to include("system") - end - end + shared_examples 'builds correct legacy storage paths' do + include_examples 'builds correct paths', + store_dir: %r{awesome/project/\h+}, + absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg} end - context 'hashed storage' do + shared_examples 'uses hashed storage' do context 'when rolled out attachments' do - let(:project) { build_stubbed(:project, :hashed) } - - describe '.absolute_path' do - it 'returns the correct absolute path by building it dynamically' do - upload = double(model: project, path: 'secret/foo.jpg') - - dynamic_segment = project.disk_path - - expect(described_class.absolute_path(upload)) - .to end_with("#{dynamic_segment}/secret/foo.jpg") - end + before do + allow(project).to receive(:disk_path).and_return('ca/fe/fe/ed') end - describe "#store_dir" do - it "stores in the namespace path" do - uploader = described_class.new(project) + let(:project) { build_stubbed(:project, :hashed, namespace: group, name: 'project') } - expect(uploader.store_dir).to include(project.disk_path) - expect(uploader.store_dir).not_to include("system") - end - end + it_behaves_like 'builds correct paths', + store_dir: %r{ca/fe/fe/ed/\h+}, + absolute_path: %r{#{described_class.root}/ca/fe/fe/ed/secret/foo.jpg} end context 'when only repositories are rolled out' do - let(:project) { build_stubbed(:project, storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) } - - describe '.absolute_path' do - it 'returns the correct absolute path by building it dynamically' do - upload = double(model: project, path: 'secret/foo.jpg') - - dynamic_segment = project.full_path - - expect(described_class.absolute_path(upload)) - .to end_with("#{dynamic_segment}/secret/foo.jpg") - end - end - - describe "#store_dir" do - it "stores in the namespace path" do - uploader = described_class.new(project) + let(:project) { build_stubbed(:project, namespace: group, name: 'project', storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) } - expect(uploader.store_dir).to include(project.full_path) - expect(uploader.store_dir).not_to include("system") - end - end + it_behaves_like 'builds correct legacy storage paths' end end - describe 'initialize' do - it 'generates a secret if none is provided' do - expect(SecureRandom).to receive(:hex).and_return('secret') - - uploader = described_class.new(double) - - expect(uploader.secret).to eq 'secret' - end - - it 'accepts a secret parameter' do - expect(SecureRandom).not_to receive(:hex) - - uploader = described_class.new(double, 'secret') - - expect(uploader.secret).to eq 'secret' - end + context 'legacy storage' do + it_behaves_like 'builds correct legacy storage paths' + include_examples 'uses hashed storage' end - describe '#move_to_cache' do - it 'is true' do - expect(uploader.move_to_cache).to eq(true) - end - end + describe 'initialize' do + let(:uploader) { described_class.new(double, 'secret') } - describe '#move_to_store' do - it 'is true' do - expect(uploader.move_to_store).to eq(true) + it 'accepts a secret parameter' do + expect(described_class).not_to receive(:generate_secret) + expect(uploader.secret).to eq('secret') end end - describe '#relative_path' do - it 'removes the leading dynamic path segment' do - fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') - uploader.store!(fixture_file_upload(fixture)) - - expect(uploader.relative_path).to match(%r{\A\h{32}/rails_sample.jpg\z}) + describe '#secret' do + it 'generates a secret if none is provided' do + expect(described_class).to receive(:generate_secret).and_return('secret') + expect(uploader.secret).to eq('secret') end end end diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index a067c3e75f4..d606404e95d 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -3,33 +3,13 @@ require 'spec_helper' describe JobArtifactUploader do let(:job_artifact) { create(:ci_job_artifact) } let(:uploader) { described_class.new(job_artifact, :file) } - let(:local_path) { Gitlab.config.artifacts.path } - describe '#store_dir' do - subject { uploader.store_dir } + subject { uploader } - let(:path) { "#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/#{job_artifact.job_id}/#{job_artifact.id}" } - - context 'when using local storage' do - it { is_expected.to start_with(local_path) } - it { is_expected.to match(%r{\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z}) } - it { is_expected.to end_with(path) } - end - end - - describe '#cache_dir' do - subject { uploader.cache_dir } - - it { is_expected.to start_with(local_path) } - it { is_expected.to end_with('/tmp/cache') } - end - - describe '#work_dir' do - subject { uploader.work_dir } - - it { is_expected.to start_with(local_path) } - it { is_expected.to end_with('/tmp/work') } - end + it_behaves_like "builds correct paths", + store_dir: %r[\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z], + cache_dir: %r[artifacts/tmp/cache], + work_dir: %r[artifacts/tmp/work] context 'file is stored in valid local_path' do let(:file) do @@ -43,7 +23,7 @@ describe JobArtifactUploader do subject { uploader.file.path } - it { is_expected.to start_with(local_path) } + it { is_expected.to start_with("#{uploader.root}/#{uploader.class.base_dir}") } it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") } it { is_expected.to include("/#{job_artifact.job_id}/#{job_artifact.id}/") } it { is_expected.to end_with("ci_build_artifacts.zip") } diff --git a/spec/uploaders/legacy_artifact_uploader_spec.rb b/spec/uploaders/legacy_artifact_uploader_spec.rb index efeffb78772..54c6a8b869b 100644 --- a/spec/uploaders/legacy_artifact_uploader_spec.rb +++ b/spec/uploaders/legacy_artifact_uploader_spec.rb @@ -3,49 +3,22 @@ require 'rails_helper' describe LegacyArtifactUploader do let(:job) { create(:ci_build) } let(:uploader) { described_class.new(job, :legacy_artifacts_file) } - let(:local_path) { Gitlab.config.artifacts.path } + let(:local_path) { described_class.root } - describe '.local_store_path' do - subject { described_class.local_store_path } + subject { uploader } - it "delegate to artifacts path" do - expect(Gitlab.config.artifacts).to receive(:path) - - subject - end - end - - describe '.artifacts_upload_path' do - subject { described_class.artifacts_upload_path } + # TODO: move to Workhorse::UploadPath + describe '.workhorse_upload_path' do + subject { described_class.workhorse_upload_path } it { is_expected.to start_with(local_path) } - it { is_expected.to end_with('tmp/uploads/') } - end - - describe '#store_dir' do - subject { uploader.store_dir } - - let(:path) { "#{job.created_at.utc.strftime('%Y_%m')}/#{job.project_id}/#{job.id}" } - - context 'when using local storage' do - it { is_expected.to start_with(local_path) } - it { is_expected.to end_with(path) } - end + it { is_expected.to end_with('tmp/uploads') } end - describe '#cache_dir' do - subject { uploader.cache_dir } - - it { is_expected.to start_with(local_path) } - it { is_expected.to end_with('/tmp/cache') } - end - - describe '#work_dir' do - subject { uploader.work_dir } - - it { is_expected.to start_with(local_path) } - it { is_expected.to end_with('/tmp/work') } - end + it_behaves_like "builds correct paths", + store_dir: %r[\d{4}_\d{1,2}/\d+/\d+\z], + cache_dir: %r[artifacts/tmp/cache], + work_dir: %r[artifacts/tmp/work] describe '#filename' do # we need to use uploader, as this makes to use mounter @@ -69,7 +42,7 @@ describe LegacyArtifactUploader do subject { uploader.file.path } - it { is_expected.to start_with(local_path) } + it { is_expected.to start_with("#{uploader.root}") } it { is_expected.to include("/#{job.created_at.utc.strftime('%Y_%m')}/") } it { is_expected.to include("/#{job.project_id}/") } it { is_expected.to end_with("ci_build_artifacts.zip") } diff --git a/spec/uploaders/lfs_object_uploader_spec.rb b/spec/uploaders/lfs_object_uploader_spec.rb index 7088bc23334..6ebc885daa8 100644 --- a/spec/uploaders/lfs_object_uploader_spec.rb +++ b/spec/uploaders/lfs_object_uploader_spec.rb @@ -2,39 +2,13 @@ require 'spec_helper' describe LfsObjectUploader do let(:lfs_object) { create(:lfs_object, :with_file) } - let(:uploader) { described_class.new(lfs_object) } + let(:uploader) { described_class.new(lfs_object, :file) } let(:path) { Gitlab.config.lfs.storage_path } - describe '#move_to_cache' do - it 'is true' do - expect(uploader.move_to_cache).to eq(true) - end - end + subject { uploader } - describe '#move_to_store' do - it 'is true' do - expect(uploader.move_to_store).to eq(true) - end - end - - describe '#store_dir' do - subject { uploader.store_dir } - - it { is_expected.to start_with(path) } - it { is_expected.to end_with("#{lfs_object.oid[0, 2]}/#{lfs_object.oid[2, 2]}") } - end - - describe '#cache_dir' do - subject { uploader.cache_dir } - - it { is_expected.to start_with(path) } - it { is_expected.to end_with('/tmp/cache') } - end - - describe '#work_dir' do - subject { uploader.work_dir } - - it { is_expected.to start_with(path) } - it { is_expected.to end_with('/tmp/work') } - end + it_behaves_like "builds correct paths", + store_dir: %r[\h{2}/\h{2}], + cache_dir: %r[/lfs-objects/tmp/cache], + work_dir: %r[/lfs-objects/tmp/work] end diff --git a/spec/uploaders/namespace_file_uploader_spec.rb b/spec/uploaders/namespace_file_uploader_spec.rb index c6c4500c179..24a2fc0f72e 100644 --- a/spec/uploaders/namespace_file_uploader_spec.rb +++ b/spec/uploaders/namespace_file_uploader_spec.rb @@ -1,21 +1,16 @@ require 'spec_helper' +IDENTIFIER = %r{\h+/\S+} + describe NamespaceFileUploader do let(:group) { build_stubbed(:group) } let(:uploader) { described_class.new(group) } + let(:upload) { create(:upload, :namespace_upload, model: group) } - describe "#store_dir" do - it "stores in the namespace id directory" do - expect(uploader.store_dir).to include(group.id.to_s) - end - end - - describe ".absolute_path" do - it "stores in thecorrect directory" do - upload_record = create(:upload, :namespace_upload, model: group) + subject { uploader } - expect(described_class.absolute_path(upload_record)) - .to include("-/system/namespace/#{group.id}") - end - end + it_behaves_like 'builds correct paths', + store_dir: %r[uploads/-/system/namespace/\d+], + upload_path: IDENTIFIER, + absolute_path: %r[#{CarrierWave.root}/uploads/-/system/namespace/\d+/#{IDENTIFIER}] end diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb index cbafa9f478d..ed1fba6edda 100644 --- a/spec/uploaders/personal_file_uploader_spec.rb +++ b/spec/uploaders/personal_file_uploader_spec.rb @@ -1,25 +1,27 @@ require 'spec_helper' +IDENTIFIER = %r{\h+/\S+} + describe PersonalFileUploader do - let(:uploader) { described_class.new(build_stubbed(:project)) } - let(:snippet) { create(:personal_snippet) } + let(:model) { create(:personal_snippet) } + let(:uploader) { described_class.new(model) } + let(:upload) { create(:upload, :personal_snippet_upload) } - describe '.absolute_path' do - it 'returns the correct absolute path by building it dynamically' do - upload = double(model: snippet, path: 'secret/foo.jpg') + subject { uploader } - dynamic_segment = "personal_snippet/#{snippet.id}" - - expect(described_class.absolute_path(upload)).to end_with("/-/system/#{dynamic_segment}/secret/foo.jpg") - end - end + it_behaves_like 'builds correct paths', + store_dir: %r[uploads/-/system/personal_snippet/\d+], + upload_path: IDENTIFIER, + absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet/\d+/#{IDENTIFIER}] describe '#to_h' do - it 'returns the hass' do - uploader = described_class.new(snippet, 'secret') + before do + subject.instance_variable_set(:@secret, 'secret') + end + it 'is correct' do allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name')) - expected_url = "/uploads/-/system/personal_snippet/#{snippet.id}/secret/file_name" + expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name" expect(uploader.to_h).to eq( alt: 'file_name', diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb index 7ef7fb7d758..9a3e5d83e01 100644 --- a/spec/uploaders/records_uploads_spec.rb +++ b/spec/uploaders/records_uploads_spec.rb @@ -3,16 +3,16 @@ require 'rails_helper' describe RecordsUploads do let!(:uploader) do class RecordsUploadsExampleUploader < GitlabUploader - include RecordsUploads + include RecordsUploads::Concern storage :file - def model - FactoryBot.build_stubbed(:user) + def dynamic_segment + 'co/fe/ee' end end - RecordsUploadsExampleUploader.new + RecordsUploadsExampleUploader.new(build_stubbed(:user)) end def upload_fixture(filename) @@ -20,48 +20,55 @@ describe RecordsUploads do end describe 'callbacks' do - it 'calls `record_upload` after `store`' do + let(:upload) { create(:upload) } + + before do + uploader.upload = upload + end + + it '#record_upload after `store`' do expect(uploader).to receive(:record_upload).once uploader.store!(upload_fixture('doc_sample.txt')) end - it 'calls `destroy_upload` after `remove`' do - expect(uploader).to receive(:destroy_upload).once - + it '#destroy_upload after `remove`' do uploader.store!(upload_fixture('doc_sample.txt')) + expect(uploader).to receive(:destroy_upload).once uploader.remove! end end describe '#record_upload callback' do - it 'returns early when not using file storage' do - allow(uploader).to receive(:file_storage?).and_return(false) - expect(Upload).not_to receive(:record) - - uploader.store!(upload_fixture('rails_sample.jpg')) + it 'creates an Upload record after store' do + expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.to change { Upload.count }.by(1) end - it "returns early when the file doesn't exist" do - allow(uploader).to receive(:file).and_return(double(exists?: false)) - expect(Upload).not_to receive(:record) - + it 'creates a new record and assigns size, path, model, and uploader' do uploader.store!(upload_fixture('rails_sample.jpg')) + + upload = uploader.upload + aggregate_failures do + expect(upload).to be_persisted + expect(upload.size).to eq uploader.file.size + expect(upload.path).to eq uploader.upload_path + expect(upload.model_id).to eq uploader.model.id + expect(upload.model_type).to eq uploader.model.class.to_s + expect(upload.uploader).to eq uploader.class.to_s + end end - it 'creates an Upload record after store' do - expect(Upload).to receive(:record) - .with(uploader) + it "does not create an Upload record when the file doesn't exist" do + allow(uploader).to receive(:file).and_return(double(exists?: false)) - uploader.store!(upload_fixture('rails_sample.jpg')) + expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count } end it 'does not create an Upload record if model is missing' do - expect_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil) - expect(Upload).not_to receive(:record).with(uploader) + allow_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil) - uploader.store!(upload_fixture('rails_sample.jpg')) + expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count } end it 'it destroys Upload records at the same path before recording' do @@ -72,29 +79,15 @@ describe RecordsUploads do uploader: uploader.class.to_s ) + uploader.upload = existing uploader.store!(upload_fixture('rails_sample.jpg')) expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound) - expect(Upload.count).to eq 1 + expect(Upload.count).to eq(1) end end describe '#destroy_upload callback' do - it 'returns early when not using file storage' do - uploader.store!(upload_fixture('rails_sample.jpg')) - - allow(uploader).to receive(:file_storage?).and_return(false) - expect(Upload).not_to receive(:remove_path) - - uploader.remove! - end - - it 'returns early when file is nil' do - expect(Upload).not_to receive(:remove_path) - - uploader.remove! - end - it 'it destroys Upload records at the same path after removal' do uploader.store!(upload_fixture('rails_sample.jpg')) -- cgit v1.2.1 From a5bb17ffd5d466568c3bcea161bac42123bd3c1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mica=C3=ABl=20Bergeron?= Date: Thu, 1 Feb 2018 14:24:59 -0500 Subject: porting changes from upstream --- spec/uploaders/attachment_uploader_spec.rb | 12 ------------ 1 file changed, 12 deletions(-) (limited to 'spec/uploaders') diff --git a/spec/uploaders/attachment_uploader_spec.rb b/spec/uploaders/attachment_uploader_spec.rb index db378dab97d..091ba824fc6 100644 --- a/spec/uploaders/attachment_uploader_spec.rb +++ b/spec/uploaders/attachment_uploader_spec.rb @@ -11,16 +11,4 @@ describe AttachmentUploader do store_dir: %r[uploads/-/system/note/attachment/], upload_path: %r[uploads/-/system/note/attachment/], absolute_path: %r[#{CarrierWave.root}/uploads/-/system/note/attachment/] - - describe '#move_to_cache' do - it 'is true' do - expect(uploader.move_to_cache).to eq(true) - end - end - - describe '#move_to_store' do - it 'is true' do - expect(uploader.move_to_store).to eq(true) - end - end end -- cgit v1.2.1 From 8af23def1d6450420d06b8de54d23311a978de20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Wed, 28 Feb 2018 21:09:34 +0100 Subject: Revert "Merge branch '3867-port-to-ce' into 'master'" This reverts commit 54a575f1bbba44573ab92dc58a4242f1ee734c5d, reversing changes made to c63af942e5baf7849a94fa99da8494bcba28e3f8. --- spec/uploaders/attachment_uploader_spec.rb | 30 ++++-- spec/uploaders/avatar_uploader_spec.rb | 18 ++-- spec/uploaders/file_mover_spec.rb | 18 ++-- spec/uploaders/file_uploader_spec.rb | 121 ++++++++++++++++++------ spec/uploaders/job_artifact_uploader_spec.rb | 32 +++++-- spec/uploaders/legacy_artifact_uploader_spec.rb | 49 +++++++--- spec/uploaders/lfs_object_uploader_spec.rb | 38 ++++++-- spec/uploaders/namespace_file_uploader_spec.rb | 21 ++-- spec/uploaders/personal_file_uploader_spec.rb | 28 +++--- spec/uploaders/records_uploads_spec.rb | 73 +++++++------- 10 files changed, 294 insertions(+), 134 deletions(-) (limited to 'spec/uploaders') diff --git a/spec/uploaders/attachment_uploader_spec.rb b/spec/uploaders/attachment_uploader_spec.rb index 091ba824fc6..04ee6e9bfad 100644 --- a/spec/uploaders/attachment_uploader_spec.rb +++ b/spec/uploaders/attachment_uploader_spec.rb @@ -1,14 +1,28 @@ require 'spec_helper' describe AttachmentUploader do - let(:note) { create(:note, :with_attachment) } - let(:uploader) { note.attachment } - let(:upload) { create(:upload, :attachment_upload, model: uploader.model) } + let(:uploader) { described_class.new(build_stubbed(:user)) } - subject { uploader } + describe "#store_dir" do + it "stores in the system dir" do + expect(uploader.store_dir).to start_with("uploads/-/system/user") + end - it_behaves_like 'builds correct paths', - store_dir: %r[uploads/-/system/note/attachment/], - upload_path: %r[uploads/-/system/note/attachment/], - absolute_path: %r[#{CarrierWave.root}/uploads/-/system/note/attachment/] + it "uses the old path when using object storage" do + expect(described_class).to receive(:file_storage?).and_return(false) + expect(uploader.store_dir).to start_with("uploads/user") + end + end + + describe '#move_to_cache' do + it 'is true' do + expect(uploader.move_to_cache).to eq(true) + end + end + + describe '#move_to_store' do + it 'is true' do + expect(uploader.move_to_store).to eq(true) + end + end end diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb index bf9028c9260..1dc574699d8 100644 --- a/spec/uploaders/avatar_uploader_spec.rb +++ b/spec/uploaders/avatar_uploader_spec.rb @@ -1,16 +1,18 @@ require 'spec_helper' describe AvatarUploader do - let(:model) { create(:user, :with_avatar) } - let(:uploader) { described_class.new(model, :avatar) } - let(:upload) { create(:upload, model: model) } + let(:uploader) { described_class.new(build_stubbed(:user)) } - subject { uploader } + describe "#store_dir" do + it "stores in the system dir" do + expect(uploader.store_dir).to start_with("uploads/-/system/user") + end - it_behaves_like 'builds correct paths', - store_dir: %r[uploads/-/system/user/avatar/], - upload_path: %r[uploads/-/system/user/avatar/], - absolute_path: %r[#{CarrierWave.root}/uploads/-/system/user/avatar/] + it "uses the old path when using object storage" do + expect(described_class).to receive(:file_storage?).and_return(false) + expect(uploader.store_dir).to start_with("uploads/user") + end + end describe '#move_to_cache' do it 'is false' do diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb index bc024cd307c..0cf462e9553 100644 --- a/spec/uploaders/file_mover_spec.rb +++ b/spec/uploaders/file_mover_spec.rb @@ -3,13 +3,13 @@ require 'spec_helper' describe FileMover do let(:filename) { 'banana_sample.gif' } let(:file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', filename)) } - let(:temp_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) } - let(:temp_description) do - "test ![banana_sample](/#{temp_file_path}) "\ - "same ![banana_sample](/#{temp_file_path}) " + 'test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) same ![banana_sample]'\ + '(/uploads/-/system/temp/secret55/banana_sample.gif)' end - let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) } + let(:temp_file_path) { File.join('secret55', filename).to_s } + let(:file_path) { File.join('uploads', '-', 'system', 'personal_snippet', snippet.id.to_s, 'secret55', filename).to_s } + let(:snippet) { create(:personal_snippet, description: temp_description) } subject { described_class.new(file_path, snippet).execute } @@ -28,8 +28,8 @@ describe FileMover do expect(snippet.reload.description) .to eq( - "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\ - "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) " + "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"\ + " same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)" ) end @@ -50,8 +50,8 @@ describe FileMover do expect(snippet.reload.description) .to eq( - "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "\ - "same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) " + "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif)"\ + " same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif)" ) end diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb index a72f853df75..845516e5004 100644 --- a/spec/uploaders/file_uploader_spec.rb +++ b/spec/uploaders/file_uploader_spec.rb @@ -1,57 +1,118 @@ require 'spec_helper' describe FileUploader do - let(:group) { create(:group, name: 'awesome') } - let(:project) { create(:project, namespace: group, name: 'project') } - let(:uploader) { described_class.new(project) } - let(:upload) { double(model: project, path: 'secret/foo.jpg') } + let(:uploader) { described_class.new(build_stubbed(:project)) } - subject { uploader } + context 'legacy storage' do + let(:project) { build_stubbed(:project) } + + describe '.absolute_path' do + it 'returns the correct absolute path by building it dynamically' do + upload = double(model: project, path: 'secret/foo.jpg') + + dynamic_segment = project.full_path - shared_examples 'builds correct legacy storage paths' do - include_examples 'builds correct paths', - store_dir: %r{awesome/project/\h+}, - absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg} + expect(described_class.absolute_path(upload)) + .to end_with("#{dynamic_segment}/secret/foo.jpg") + end + end + + describe "#store_dir" do + it "stores in the namespace path" do + uploader = described_class.new(project) + + expect(uploader.store_dir).to include(project.full_path) + expect(uploader.store_dir).not_to include("system") + end + end end - shared_examples 'uses hashed storage' do + context 'hashed storage' do context 'when rolled out attachments' do - before do - allow(project).to receive(:disk_path).and_return('ca/fe/fe/ed') + let(:project) { build_stubbed(:project, :hashed) } + + describe '.absolute_path' do + it 'returns the correct absolute path by building it dynamically' do + upload = double(model: project, path: 'secret/foo.jpg') + + dynamic_segment = project.disk_path + + expect(described_class.absolute_path(upload)) + .to end_with("#{dynamic_segment}/secret/foo.jpg") + end end - let(:project) { build_stubbed(:project, :hashed, namespace: group, name: 'project') } + describe "#store_dir" do + it "stores in the namespace path" do + uploader = described_class.new(project) - it_behaves_like 'builds correct paths', - store_dir: %r{ca/fe/fe/ed/\h+}, - absolute_path: %r{#{described_class.root}/ca/fe/fe/ed/secret/foo.jpg} + expect(uploader.store_dir).to include(project.disk_path) + expect(uploader.store_dir).not_to include("system") + end + end end context 'when only repositories are rolled out' do - let(:project) { build_stubbed(:project, namespace: group, name: 'project', storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) } + let(:project) { build_stubbed(:project, storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) } - it_behaves_like 'builds correct legacy storage paths' - end - end + describe '.absolute_path' do + it 'returns the correct absolute path by building it dynamically' do + upload = double(model: project, path: 'secret/foo.jpg') - context 'legacy storage' do - it_behaves_like 'builds correct legacy storage paths' - include_examples 'uses hashed storage' + dynamic_segment = project.full_path + + expect(described_class.absolute_path(upload)) + .to end_with("#{dynamic_segment}/secret/foo.jpg") + end + end + + describe "#store_dir" do + it "stores in the namespace path" do + uploader = described_class.new(project) + + expect(uploader.store_dir).to include(project.full_path) + expect(uploader.store_dir).not_to include("system") + end + end + end end describe 'initialize' do - let(:uploader) { described_class.new(double, 'secret') } + it 'generates a secret if none is provided' do + expect(SecureRandom).to receive(:hex).and_return('secret') + + uploader = described_class.new(double) + + expect(uploader.secret).to eq 'secret' + end it 'accepts a secret parameter' do - expect(described_class).not_to receive(:generate_secret) - expect(uploader.secret).to eq('secret') + expect(SecureRandom).not_to receive(:hex) + + uploader = described_class.new(double, 'secret') + + expect(uploader.secret).to eq 'secret' end end - describe '#secret' do - it 'generates a secret if none is provided' do - expect(described_class).to receive(:generate_secret).and_return('secret') - expect(uploader.secret).to eq('secret') + describe '#move_to_cache' do + it 'is true' do + expect(uploader.move_to_cache).to eq(true) + end + end + + describe '#move_to_store' do + it 'is true' do + expect(uploader.move_to_store).to eq(true) + end + end + + describe '#relative_path' do + it 'removes the leading dynamic path segment' do + fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg') + uploader.store!(fixture_file_upload(fixture)) + + expect(uploader.relative_path).to match(%r{\A\h{32}/rails_sample.jpg\z}) end end end diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb index d606404e95d..a067c3e75f4 100644 --- a/spec/uploaders/job_artifact_uploader_spec.rb +++ b/spec/uploaders/job_artifact_uploader_spec.rb @@ -3,13 +3,33 @@ require 'spec_helper' describe JobArtifactUploader do let(:job_artifact) { create(:ci_job_artifact) } let(:uploader) { described_class.new(job_artifact, :file) } + let(:local_path) { Gitlab.config.artifacts.path } - subject { uploader } + describe '#store_dir' do + subject { uploader.store_dir } - it_behaves_like "builds correct paths", - store_dir: %r[\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z], - cache_dir: %r[artifacts/tmp/cache], - work_dir: %r[artifacts/tmp/work] + let(:path) { "#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/#{job_artifact.job_id}/#{job_artifact.id}" } + + context 'when using local storage' do + it { is_expected.to start_with(local_path) } + it { is_expected.to match(%r{\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z}) } + it { is_expected.to end_with(path) } + end + end + + describe '#cache_dir' do + subject { uploader.cache_dir } + + it { is_expected.to start_with(local_path) } + it { is_expected.to end_with('/tmp/cache') } + end + + describe '#work_dir' do + subject { uploader.work_dir } + + it { is_expected.to start_with(local_path) } + it { is_expected.to end_with('/tmp/work') } + end context 'file is stored in valid local_path' do let(:file) do @@ -23,7 +43,7 @@ describe JobArtifactUploader do subject { uploader.file.path } - it { is_expected.to start_with("#{uploader.root}/#{uploader.class.base_dir}") } + it { is_expected.to start_with(local_path) } it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") } it { is_expected.to include("/#{job_artifact.job_id}/#{job_artifact.id}/") } it { is_expected.to end_with("ci_build_artifacts.zip") } diff --git a/spec/uploaders/legacy_artifact_uploader_spec.rb b/spec/uploaders/legacy_artifact_uploader_spec.rb index 54c6a8b869b..efeffb78772 100644 --- a/spec/uploaders/legacy_artifact_uploader_spec.rb +++ b/spec/uploaders/legacy_artifact_uploader_spec.rb @@ -3,22 +3,49 @@ require 'rails_helper' describe LegacyArtifactUploader do let(:job) { create(:ci_build) } let(:uploader) { described_class.new(job, :legacy_artifacts_file) } - let(:local_path) { described_class.root } + let(:local_path) { Gitlab.config.artifacts.path } - subject { uploader } + describe '.local_store_path' do + subject { described_class.local_store_path } - # TODO: move to Workhorse::UploadPath - describe '.workhorse_upload_path' do - subject { described_class.workhorse_upload_path } + it "delegate to artifacts path" do + expect(Gitlab.config.artifacts).to receive(:path) + + subject + end + end + + describe '.artifacts_upload_path' do + subject { described_class.artifacts_upload_path } it { is_expected.to start_with(local_path) } - it { is_expected.to end_with('tmp/uploads') } + it { is_expected.to end_with('tmp/uploads/') } + end + + describe '#store_dir' do + subject { uploader.store_dir } + + let(:path) { "#{job.created_at.utc.strftime('%Y_%m')}/#{job.project_id}/#{job.id}" } + + context 'when using local storage' do + it { is_expected.to start_with(local_path) } + it { is_expected.to end_with(path) } + end end - it_behaves_like "builds correct paths", - store_dir: %r[\d{4}_\d{1,2}/\d+/\d+\z], - cache_dir: %r[artifacts/tmp/cache], - work_dir: %r[artifacts/tmp/work] + describe '#cache_dir' do + subject { uploader.cache_dir } + + it { is_expected.to start_with(local_path) } + it { is_expected.to end_with('/tmp/cache') } + end + + describe '#work_dir' do + subject { uploader.work_dir } + + it { is_expected.to start_with(local_path) } + it { is_expected.to end_with('/tmp/work') } + end describe '#filename' do # we need to use uploader, as this makes to use mounter @@ -42,7 +69,7 @@ describe LegacyArtifactUploader do subject { uploader.file.path } - it { is_expected.to start_with("#{uploader.root}") } + it { is_expected.to start_with(local_path) } it { is_expected.to include("/#{job.created_at.utc.strftime('%Y_%m')}/") } it { is_expected.to include("/#{job.project_id}/") } it { is_expected.to end_with("ci_build_artifacts.zip") } diff --git a/spec/uploaders/lfs_object_uploader_spec.rb b/spec/uploaders/lfs_object_uploader_spec.rb index 6ebc885daa8..7088bc23334 100644 --- a/spec/uploaders/lfs_object_uploader_spec.rb +++ b/spec/uploaders/lfs_object_uploader_spec.rb @@ -2,13 +2,39 @@ require 'spec_helper' describe LfsObjectUploader do let(:lfs_object) { create(:lfs_object, :with_file) } - let(:uploader) { described_class.new(lfs_object, :file) } + let(:uploader) { described_class.new(lfs_object) } let(:path) { Gitlab.config.lfs.storage_path } - subject { uploader } + describe '#move_to_cache' do + it 'is true' do + expect(uploader.move_to_cache).to eq(true) + end + end - it_behaves_like "builds correct paths", - store_dir: %r[\h{2}/\h{2}], - cache_dir: %r[/lfs-objects/tmp/cache], - work_dir: %r[/lfs-objects/tmp/work] + describe '#move_to_store' do + it 'is true' do + expect(uploader.move_to_store).to eq(true) + end + end + + describe '#store_dir' do + subject { uploader.store_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with("#{lfs_object.oid[0, 2]}/#{lfs_object.oid[2, 2]}") } + end + + describe '#cache_dir' do + subject { uploader.cache_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with('/tmp/cache') } + end + + describe '#work_dir' do + subject { uploader.work_dir } + + it { is_expected.to start_with(path) } + it { is_expected.to end_with('/tmp/work') } + end end diff --git a/spec/uploaders/namespace_file_uploader_spec.rb b/spec/uploaders/namespace_file_uploader_spec.rb index 24a2fc0f72e..c6c4500c179 100644 --- a/spec/uploaders/namespace_file_uploader_spec.rb +++ b/spec/uploaders/namespace_file_uploader_spec.rb @@ -1,16 +1,21 @@ require 'spec_helper' -IDENTIFIER = %r{\h+/\S+} - describe NamespaceFileUploader do let(:group) { build_stubbed(:group) } let(:uploader) { described_class.new(group) } - let(:upload) { create(:upload, :namespace_upload, model: group) } - subject { uploader } + describe "#store_dir" do + it "stores in the namespace id directory" do + expect(uploader.store_dir).to include(group.id.to_s) + end + end + + describe ".absolute_path" do + it "stores in thecorrect directory" do + upload_record = create(:upload, :namespace_upload, model: group) - it_behaves_like 'builds correct paths', - store_dir: %r[uploads/-/system/namespace/\d+], - upload_path: IDENTIFIER, - absolute_path: %r[#{CarrierWave.root}/uploads/-/system/namespace/\d+/#{IDENTIFIER}] + expect(described_class.absolute_path(upload_record)) + .to include("-/system/namespace/#{group.id}") + end + end end diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb index ed1fba6edda..cbafa9f478d 100644 --- a/spec/uploaders/personal_file_uploader_spec.rb +++ b/spec/uploaders/personal_file_uploader_spec.rb @@ -1,27 +1,25 @@ require 'spec_helper' -IDENTIFIER = %r{\h+/\S+} - describe PersonalFileUploader do - let(:model) { create(:personal_snippet) } - let(:uploader) { described_class.new(model) } - let(:upload) { create(:upload, :personal_snippet_upload) } + let(:uploader) { described_class.new(build_stubbed(:project)) } + let(:snippet) { create(:personal_snippet) } - subject { uploader } + describe '.absolute_path' do + it 'returns the correct absolute path by building it dynamically' do + upload = double(model: snippet, path: 'secret/foo.jpg') - it_behaves_like 'builds correct paths', - store_dir: %r[uploads/-/system/personal_snippet/\d+], - upload_path: IDENTIFIER, - absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet/\d+/#{IDENTIFIER}] + dynamic_segment = "personal_snippet/#{snippet.id}" - describe '#to_h' do - before do - subject.instance_variable_set(:@secret, 'secret') + expect(described_class.absolute_path(upload)).to end_with("/-/system/#{dynamic_segment}/secret/foo.jpg") end + end + + describe '#to_h' do + it 'returns the hass' do + uploader = described_class.new(snippet, 'secret') - it 'is correct' do allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name')) - expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name" + expected_url = "/uploads/-/system/personal_snippet/#{snippet.id}/secret/file_name" expect(uploader.to_h).to eq( alt: 'file_name', diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb index 9a3e5d83e01..7ef7fb7d758 100644 --- a/spec/uploaders/records_uploads_spec.rb +++ b/spec/uploaders/records_uploads_spec.rb @@ -3,16 +3,16 @@ require 'rails_helper' describe RecordsUploads do let!(:uploader) do class RecordsUploadsExampleUploader < GitlabUploader - include RecordsUploads::Concern + include RecordsUploads storage :file - def dynamic_segment - 'co/fe/ee' + def model + FactoryBot.build_stubbed(:user) end end - RecordsUploadsExampleUploader.new(build_stubbed(:user)) + RecordsUploadsExampleUploader.new end def upload_fixture(filename) @@ -20,55 +20,48 @@ describe RecordsUploads do end describe 'callbacks' do - let(:upload) { create(:upload) } - - before do - uploader.upload = upload - end - - it '#record_upload after `store`' do + it 'calls `record_upload` after `store`' do expect(uploader).to receive(:record_upload).once uploader.store!(upload_fixture('doc_sample.txt')) end - it '#destroy_upload after `remove`' do + it 'calls `destroy_upload` after `remove`' do + expect(uploader).to receive(:destroy_upload).once + uploader.store!(upload_fixture('doc_sample.txt')) - expect(uploader).to receive(:destroy_upload).once uploader.remove! end end describe '#record_upload callback' do - it 'creates an Upload record after store' do - expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.to change { Upload.count }.by(1) - end + it 'returns early when not using file storage' do + allow(uploader).to receive(:file_storage?).and_return(false) + expect(Upload).not_to receive(:record) - it 'creates a new record and assigns size, path, model, and uploader' do uploader.store!(upload_fixture('rails_sample.jpg')) - - upload = uploader.upload - aggregate_failures do - expect(upload).to be_persisted - expect(upload.size).to eq uploader.file.size - expect(upload.path).to eq uploader.upload_path - expect(upload.model_id).to eq uploader.model.id - expect(upload.model_type).to eq uploader.model.class.to_s - expect(upload.uploader).to eq uploader.class.to_s - end end - it "does not create an Upload record when the file doesn't exist" do + it "returns early when the file doesn't exist" do allow(uploader).to receive(:file).and_return(double(exists?: false)) + expect(Upload).not_to receive(:record) - expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count } + uploader.store!(upload_fixture('rails_sample.jpg')) + end + + it 'creates an Upload record after store' do + expect(Upload).to receive(:record) + .with(uploader) + + uploader.store!(upload_fixture('rails_sample.jpg')) end it 'does not create an Upload record if model is missing' do - allow_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil) + expect_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil) + expect(Upload).not_to receive(:record).with(uploader) - expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count } + uploader.store!(upload_fixture('rails_sample.jpg')) end it 'it destroys Upload records at the same path before recording' do @@ -79,15 +72,29 @@ describe RecordsUploads do uploader: uploader.class.to_s ) - uploader.upload = existing uploader.store!(upload_fixture('rails_sample.jpg')) expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound) - expect(Upload.count).to eq(1) + expect(Upload.count).to eq 1 end end describe '#destroy_upload callback' do + it 'returns early when not using file storage' do + uploader.store!(upload_fixture('rails_sample.jpg')) + + allow(uploader).to receive(:file_storage?).and_return(false) + expect(Upload).not_to receive(:remove_path) + + uploader.remove! + end + + it 'returns early when file is nil' do + expect(Upload).not_to receive(:remove_path) + + uploader.remove! + end + it 'it destroys Upload records at the same path after removal' do uploader.store!(upload_fixture('rails_sample.jpg')) -- cgit v1.2.1