From 7648f113814a78ffde802172197ba2b0074ec753 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 28 Jun 2017 17:33:48 +0200 Subject: [PATCH 1/4] Remove unnecessary contexts --- spec/lib/gitlab/git/repository_spec.rb | 174 ++++++++-------------- spec/lib/gitlab/gitaly_client/ref_spec.rb | 4 - spec/models/environment_spec.rb | 29 ++-- 3 files changed, 75 insertions(+), 132 deletions(-) diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 0cd458bf933..464cb41a842 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -5,6 +5,11 @@ describe Gitlab::Git::Repository, seed_helper: true do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) } + after do + # Prevent cached stubs (gRPC connection objects) from poisoning tests. + Gitlab::GitalyClient.clear_stubs! + end + describe "Respond to" do subject { repository } @@ -30,31 +35,21 @@ describe Gitlab::Git::Repository, seed_helper: true do expect(repository.root_ref.encoding).to eq(Encoding.find('UTF-8')) end - context 'with gitaly enabled' do - before do - stub_gitaly - end + it 'gets the branch name from GitalyClient' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:default_branch_name) + repository.root_ref + end - after do - Gitlab::GitalyClient.clear_stubs! - end + it 'wraps GRPC not found' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:default_branch_name) + .and_raise(GRPC::NotFound) + expect { repository.root_ref }.to raise_error(Gitlab::Git::Repository::NoRepository) + end - it 'gets the branch name from GitalyClient' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:default_branch_name) - repository.root_ref - end - - it 'wraps GRPC not found' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:default_branch_name) - .and_raise(GRPC::NotFound) - expect { repository.root_ref }.to raise_error(Gitlab::Git::Repository::NoRepository) - end - - it 'wraps GRPC exceptions' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:default_branch_name) - .and_raise(GRPC::Unknown) - expect { repository.root_ref }.to raise_error(Gitlab::Git::CommandError) - end + it 'wraps GRPC exceptions' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:default_branch_name) + .and_raise(GRPC::Unknown) + expect { repository.root_ref }.to raise_error(Gitlab::Git::CommandError) end end @@ -135,31 +130,21 @@ describe Gitlab::Git::Repository, seed_helper: true do it { is_expected.to include("master") } it { is_expected.not_to include("branch-from-space") } - context 'with gitaly enabled' do - before do - stub_gitaly - end + it 'gets the branch names from GitalyClient' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names) + subject + end - after do - Gitlab::GitalyClient.clear_stubs! - end + it 'wraps GRPC not found' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names) + .and_raise(GRPC::NotFound) + expect { subject }.to raise_error(Gitlab::Git::Repository::NoRepository) + end - it 'gets the branch names from GitalyClient' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names) - subject - end - - it 'wraps GRPC not found' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names) - .and_raise(GRPC::NotFound) - expect { subject }.to raise_error(Gitlab::Git::Repository::NoRepository) - end - - it 'wraps GRPC other exceptions' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names) - .and_raise(GRPC::Unknown) - expect { subject }.to raise_error(Gitlab::Git::CommandError) - end + it 'wraps GRPC other exceptions' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names) + .and_raise(GRPC::Unknown) + expect { subject }.to raise_error(Gitlab::Git::CommandError) end end @@ -183,31 +168,21 @@ describe Gitlab::Git::Repository, seed_helper: true do it { is_expected.to include("v1.0.0") } it { is_expected.not_to include("v5.0.0") } - context 'with gitaly enabled' do - before do - stub_gitaly - end + it 'gets the tag names from GitalyClient' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names) + subject + end - after do - Gitlab::GitalyClient.clear_stubs! - end + it 'wraps GRPC not found' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names) + .and_raise(GRPC::NotFound) + expect { subject }.to raise_error(Gitlab::Git::Repository::NoRepository) + end - it 'gets the tag names from GitalyClient' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names) - subject - end - - it 'wraps GRPC not found' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names) - .and_raise(GRPC::NotFound) - expect { subject }.to raise_error(Gitlab::Git::Repository::NoRepository) - end - - it 'wraps GRPC exceptions' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names) - .and_raise(GRPC::Unknown) - expect { subject }.to raise_error(Gitlab::Git::CommandError) - end + it 'wraps GRPC exceptions' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names) + .and_raise(GRPC::Unknown) + expect { subject }.to raise_error(Gitlab::Git::CommandError) end end @@ -1281,42 +1256,32 @@ describe Gitlab::Git::Repository, seed_helper: true do expect(@repo.local_branches.any? { |branch| branch.name == 'local_branch' }).to eq(true) end - context 'with gitaly enabled' do - before do - stub_gitaly + it 'returns a Branch with UTF-8 fields' do + branches = @repo.local_branches.to_a + expect(branches.size).to be > 0 + utf_8 = Encoding.find('utf-8') + branches.each do |branch| + expect(branch.name.encoding).to eq(utf_8) + expect(branch.target.encoding).to eq(utf_8) unless branch.target.nil? end + end - after do - Gitlab::GitalyClient.clear_stubs! - end + it 'gets the branches from GitalyClient' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches) + .and_return([]) + @repo.local_branches + end - it 'returns a Branch with UTF-8 fields' do - branches = @repo.local_branches.to_a - expect(branches.size).to be > 0 - utf_8 = Encoding.find('utf-8') - branches.each do |branch| - expect(branch.name.encoding).to eq(utf_8) - expect(branch.target.encoding).to eq(utf_8) unless branch.target.nil? - end - end + it 'wraps GRPC not found' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches) + .and_raise(GRPC::NotFound) + expect { @repo.local_branches }.to raise_error(Gitlab::Git::Repository::NoRepository) + end - it 'gets the branches from GitalyClient' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches) - .and_return([]) - @repo.local_branches - end - - it 'wraps GRPC not found' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches) - .and_raise(GRPC::NotFound) - expect { @repo.local_branches }.to raise_error(Gitlab::Git::Repository::NoRepository) - end - - it 'wraps GRPC exceptions' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches) - .and_raise(GRPC::Unknown) - expect { @repo.local_branches }.to raise_error(Gitlab::Git::CommandError) - end + it 'wraps GRPC exceptions' do + expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches) + .and_raise(GRPC::Unknown) + expect { @repo.local_branches }.to raise_error(Gitlab::Git::CommandError) end end @@ -1395,11 +1360,4 @@ describe Gitlab::Git::Repository, seed_helper: true do sha = Rugged::Commit.create(repo, options) repo.lookup(sha) end - - def stub_gitaly - allow(Gitlab::GitalyClient).to receive(:feature_enabled?).and_return(true) - - stub = double(:stub) - allow(Gitaly::Ref::Stub).to receive(:new).and_return(stub) - end end diff --git a/spec/lib/gitlab/gitaly_client/ref_spec.rb b/spec/lib/gitlab/gitaly_client/ref_spec.rb index 8ad39a02b93..986ae348652 100644 --- a/spec/lib/gitlab/gitaly_client/ref_spec.rb +++ b/spec/lib/gitlab/gitaly_client/ref_spec.rb @@ -6,10 +6,6 @@ describe Gitlab::GitalyClient::Ref do let(:relative_path) { project.path_with_namespace + '.git' } let(:client) { described_class.new(project.repository) } - before do - allow(Gitlab.config.gitaly).to receive(:enabled).and_return(true) - end - after do # When we say `expect_any_instance_of(Gitaly::Ref::Stub)` a double is created, # and because GitalyClient shares stubs these will get passed from example to diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index b0635c6a90a..0a2cd8c2957 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -120,28 +120,17 @@ describe Environment, models: true do let(:head_commit) { project.commit } let(:commit) { project.commit.parent } - context 'Gitaly find_ref_name feature disabled' do - it 'returns deployment id for the environment' do - expect(environment.first_deployment_for(commit)).to eq deployment1 - end - - it 'return nil when no deployment is found' do - expect(environment.first_deployment_for(head_commit)).to eq nil - end + it 'returns deployment id for the environment' do + expect(environment.first_deployment_for(commit)).to eq deployment1 end - # TODO: Uncomment when feature is reenabled - # context 'Gitaly find_ref_name feature enabled' do - # before do - # allow(Gitlab::GitalyClient).to receive(:feature_enabled?).with(:find_ref_name).and_return(true) - # end - # - # it 'calls GitalyClient' do - # expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:find_ref_name) - # - # environment.first_deployment_for(commit) - # end - # end + it 'return nil when no deployment is found' do + expect(environment.first_deployment_for(head_commit)).to eq nil + end + + it 'returns a UTF-8 ref' do + expect(environment.first_deployment_for(commit).ref).to be_utf8 + end end describe '#environment_type' do From 8a62f304ef541b93ac47dab3b69b645f2b65537a Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Wed, 28 Jun 2017 17:44:37 +0200 Subject: [PATCH 2/4] Add a UTF-8 encoding matcher --- spec/lib/gitlab/git/blame_spec.rb | 3 +++ spec/lib/gitlab/git/branch_spec.rb | 2 +- spec/lib/gitlab/git/diff_spec.rb | 2 +- spec/lib/gitlab/git/repository_spec.rb | 11 +++++------ spec/support/matchers/be_utf8.rb | 9 +++++++++ 5 files changed, 19 insertions(+), 8 deletions(-) create mode 100644 spec/support/matchers/be_utf8.rb diff --git a/spec/lib/gitlab/git/blame_spec.rb b/spec/lib/gitlab/git/blame_spec.rb index 8b041ac69b1..66c016d14b3 100644 --- a/spec/lib/gitlab/git/blame_spec.rb +++ b/spec/lib/gitlab/git/blame_spec.rb @@ -20,6 +20,7 @@ describe Gitlab::Git::Blame, seed_helper: true do expect(data.size).to eq(95) expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit) expect(data.first[:line]).to eq("# Contribute to GitLab") + expect(data.first[:line]).to be_utf8 end end @@ -40,6 +41,7 @@ describe Gitlab::Git::Blame, seed_helper: true do expect(data.size).to eq(1) expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit) expect(data.first[:line]).to eq("Ä ü") + expect(data.first[:line]).to be_utf8 end end @@ -61,6 +63,7 @@ describe Gitlab::Git::Blame, seed_helper: true do expect(data.size).to eq(1) expect(data.first[:commit]).to be_kind_of(Gitlab::Git::Commit) expect(data.first[:line]).to eq(" ") + expect(data.first[:line]).to be_utf8 end end end diff --git a/spec/lib/gitlab/git/branch_spec.rb b/spec/lib/gitlab/git/branch_spec.rb index 9dba4397e79..d1d7ed1d02a 100644 --- a/spec/lib/gitlab/git/branch_spec.rb +++ b/spec/lib/gitlab/git/branch_spec.rb @@ -48,7 +48,7 @@ describe Gitlab::Git::Branch, seed_helper: true do expect(Gitlab::Git::Commit).to receive(:decorate) .with(hash_including(attributes)).and_call_original - expect(branch.dereferenced_target.message.encoding).to be(Encoding::UTF_8) + expect(branch.dereferenced_target.message).to be_utf8 end end diff --git a/spec/lib/gitlab/git/diff_spec.rb b/spec/lib/gitlab/git/diff_spec.rb index d50ccb0df30..d97e85364c2 100644 --- a/spec/lib/gitlab/git/diff_spec.rb +++ b/spec/lib/gitlab/git/diff_spec.rb @@ -180,7 +180,7 @@ EOT let(:raw_patch) { @raw_diff_hash[:diff].encode(Encoding::ASCII_8BIT) } it 'encodes diff patch to UTF-8' do - expect(diff.diff.encoding).to eq(Encoding::UTF_8) + expect(diff.diff).to be_utf8 end end end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 464cb41a842..9e924c2541b 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -32,7 +32,7 @@ describe Gitlab::Git::Repository, seed_helper: true do end it 'returns UTF-8' do - expect(repository.root_ref.encoding).to eq(Encoding.find('UTF-8')) + expect(repository.root_ref).to be_utf8 end it 'gets the branch name from GitalyClient' do @@ -124,7 +124,7 @@ describe Gitlab::Git::Repository, seed_helper: true do end it 'returns UTF-8' do - expect(subject.first.encoding).to eq(Encoding.find('UTF-8')) + expect(subject.first).to be_utf8 end it { is_expected.to include("master") } @@ -158,7 +158,7 @@ describe Gitlab::Git::Repository, seed_helper: true do end it 'returns UTF-8' do - expect(subject.first.encoding).to eq(Encoding.find('UTF-8')) + expect(subject.first).to be_utf8 end describe '#last' do @@ -1259,10 +1259,9 @@ describe Gitlab::Git::Repository, seed_helper: true do it 'returns a Branch with UTF-8 fields' do branches = @repo.local_branches.to_a expect(branches.size).to be > 0 - utf_8 = Encoding.find('utf-8') branches.each do |branch| - expect(branch.name.encoding).to eq(utf_8) - expect(branch.target.encoding).to eq(utf_8) unless branch.target.nil? + expect(branch.name).to be_utf8 + expect(branch.target).to be_utf8 unless branch.target.nil? end end diff --git a/spec/support/matchers/be_utf8.rb b/spec/support/matchers/be_utf8.rb new file mode 100644 index 00000000000..ea806352422 --- /dev/null +++ b/spec/support/matchers/be_utf8.rb @@ -0,0 +1,9 @@ +RSpec::Matchers.define :be_utf8 do |_| + match do |actual| + actual.is_a?(String) && actual.encoding == Encoding.find('UTF-8') + end + + description do + "be a String with encoding UTF-8" + end +end From f39fe34afb4a69ebca462219abb881e243f4a955 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 29 Jun 2017 14:11:42 +0200 Subject: [PATCH 3/4] Add test for GitalyClient::Ref#find_ref_name --- lib/gitlab/gitaly_client/ref.rb | 2 +- spec/lib/gitlab/gitaly_client/ref_spec.rb | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/gitaly_client/ref.rb b/lib/gitlab/gitaly_client/ref.rb index 2d61992f595..6edc69de078 100644 --- a/lib/gitlab/gitaly_client/ref.rb +++ b/lib/gitlab/gitaly_client/ref.rb @@ -34,7 +34,7 @@ module Gitlab commit_id: commit_id, prefix: ref_prefix ) - GitalyClient.call(@storage, :ref, :find_ref_name, request).name + encode!(GitalyClient.call(@storage, :ref, :find_ref_name, request).name.dup) end def count_tag_names diff --git a/spec/lib/gitlab/gitaly_client/ref_spec.rb b/spec/lib/gitlab/gitaly_client/ref_spec.rb index 986ae348652..b7de7e598e3 100644 --- a/spec/lib/gitlab/gitaly_client/ref_spec.rb +++ b/spec/lib/gitlab/gitaly_client/ref_spec.rb @@ -78,4 +78,13 @@ describe Gitlab::GitalyClient::Ref do expect { client.local_branches(sort_by: 'invalid_sort') }.to raise_error(ArgumentError) end end + + describe '#find_ref_name', seed_helper: true do + let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) } + let(:client) { described_class.new(repository) } + subject { client.find_ref_name(SeedRepo::Commit::ID, 'refs/heads/master') } + + it { should be_utf8 } + it { should eq('refs/heads/master') } + end end From ec35a9e8609c3e110e025629f5058433ce06b123 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 29 Jun 2017 14:37:54 +0200 Subject: [PATCH 4/4] Remove unnecessary clear_stubs calls --- spec/lib/gitlab/git/repository_spec.rb | 5 ----- spec/lib/gitlab/gitaly_client/ref_spec.rb | 7 ------- 2 files changed, 12 deletions(-) diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 9e924c2541b..f1cdd86edb9 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -5,11 +5,6 @@ describe Gitlab::Git::Repository, seed_helper: true do let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH) } - after do - # Prevent cached stubs (gRPC connection objects) from poisoning tests. - Gitlab::GitalyClient.clear_stubs! - end - describe "Respond to" do subject { repository } diff --git a/spec/lib/gitlab/gitaly_client/ref_spec.rb b/spec/lib/gitlab/gitaly_client/ref_spec.rb index b7de7e598e3..df22fcad902 100644 --- a/spec/lib/gitlab/gitaly_client/ref_spec.rb +++ b/spec/lib/gitlab/gitaly_client/ref_spec.rb @@ -6,13 +6,6 @@ describe Gitlab::GitalyClient::Ref do let(:relative_path) { project.path_with_namespace + '.git' } let(:client) { described_class.new(project.repository) } - after do - # When we say `expect_any_instance_of(Gitaly::Ref::Stub)` a double is created, - # and because GitalyClient shares stubs these will get passed from example to - # example, which will cause an error, so we clean the stubs after each example. - Gitlab::GitalyClient.clear_stubs! - end - describe '#branch_names' do it 'sends a find_all_branch_names message' do expect_any_instance_of(Gitaly::Ref::Stub)