diff options
Diffstat (limited to 'spec')
29 files changed, 1048 insertions, 328 deletions
diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 90c35e2c7f8..93f4903119c 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -40,6 +40,7 @@ FactoryGirl.define do transient do line_number 14 + diff_refs { noteable.try(:diff_refs) } end position do @@ -48,7 +49,7 @@ FactoryGirl.define do new_path: "files/ruby/popen.rb", old_line: nil, new_line: line_number, - diff_refs: noteable.try(:diff_refs) + diff_refs: diff_refs ) end diff --git a/spec/features/discussion_comments/commit_spec.rb b/spec/features/discussion_comments/commit_spec.rb new file mode 100644 index 00000000000..96e0b78f6b9 --- /dev/null +++ b/spec/features/discussion_comments/commit_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe 'Discussion Comments Merge Request', :feature, :js do + include RepoHelpers + + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:merge_request) { create(:merge_request, source_project: project) } + + before do + project.add_master(user) + login_as(user) + + visit namespace_project_commit_path(project.namespace, project, sample_commit.id) + end + + it_behaves_like 'discussion comments', 'commit' +end diff --git a/spec/features/discussion_comments/issue_spec.rb b/spec/features/discussion_comments/issue_spec.rb new file mode 100644 index 00000000000..ccc9efccd18 --- /dev/null +++ b/spec/features/discussion_comments/issue_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe 'Discussion Comments Issue', :feature, :js do + let(:user) { create(:user) } + let(:project) { create(:empty_project) } + let(:issue) { create(:issue, project: project) } + + before do + project.add_master(user) + login_as(user) + + visit namespace_project_issue_path(project.namespace, project, issue) + end + + it_behaves_like 'discussion comments', 'issue' +end diff --git a/spec/features/discussion_comments/merge_request_spec.rb b/spec/features/discussion_comments/merge_request_spec.rb new file mode 100644 index 00000000000..f99ebeb9cd9 --- /dev/null +++ b/spec/features/discussion_comments/merge_request_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe 'Discussion Comments Merge Request', :feature, :js do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:merge_request) { create(:merge_request, source_project: project) } + + before do + project.add_master(user) + login_as(user) + + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + end + + it_behaves_like 'discussion comments', 'merge request' +end diff --git a/spec/features/discussion_comments/snippets_spec.rb b/spec/features/discussion_comments/snippets_spec.rb new file mode 100644 index 00000000000..19a306511b2 --- /dev/null +++ b/spec/features/discussion_comments/snippets_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe 'Discussion Comments Issue', :feature, :js do + let(:user) { create(:user) } + let(:project) { create(:empty_project) } + let(:snippet) { create(:project_snippet, :private, project: project, author: user) } + + before do + project.add_master(user) + login_as(user) + + visit namespace_project_snippet_path(project.namespace, project, snippet) + end + + it_behaves_like 'discussion comments', 'snippet' +end diff --git a/spec/features/merge_requests/discussion_spec.rb b/spec/features/merge_requests/discussion_spec.rb new file mode 100644 index 00000000000..f59d0faa274 --- /dev/null +++ b/spec/features/merge_requests/discussion_spec.rb @@ -0,0 +1,51 @@ +require 'spec_helper' + +feature 'Merge Request Discussions', feature: true do + before do + login_as :admin + end + + context "Diff discussions" do + let(:merge_request) { create(:merge_request, importing: true) } + let(:project) { merge_request.source_project } + let!(:old_merge_request_diff) { merge_request.merge_request_diffs.create(diff_refs: outdated_diff_refs) } + let!(:new_merge_request_diff) { merge_request.merge_request_diffs.create } + + let!(:outdated_discussion) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: outdated_position).to_discussion } + let!(:active_discussion) { create(:diff_note_on_merge_request, noteable: merge_request, project: project).to_discussion } + + let(:outdated_position) do + Gitlab::Diff::Position.new( + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 9, + diff_refs: outdated_diff_refs + ) + end + + let(:outdated_diff_refs) { project.commit("874797c3a73b60d2187ed6e2fcabd289ff75171e").diff_refs } + + before(:each) do + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + end + + context 'active discussions' do + it 'shows a link to the diff' do + within(".discussion[data-discussion-id='#{active_discussion.id}']") do + path = diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, anchor: active_discussion.line_code) + expect(page).to have_link('the diff', href: path) + end + end + end + + context 'outdated discussions' do + it 'shows a link to the outdated diff' do + within(".discussion[data-discussion-id='#{outdated_discussion.id}']") do + path = diffs_namespace_project_merge_request_path(project.namespace, project, merge_request, diff_id: old_merge_request_diff.id, anchor: outdated_discussion.line_code) + expect(page).to have_link('an outdated diff', href: path) + end + end + end + end +end diff --git a/spec/features/merge_requests/merge_request_versions_spec.rb b/spec/features/merge_requests/versions_spec.rb index 04e85ed3f73..68a68f5d3f3 100644 --- a/spec/features/merge_requests/merge_request_versions_spec.rb +++ b/spec/features/merge_requests/versions_spec.rb @@ -36,8 +36,23 @@ feature 'Merge Request versions', js: true, feature: true do expect(page).to have_content '5 changed files' end - it 'show the message about disabled comments' do - expect(page).to have_content 'Comments are disabled' + it 'show the message about disabled comment creation' do + expect(page).to have_content 'comment creation is disabled' + end + + it 'shows comments that were last relevant at that version' do + position = Gitlab::Diff::Position.new( + old_path: ".gitmodules", + new_path: ".gitmodules", + old_line: nil, + new_line: 4, + diff_refs: merge_request_diff1.diff_refs + ) + outdated_diff_note = create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) + outdated_diff_note.position = outdated_diff_note.original_position + outdated_diff_note.save! + + expect(page).to have_css(".diffs .notes[data-discussion-id='#{outdated_diff_note.discussion_id}']") end end diff --git a/spec/javascripts/issue_spec.js b/spec/javascripts/issue_spec.js index aabc8bea12f..9a2570ef7e9 100644 --- a/spec/javascripts/issue_spec.js +++ b/spec/javascripts/issue_spec.js @@ -1,18 +1,17 @@ -/* eslint-disable space-before-function-paren, no-var, one-var, one-var-declaration-per-line, no-use-before-define, comma-dangle, max-len */ +/* eslint-disable space-before-function-paren, one-var, one-var-declaration-per-line, no-use-before-define, comma-dangle, max-len */ import Issue from '~/issue'; require('~/lib/utils/text_utility'); describe('Issue', function() { - var INVALID_URL = 'http://goesnowhere.nothing/whereami'; - var $boxClosed, $boxOpen, $btnClose, $btnReopen; + let $boxClosed, $boxOpen, $btnClose, $btnReopen; preloadFixtures('issues/closed-issue.html.raw'); preloadFixtures('issues/issue-with-task-list.html.raw'); preloadFixtures('issues/open-issue.html.raw'); function expectErrorMessage() { - var $flashMessage = $('div.flash-alert'); + const $flashMessage = $('div.flash-alert'); expect($flashMessage).toExist(); expect($flashMessage).toBeVisible(); expect($flashMessage).toHaveText('Unable to update this issue at this time.'); @@ -26,10 +25,28 @@ describe('Issue', function() { expectVisibility($btnReopen, !isIssueOpen); } - function expectPendingRequest(req, $triggeredButton) { - expect(req.type).toBe('PUT'); - expect(req.url).toBe($triggeredButton.attr('href')); - expect($triggeredButton).toHaveProp('disabled', true); + function expectNewBranchButtonState(isPending, canCreate) { + if (Issue.$btnNewBranch.length === 0) { + return; + } + + const $available = Issue.$btnNewBranch.find('.available'); + expect($available).toHaveText('New branch'); + + if (!isPending && canCreate) { + expect($available).toBeVisible(); + } else { + expect($available).toBeHidden(); + } + + const $unavailable = Issue.$btnNewBranch.find('.unavailable'); + expect($unavailable).toHaveText('New branch unavailable'); + + if (!isPending && !canCreate) { + expect($unavailable).toBeVisible(); + } else { + expect($unavailable).toBeHidden(); + } } function expectVisibility($element, shouldBeVisible) { @@ -81,100 +98,107 @@ describe('Issue', function() { }); }); - describe('close issue', function() { - beforeEach(function() { - loadFixtures('issues/open-issue.html.raw'); - findElements(); - this.issue = new Issue(); - - expectIssueState(true); - }); + [true, false].forEach((isIssueInitiallyOpen) => { + describe(`with ${isIssueInitiallyOpen ? 'open' : 'closed'} issue`, function() { + const action = isIssueInitiallyOpen ? 'close' : 'reopen'; + + function ajaxSpy(req) { + if (req.url === this.$triggeredButton.attr('href')) { + expect(req.type).toBe('PUT'); + expect(this.$triggeredButton).toHaveProp('disabled', true); + expectNewBranchButtonState(true, false); + return this.issueStateDeferred; + } else if (req.url === Issue.$btnNewBranch.data('path')) { + expect(req.type).toBe('get'); + expectNewBranchButtonState(true, false); + return this.canCreateBranchDeferred; + } + + expect(req.url).toBe('unexpected'); + return null; + } + + beforeEach(function() { + if (isIssueInitiallyOpen) { + loadFixtures('issues/open-issue.html.raw'); + } else { + loadFixtures('issues/closed-issue.html.raw'); + } + + findElements(); + this.issue = new Issue(); + expectIssueState(isIssueInitiallyOpen); + this.$triggeredButton = isIssueInitiallyOpen ? $btnClose : $btnReopen; + + this.$projectIssuesCounter = $('.issue_counter'); + this.$projectIssuesCounter.text('1,001'); + + this.issueStateDeferred = new jQuery.Deferred(); + this.canCreateBranchDeferred = new jQuery.Deferred(); + + spyOn(jQuery, 'ajax').and.callFake(ajaxSpy.bind(this)); + }); - it('closes an issue', function() { - spyOn(jQuery, 'ajax').and.callFake(function(req) { - expectPendingRequest(req, $btnClose); - req.success({ + it(`${action}s the issue`, function() { + this.$triggeredButton.trigger('click'); + this.issueStateDeferred.resolve({ id: 34 }); - }); - - $btnClose.trigger('click'); + this.canCreateBranchDeferred.resolve({ + can_create_branch: !isIssueInitiallyOpen + }); - expectIssueState(false); - expect($btnClose).toHaveProp('disabled', false); - expect($('.issue_counter')).toHaveText(0); - }); + expectIssueState(!isIssueInitiallyOpen); + expect(this.$triggeredButton).toHaveProp('disabled', false); + expect(this.$projectIssuesCounter.text()).toBe(isIssueInitiallyOpen ? '1,000' : '1,002'); + expectNewBranchButtonState(false, !isIssueInitiallyOpen); + }); - it('fails to close an issue with success:false', function() { - spyOn(jQuery, 'ajax').and.callFake(function(req) { - expectPendingRequest(req, $btnClose); - req.success({ + it(`fails to ${action} the issue if saved:false`, function() { + this.$triggeredButton.trigger('click'); + this.issueStateDeferred.resolve({ saved: false }); - }); - - $btnClose.attr('href', INVALID_URL); - $btnClose.trigger('click'); - - expectIssueState(true); - expect($btnClose).toHaveProp('disabled', false); - expectErrorMessage(); - expect($('.issue_counter')).toHaveText(1); - }); + this.canCreateBranchDeferred.resolve({ + can_create_branch: isIssueInitiallyOpen + }); - it('fails to closes an issue with HTTP error', function() { - spyOn(jQuery, 'ajax').and.callFake(function(req) { - expectPendingRequest(req, $btnClose); - req.error(); + expectIssueState(isIssueInitiallyOpen); + expect(this.$triggeredButton).toHaveProp('disabled', false); + expectErrorMessage(); + expect(this.$projectIssuesCounter.text()).toBe('1,001'); + expectNewBranchButtonState(false, isIssueInitiallyOpen); }); - $btnClose.attr('href', INVALID_URL); - $btnClose.trigger('click'); - - expectIssueState(true); - expect($btnClose).toHaveProp('disabled', true); - expectErrorMessage(); - expect($('.issue_counter')).toHaveText(1); - }); - - it('updates counter', () => { - spyOn(jQuery, 'ajax').and.callFake(function(req) { - expectPendingRequest(req, $btnClose); - req.success({ - id: 34 + it(`fails to ${action} the issue if HTTP error occurs`, function() { + this.$triggeredButton.trigger('click'); + this.issueStateDeferred.reject(); + this.canCreateBranchDeferred.resolve({ + can_create_branch: isIssueInitiallyOpen }); - }); - expect($('.issue_counter')).toHaveText(1); - $('.issue_counter').text('1,001'); - expect($('.issue_counter').text()).toEqual('1,001'); - $btnClose.trigger('click'); - expect($('.issue_counter').text()).toEqual('1,000'); - }); - }); + expectIssueState(isIssueInitiallyOpen); + expect(this.$triggeredButton).toHaveProp('disabled', true); + expectErrorMessage(); + expect(this.$projectIssuesCounter.text()).toBe('1,001'); + expectNewBranchButtonState(false, isIssueInitiallyOpen); + }); - describe('reopen issue', function() { - beforeEach(function() { - loadFixtures('issues/closed-issue.html.raw'); - findElements(); - this.issue = new Issue(); + it('disables the new branch button if Ajax call fails', function() { + this.$triggeredButton.trigger('click'); + this.issueStateDeferred.reject(); + this.canCreateBranchDeferred.reject(); - expectIssueState(false); - }); - - it('reopens an issue', function() { - spyOn(jQuery, 'ajax').and.callFake(function(req) { - expectPendingRequest(req, $btnReopen); - req.success({ - id: 34 - }); + expectNewBranchButtonState(false, false); }); - $btnReopen.trigger('click'); + it('does not trigger Ajax call if new branch button is missing', function() { + Issue.$btnNewBranch = $(); + this.canCreateBranchDeferred = null; - expectIssueState(true); - expect($btnReopen).toHaveProp('disabled', false); - expect($('.issue_counter')).toHaveText(1); + this.$triggeredButton.trigger('click'); + this.issueStateDeferred.reject(); + }); }); }); }); diff --git a/spec/javascripts/lib/utils/poll_spec.js b/spec/javascripts/lib/utils/poll_spec.js index e3429c2a1cb..918b6d32c43 100644 --- a/spec/javascripts/lib/utils/poll_spec.js +++ b/spec/javascripts/lib/utils/poll_spec.js @@ -4,6 +4,20 @@ import Poll from '~/lib/utils/poll'; Vue.use(VueResource); +const waitForAllCallsToFinish = (service, waitForCount, successCallback) => { + const timer = () => { + setTimeout(() => { + if (service.fetch.calls.count() === waitForCount) { + successCallback(); + } else { + timer(); + } + }, 5); + }; + + timer(); +}; + class ServiceMock { constructor(endpoint) { this.service = Vue.resource(endpoint); @@ -16,6 +30,7 @@ class ServiceMock { describe('Poll', () => { let callbacks; + let service; beforeEach(() => { callbacks = { @@ -23,8 +38,11 @@ describe('Poll', () => { error: () => {}, }; + service = new ServiceMock('endpoint'); + spyOn(callbacks, 'success'); spyOn(callbacks, 'error'); + spyOn(service, 'fetch').and.callThrough(); }); it('calls the success callback when no header for interval is provided', (done) => { @@ -35,19 +53,20 @@ describe('Poll', () => { Vue.http.interceptors.push(successInterceptor); new Poll({ - resource: new ServiceMock('endpoint'), + resource: service, method: 'fetch', successCallback: callbacks.success, errorCallback: callbacks.error, }).makeRequest(); - setTimeout(() => { + waitForAllCallsToFinish(service, 1, () => { expect(callbacks.success).toHaveBeenCalled(); expect(callbacks.error).not.toHaveBeenCalled(); + + Vue.http.interceptors = _.without(Vue.http.interceptors, successInterceptor); + done(); }, 0); - - Vue.http.interceptors = _.without(Vue.http.interceptors, successInterceptor); }); it('calls the error callback whe the http request returns an error', (done) => { @@ -58,19 +77,19 @@ describe('Poll', () => { Vue.http.interceptors.push(errorInterceptor); new Poll({ - resource: new ServiceMock('endpoint'), + resource: service, method: 'fetch', successCallback: callbacks.success, errorCallback: callbacks.error, }).makeRequest(); - setTimeout(() => { + waitForAllCallsToFinish(service, 1, () => { expect(callbacks.success).not.toHaveBeenCalled(); expect(callbacks.error).toHaveBeenCalled(); - done(); - }, 0); + Vue.http.interceptors = _.without(Vue.http.interceptors, errorInterceptor); - Vue.http.interceptors = _.without(Vue.http.interceptors, errorInterceptor); + done(); + }); }); it('should call the success callback when the interval header is -1', (done) => { @@ -81,7 +100,7 @@ describe('Poll', () => { Vue.http.interceptors.push(intervalInterceptor); new Poll({ - resource: new ServiceMock('endpoint'), + resource: service, method: 'fetch', successCallback: callbacks.success, errorCallback: callbacks.error, @@ -90,10 +109,11 @@ describe('Poll', () => { setTimeout(() => { expect(callbacks.success).toHaveBeenCalled(); expect(callbacks.error).not.toHaveBeenCalled(); + + Vue.http.interceptors = _.without(Vue.http.interceptors, intervalInterceptor); + done(); }, 0); - - Vue.http.interceptors = _.without(Vue.http.interceptors, intervalInterceptor); }); it('starts polling when http status is 200 and interval header is provided', (done) => { @@ -103,26 +123,28 @@ describe('Poll', () => { Vue.http.interceptors.push(pollInterceptor); - const service = new ServiceMock('endpoint'); - spyOn(service, 'fetch').and.callThrough(); - - new Poll({ + const Polling = new Poll({ resource: service, method: 'fetch', data: { page: 1 }, successCallback: callbacks.success, errorCallback: callbacks.error, - }).makeRequest(); + }); + + Polling.makeRequest(); + + waitForAllCallsToFinish(service, 2, () => { + Polling.stop(); - setTimeout(() => { expect(service.fetch.calls.count()).toEqual(2); expect(service.fetch).toHaveBeenCalledWith({ page: 1 }); expect(callbacks.success).toHaveBeenCalled(); expect(callbacks.error).not.toHaveBeenCalled(); - done(); - }, 5); - Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + + done(); + }); }); describe('stop', () => { @@ -133,9 +155,6 @@ describe('Poll', () => { Vue.http.interceptors.push(pollInterceptor); - const service = new ServiceMock('endpoint'); - spyOn(service, 'fetch').and.callThrough(); - const Polling = new Poll({ resource: service, method: 'fetch', @@ -150,14 +169,15 @@ describe('Poll', () => { Polling.makeRequest(); - setTimeout(() => { + waitForAllCallsToFinish(service, 1, () => { expect(service.fetch.calls.count()).toEqual(1); expect(service.fetch).toHaveBeenCalledWith({ page: 1 }); expect(Polling.stop).toHaveBeenCalled(); - done(); - }, 100); - Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + + done(); + }); }); }); @@ -169,10 +189,6 @@ describe('Poll', () => { Vue.http.interceptors.push(pollInterceptor); - const service = new ServiceMock('endpoint'); - - spyOn(service, 'fetch').and.callThrough(); - const Polling = new Poll({ resource: service, method: 'fetch', @@ -187,17 +203,22 @@ describe('Poll', () => { }); spyOn(Polling, 'stop').and.callThrough(); + spyOn(Polling, 'restart').and.callThrough(); Polling.makeRequest(); - setTimeout(() => { + waitForAllCallsToFinish(service, 2, () => { + Polling.stop(); + expect(service.fetch.calls.count()).toEqual(2); expect(service.fetch).toHaveBeenCalledWith({ page: 1 }); expect(Polling.stop).toHaveBeenCalled(); - done(); - }, 10); + expect(Polling.restart).toHaveBeenCalled(); - Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + + done(); + }); }); }); }); diff --git a/spec/lib/gitlab/ci/trace/stream_spec.rb b/spec/lib/gitlab/ci/trace/stream_spec.rb index f1a1a71c528..2e57ccef182 100644 --- a/spec/lib/gitlab/ci/trace/stream_spec.rb +++ b/spec/lib/gitlab/ci/trace/stream_spec.rb @@ -167,7 +167,7 @@ describe Gitlab::Ci::Trace::Stream do let(:data) { 'Coverage 1033 / 1051 LOC (98.29%) covered' } let(:regex) { '\(\d+.\d+\%\) covered' } - it { is_expected.to eq(98.29) } + it { is_expected.to eq("98.29") } end context 'valid content & bad regex' do @@ -188,14 +188,14 @@ describe Gitlab::Ci::Trace::Stream do let(:data) { ' (98.39%) covered. (98.29%) covered' } let(:regex) { '\(\d+.\d+\%\) covered' } - it { is_expected.to eq(98.29) } + it { is_expected.to eq("98.29") } end context 'using a regex capture' do let(:data) { 'TOTAL 9926 3489 65%' } let(:regex) { 'TOTAL\s+\d+\s+\d+\s+(\d{1,3}\%)' } - it { is_expected.to eq(65) } + it { is_expected.to eq("65") } end end end diff --git a/spec/lib/gitlab/ci/trace_spec.rb b/spec/lib/gitlab/ci/trace_spec.rb index 69e8dc9220d..9cb0b62590a 100644 --- a/spec/lib/gitlab/ci/trace_spec.rb +++ b/spec/lib/gitlab/ci/trace_spec.rb @@ -40,12 +40,24 @@ describe Gitlab::Ci::Trace do describe '#extract_coverage' do let(:regex) { '\(\d+.\d+\%\) covered' } - before do - trace.set('Coverage 1033 / 1051 LOC (98.29%) covered') + context 'matching coverage' do + before do + trace.set('Coverage 1033 / 1051 LOC (98.29%) covered') + end + + it "returns valid coverage" do + expect(trace.extract_coverage(regex)).to eq("98.29") + end end - it "returns valid coverage" do - expect(trace.extract_coverage(regex)).to eq(98.29) + context 'no coverage' do + before do + trace.set('No coverage') + end + + it 'returs nil' do + expect(trace.extract_coverage(regex)).to be_nil + end end end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 4ac79454647..a044b871730 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -175,6 +175,50 @@ describe Gitlab::Database::MigrationHelpers, lib: true do end end + describe '#true_value' do + context 'using PostgreSQL' do + before do + expect(Gitlab::Database).to receive(:postgresql?).and_return(true) + end + + it 'returns the appropriate value' do + expect(model.true_value).to eq("'t'") + end + end + + context 'using MySQL' do + before do + expect(Gitlab::Database).to receive(:postgresql?).and_return(false) + end + + it 'returns the appropriate value' do + expect(model.true_value).to eq(1) + end + end + end + + describe '#false_value' do + context 'using PostgreSQL' do + before do + expect(Gitlab::Database).to receive(:postgresql?).and_return(true) + end + + it 'returns the appropriate value' do + expect(model.false_value).to eq("'f'") + end + end + + context 'using MySQL' do + before do + expect(Gitlab::Database).to receive(:postgresql?).and_return(false) + end + + it 'returns the appropriate value' do + expect(model.false_value).to eq(0) + end + end + end + describe '#update_column_in_batches' do before do create_list(:empty_project, 5) @@ -294,4 +338,392 @@ describe Gitlab::Database::MigrationHelpers, lib: true do end end end + + describe '#rename_column_concurrently' do + context 'in a transaction' do + it 'raises RuntimeError' do + allow(model).to receive(:transaction_open?).and_return(true) + + expect { model.rename_column_concurrently(:users, :old, :new) }. + to raise_error(RuntimeError) + end + end + + context 'outside a transaction' do + let(:old_column) do + double(:column, + type: :integer, + limit: 8, + default: 0, + null: false, + precision: 5, + scale: 1) + end + + let(:trigger_name) { model.rename_trigger_name(:users, :old, :new) } + + before do + allow(model).to receive(:transaction_open?).and_return(false) + allow(model).to receive(:column_for).and_return(old_column) + + # Since MySQL and PostgreSQL use different quoting styles we'll just + # stub the methods used for this to make testing easier. + allow(model).to receive(:quote_column_name) { |name| name.to_s } + allow(model).to receive(:quote_table_name) { |name| name.to_s } + end + + context 'using MySQL' do + it 'renames a column concurrently' do + allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + + expect(model).to receive(:install_rename_triggers_for_mysql). + with(trigger_name, 'users', 'old', 'new') + + expect(model).to receive(:add_column). + with(:users, :new, :integer, + limit: old_column.limit, + default: old_column.default, + null: old_column.null, + precision: old_column.precision, + scale: old_column.scale) + + expect(model).to receive(:update_column_in_batches) + + expect(model).to receive(:copy_indexes).with(:users, :old, :new) + expect(model).to receive(:copy_foreign_keys).with(:users, :old, :new) + + model.rename_column_concurrently(:users, :old, :new) + end + end + + context 'using PostgreSQL' do + it 'renames a column concurrently' do + allow(Gitlab::Database).to receive(:postgresql?).and_return(true) + + expect(model).to receive(:install_rename_triggers_for_postgresql). + with(trigger_name, 'users', 'old', 'new') + + expect(model).to receive(:add_column). + with(:users, :new, :integer, + limit: old_column.limit, + default: old_column.default, + null: old_column.null, + precision: old_column.precision, + scale: old_column.scale) + + expect(model).to receive(:update_column_in_batches) + + expect(model).to receive(:copy_indexes).with(:users, :old, :new) + expect(model).to receive(:copy_foreign_keys).with(:users, :old, :new) + + model.rename_column_concurrently(:users, :old, :new) + end + end + end + end + + describe '#cleanup_concurrent_column_rename' do + it 'cleans up the renaming procedure for PostgreSQL' do + allow(Gitlab::Database).to receive(:postgresql?).and_return(true) + + expect(model).to receive(:remove_rename_triggers_for_postgresql). + with(:users, /trigger_.{12}/) + + expect(model).to receive(:remove_column).with(:users, :old) + + model.cleanup_concurrent_column_rename(:users, :old, :new) + end + + it 'cleans up the renaming procedure for MySQL' do + allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + + expect(model).to receive(:remove_rename_triggers_for_mysql). + with(/trigger_.{12}/) + + expect(model).to receive(:remove_column).with(:users, :old) + + model.cleanup_concurrent_column_rename(:users, :old, :new) + end + end + + describe '#change_column_type_concurrently' do + it 'changes the column type' do + expect(model).to receive(:rename_column_concurrently). + with('users', 'username', 'username_for_type_change', type: :text) + + model.change_column_type_concurrently('users', 'username', :text) + end + end + + describe '#cleanup_concurrent_column_type_change' do + it 'cleans up the type changing procedure' do + expect(model).to receive(:cleanup_concurrent_column_rename). + with('users', 'username', 'username_for_type_change') + + expect(model).to receive(:rename_column). + with('users', 'username_for_type_change', 'username') + + model.cleanup_concurrent_column_type_change('users', 'username') + end + end + + describe '#install_rename_triggers_for_postgresql' do + it 'installs the triggers for PostgreSQL' do + expect(model).to receive(:execute). + with(/CREATE OR REPLACE FUNCTION foo()/m) + + expect(model).to receive(:execute). + with(/CREATE TRIGGER foo/m) + + model.install_rename_triggers_for_postgresql('foo', :users, :old, :new) + end + end + + describe '#install_rename_triggers_for_mysql' do + it 'installs the triggers for MySQL' do + expect(model).to receive(:execute). + with(/CREATE TRIGGER foo_insert.+ON users/m) + + expect(model).to receive(:execute). + with(/CREATE TRIGGER foo_update.+ON users/m) + + model.install_rename_triggers_for_mysql('foo', :users, :old, :new) + end + end + + describe '#remove_rename_triggers_for_postgresql' do + it 'removes the function and trigger' do + expect(model).to receive(:execute).with('DROP TRIGGER foo ON bar') + expect(model).to receive(:execute).with('DROP FUNCTION foo()') + + model.remove_rename_triggers_for_postgresql('bar', 'foo') + end + end + + describe '#remove_rename_triggers_for_mysql' do + it 'removes the triggers' do + expect(model).to receive(:execute).with('DROP TRIGGER foo_insert') + expect(model).to receive(:execute).with('DROP TRIGGER foo_update') + + model.remove_rename_triggers_for_mysql('foo') + end + end + + describe '#rename_trigger_name' do + it 'returns a String' do + expect(model.rename_trigger_name(:users, :foo, :bar)). + to match(/trigger_.{12}/) + end + end + + describe '#indexes_for' do + it 'returns the indexes for a column' do + idx1 = double(:idx, columns: %w(project_id)) + idx2 = double(:idx, columns: %w(user_id)) + + allow(model).to receive(:indexes).with('table').and_return([idx1, idx2]) + + expect(model.indexes_for('table', :user_id)).to eq([idx2]) + end + end + + describe '#foreign_keys_for' do + it 'returns the foreign keys for a column' do + fk1 = double(:fk, column: 'project_id') + fk2 = double(:fk, column: 'user_id') + + allow(model).to receive(:foreign_keys).with('table').and_return([fk1, fk2]) + + expect(model.foreign_keys_for('table', :user_id)).to eq([fk2]) + end + end + + describe '#copy_indexes' do + context 'using a regular index using a single column' do + it 'copies the index' do + index = double(:index, + columns: %w(project_id), + name: 'index_on_issues_project_id', + using: nil, + where: nil, + opclasses: {}, + unique: false, + lengths: [], + orders: []) + + allow(model).to receive(:indexes_for).with(:issues, 'project_id'). + and_return([index]) + + expect(model).to receive(:add_concurrent_index). + with(:issues, + %w(gl_project_id), + unique: false, + name: 'index_on_issues_gl_project_id', + length: [], + order: []) + + model.copy_indexes(:issues, :project_id, :gl_project_id) + end + end + + context 'using a regular index with multiple columns' do + it 'copies the index' do + index = double(:index, + columns: %w(project_id foobar), + name: 'index_on_issues_project_id_foobar', + using: nil, + where: nil, + opclasses: {}, + unique: false, + lengths: [], + orders: []) + + allow(model).to receive(:indexes_for).with(:issues, 'project_id'). + and_return([index]) + + expect(model).to receive(:add_concurrent_index). + with(:issues, + %w(gl_project_id foobar), + unique: false, + name: 'index_on_issues_gl_project_id_foobar', + length: [], + order: []) + + model.copy_indexes(:issues, :project_id, :gl_project_id) + end + end + + context 'using an index with a WHERE clause' do + it 'copies the index' do + index = double(:index, + columns: %w(project_id), + name: 'index_on_issues_project_id', + using: nil, + where: 'foo', + opclasses: {}, + unique: false, + lengths: [], + orders: []) + + allow(model).to receive(:indexes_for).with(:issues, 'project_id'). + and_return([index]) + + expect(model).to receive(:add_concurrent_index). + with(:issues, + %w(gl_project_id), + unique: false, + name: 'index_on_issues_gl_project_id', + length: [], + order: [], + where: 'foo') + + model.copy_indexes(:issues, :project_id, :gl_project_id) + end + end + + context 'using an index with a USING clause' do + it 'copies the index' do + index = double(:index, + columns: %w(project_id), + name: 'index_on_issues_project_id', + where: nil, + using: 'foo', + opclasses: {}, + unique: false, + lengths: [], + orders: []) + + allow(model).to receive(:indexes_for).with(:issues, 'project_id'). + and_return([index]) + + expect(model).to receive(:add_concurrent_index). + with(:issues, + %w(gl_project_id), + unique: false, + name: 'index_on_issues_gl_project_id', + length: [], + order: [], + using: 'foo') + + model.copy_indexes(:issues, :project_id, :gl_project_id) + end + end + + context 'using an index with custom operator classes' do + it 'copies the index' do + index = double(:index, + columns: %w(project_id), + name: 'index_on_issues_project_id', + using: nil, + where: nil, + opclasses: { 'project_id' => 'bar' }, + unique: false, + lengths: [], + orders: []) + + allow(model).to receive(:indexes_for).with(:issues, 'project_id'). + and_return([index]) + + expect(model).to receive(:add_concurrent_index). + with(:issues, + %w(gl_project_id), + unique: false, + name: 'index_on_issues_gl_project_id', + length: [], + order: [], + opclasses: { 'gl_project_id' => 'bar' }) + + model.copy_indexes(:issues, :project_id, :gl_project_id) + end + end + + describe 'using an index of which the name does not contain the source column' do + it 'raises RuntimeError' do + index = double(:index, + columns: %w(project_id), + name: 'index_foobar_index', + using: nil, + where: nil, + opclasses: {}, + unique: false, + lengths: [], + orders: []) + + allow(model).to receive(:indexes_for).with(:issues, 'project_id'). + and_return([index]) + + expect { model.copy_indexes(:issues, :project_id, :gl_project_id) }. + to raise_error(RuntimeError) + end + end + end + + describe '#copy_foreign_keys' do + it 'copies foreign keys from one column to another' do + fk = double(:fk, + from_table: 'issues', + to_table: 'projects', + on_delete: :cascade) + + allow(model).to receive(:foreign_keys_for).with(:issues, :project_id). + and_return([fk]) + + expect(model).to receive(:add_concurrent_foreign_key). + with('issues', 'projects', column: :gl_project_id, on_delete: :cascade) + + model.copy_foreign_keys(:issues, :project_id, :gl_project_id) + end + end + + describe '#column_for' do + it 'returns a column object for an existing column' do + column = model.column_for(:users, :id) + + expect(column.name).to eq('id') + end + + it 'returns nil when a column does not exist' do + expect(model.column_for(:users, :kittens)).to be_nil + end + end end diff --git a/spec/lib/gitlab/database/multi_threaded_migration_spec.rb b/spec/lib/gitlab/database/multi_threaded_migration_spec.rb new file mode 100644 index 00000000000..6c45f13bb5a --- /dev/null +++ b/spec/lib/gitlab/database/multi_threaded_migration_spec.rb @@ -0,0 +1,41 @@ +require 'spec_helper' + +describe Gitlab::Database::MultiThreadedMigration do + let(:migration) do + Class.new { include Gitlab::Database::MultiThreadedMigration }.new + end + + describe '#connection' do + after do + Thread.current[described_class::MULTI_THREAD_AR_CONNECTION] = nil + end + + it 'returns the thread-local connection if present' do + Thread.current[described_class::MULTI_THREAD_AR_CONNECTION] = 10 + + expect(migration.connection).to eq(10) + end + + it 'returns the global connection if no thread-local connection was set' do + expect(migration.connection).to eq(ActiveRecord::Base.connection) + end + end + + describe '#with_multiple_threads' do + it 'starts multiple threads and yields the supplied block in every thread' do + output = Queue.new + + migration.with_multiple_threads(2) do + output << migration.connection.execute('SELECT 1') + end + + expect(output.size).to eq(2) + end + + it 'joins the threads when the join parameter is set' do + expect_any_instance_of(Thread).to receive(:join).and_call_original + + migration.with_multiple_threads(1) { } + end + end +end diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index 4ce4e6e1034..9b1d66a1b1c 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -150,13 +150,13 @@ describe Gitlab::Database, lib: true do it 'returns correct value for PostgreSQL' do expect(described_class).to receive(:postgresql?).and_return(true) - expect(MigrationTest.new.true_value).to eq "'t'" + expect(described_class.true_value).to eq "'t'" end it 'returns correct value for MySQL' do expect(described_class).to receive(:postgresql?).and_return(false) - expect(MigrationTest.new.true_value).to eq 1 + expect(described_class.true_value).to eq 1 end end @@ -164,13 +164,13 @@ describe Gitlab::Database, lib: true do it 'returns correct value for PostgreSQL' do expect(described_class).to receive(:postgresql?).and_return(true) - expect(MigrationTest.new.false_value).to eq "'f'" + expect(described_class.false_value).to eq "'f'" end it 'returns correct value for MySQL' do expect(described_class).to receive(:postgresql?).and_return(false) - expect(MigrationTest.new.false_value).to eq 0 + expect(described_class.false_value).to eq 0 end end end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 690f604db5e..3d6d7292b42 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -24,20 +24,21 @@ describe Gitlab::Git::Repository, seed_helper: true do end end - context 'with gitaly enabled' do - before { stub_gitaly } - - 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 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 + # TODO: Uncomment when feature is reenabled + # context 'with gitaly enabled' do + # before { stub_gitaly } + # + # 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 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 end describe "#rugged" do @@ -112,20 +113,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 { stub_gitaly } - - it 'gets the branch names from GitalyClient' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names) - subject - end - - it 'wraps GRPC 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 + # TODO: Uncomment when feature is reenabled + # context 'with gitaly enabled' do + # before { stub_gitaly } + # + # it 'gets the branch names from GitalyClient' do + # expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names) + # subject + # end + # + # it 'wraps GRPC 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 end describe '#tag_names' do @@ -143,20 +145,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 { stub_gitaly } - - it 'gets the tag names from GitalyClient' do - expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names) - subject - 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 + # TODO: Uncomment when feature is reenabled + # context 'with gitaly enabled' do + # before { stub_gitaly } + # + # it 'gets the tag names from GitalyClient' do + # expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names) + # subject + # 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 end shared_examples 'archive check' do |extenstion| diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 980a1b70ef5..ce31c8ed94c 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -389,31 +389,32 @@ eos end end - describe '#raw_diffs' do - context 'Gitaly commit_raw_diffs feature enabled' do - before do - allow(Gitlab::GitalyClient).to receive(:feature_enabled?).with(:commit_raw_diffs).and_return(true) - end - - context 'when a truthy deltas_only is not passed to args' do - it 'fetches diffs from Gitaly server' do - expect(Gitlab::GitalyClient::Commit).to receive(:diff_from_parent). - with(commit) - - commit.raw_diffs - end - end - - context 'when a truthy deltas_only is passed to args' do - it 'fetches diffs using Rugged' do - opts = { deltas_only: true } - - expect(Gitlab::GitalyClient::Commit).not_to receive(:diff_from_parent) - expect(commit.raw).to receive(:diffs).with(opts) - - commit.raw_diffs(opts) - end - end - end - end + # describe '#raw_diffs' do + # TODO: Uncomment when feature is reenabled + # context 'Gitaly commit_raw_diffs feature enabled' do + # before do + # allow(Gitlab::GitalyClient).to receive(:feature_enabled?).with(:commit_raw_diffs).and_return(true) + # end + # + # context 'when a truthy deltas_only is not passed to args' do + # it 'fetches diffs from Gitaly server' do + # expect(Gitlab::GitalyClient::Commit).to receive(:diff_from_parent). + # with(commit) + # + # commit.raw_diffs + # end + # end + # + # context 'when a truthy deltas_only is passed to args' do + # it 'fetches diffs using Rugged' do + # opts = { deltas_only: true } + # + # expect(Gitlab::GitalyClient::Commit).not_to receive(:diff_from_parent) + # expect(commit.raw).to receive(:diffs).with(opts) + # + # commit.raw_diffs(opts) + # end + # end + # end + # end end diff --git a/spec/models/concerns/ignorable_column_spec.rb b/spec/models/concerns/ignorable_column_spec.rb new file mode 100644 index 00000000000..dba9fe43327 --- /dev/null +++ b/spec/models/concerns/ignorable_column_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe IgnorableColumn do + let :base_class do + Class.new do + def self.columns + # This method does not have access to "double" + [Struct.new(:name).new('id'), Struct.new(:name).new('title')] + end + end + end + + let :model do + Class.new(base_class) do + include IgnorableColumn + end + end + + describe '.columns' do + it 'returns the columns, excluding the ignored ones' do + model.ignore_column(:title) + + expect(model.columns.map(&:name)).to eq(%w(id)) + end + end + + describe '.ignored_columns' do + it 'returns a Set' do + expect(model.ignored_columns).to be_an_instance_of(Set) + end + + it 'returns the names of the ignored columns' do + model.ignore_column(:title) + + expect(model.ignored_columns).to eq(Set.new(%w(title))) + end + end +end diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index fb80b74b226..f32b6b99b3d 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -155,6 +155,23 @@ describe DiffNote, models: true do end end + describe '#latest_merge_request_diff' do + context 'when active' do + it 'returns the current merge request diff' do + expect(subject.latest_merge_request_diff).to eq(merge_request.merge_request_diff) + end + end + + context 'when outdated' do + let!(:old_merge_request_diff) { merge_request.merge_request_diff } + let!(:new_merge_request_diff) { merge_request.merge_request_diffs.create(diff_refs: commit.diff_refs) } + + it 'returns the latest merge request diff that this diff note applied to' do + expect(subject.latest_merge_request_diff).to eq(old_merge_request_diff) + end + end + end + describe "creation" do describe "updating of position" do context "when noteable is a commit" do diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index af7753caba6..070716e859a 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -110,17 +110,18 @@ describe Environment, models: true do end end - 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 + # 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 end describe '#environment_type' do diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index a9139f7d4ab..80ca19acdda 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -42,11 +42,27 @@ describe Label, models: true do end end + describe '#color' do + it 'strips color' do + label = described_class.new(color: ' #abcdef ') + label.valid? + + expect(label.color).to eq('#abcdef') + end + end + describe '#title' do it 'sanitizes title' do label = described_class.new(title: '<b>foo & bar?</b>') expect(label.title).to eq('foo & bar?') end + + it 'strips title' do + label = described_class.new(title: ' label ') + label.valid? + + expect(label.title).to eq('label') + end end describe 'priorization' do diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 3c4bf3f4ddb..557ea97b008 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -622,12 +622,22 @@ describe Note, models: true do describe 'expiring ETag cache' do let(:note) { build(:note_on_issue) } - it "expires cache for note's issue when note is saved" do + def expect_expiration(note) expect_any_instance_of(Gitlab::EtagCaching::Store) .to receive(:touch) .with("/#{note.project.namespace.to_param}/#{note.project.to_param}/noteable/issue/#{note.noteable.id}/notes") + end + + it "expires cache for note's issue when note is saved" do + expect_expiration(note) note.save! end + + it "expires cache for note's issue when note is destroyed" do + expect_expiration(note) + + note.destroy! + end end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 8bd436558cb..5e5c2b016b6 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1829,16 +1829,17 @@ describe Repository, models: true do end end - describe '#is_ancestor?' do - context 'Gitaly is_ancestor feature enabled' do - it 'asks Gitaly server if it\'s an ancestor' do - commit = repository.commit - allow(Gitlab::GitalyClient).to receive(:feature_enabled?).with(:is_ancestor).and_return(true) - expect(Gitlab::GitalyClient::Commit).to receive(:is_ancestor). - with(repository.raw_repository, commit.id, commit.id).and_return(true) - - expect(repository.is_ancestor?(commit.id, commit.id)).to be true - end - end - end + # TODO: Uncomment when feature is reenabled + # describe '#is_ancestor?' do + # context 'Gitaly is_ancestor feature enabled' do + # it 'asks Gitaly server if it\'s an ancestor' do + # commit = repository.commit + # allow(Gitlab::GitalyClient).to receive(:feature_enabled?).with(:is_ancestor).and_return(true) + # expect(Gitlab::GitalyClient::Commit).to receive(:is_ancestor). + # with(repository.raw_repository, commit.id, commit.id).and_return(true) + # + # expect(repository.is_ancestor?(commit.id, commit.id)).to be true + # end + # end + # end end diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 5c34ff04152..2077c14ff7a 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -22,7 +22,8 @@ describe GroupPolicy, models: true do :admin_group, :admin_namespace, :admin_group_member, - :change_visibility_level + :change_visibility_level, + :create_subgroup ] end diff --git a/spec/requests/api/project_hooks_spec.rb b/spec/requests/api/project_hooks_spec.rb index b1f8c249092..b1603233f9e 100644 --- a/spec/requests/api/project_hooks_spec.rb +++ b/spec/requests/api/project_hooks_spec.rb @@ -22,8 +22,8 @@ describe API::ProjectHooks, 'ProjectHooks', api: true do context "authorized user" do it "returns project hooks" do get api("/projects/#{project.id}/hooks", user) - expect(response).to have_http_status(200) + expect(response).to have_http_status(200) expect(json_response).to be_an Array expect(response).to include_pagination_headers expect(json_response.count).to eq(1) @@ -43,6 +43,7 @@ describe API::ProjectHooks, 'ProjectHooks', api: true do context "unauthorized user" do it "does not access project hooks" do get api("/projects/#{project.id}/hooks", user3) + expect(response).to have_http_status(403) end end @@ -52,6 +53,7 @@ describe API::ProjectHooks, 'ProjectHooks', api: true do context "authorized user" do it "returns a project hook" do get api("/projects/#{project.id}/hooks/#{hook.id}", user) + expect(response).to have_http_status(200) expect(json_response['url']).to eq(hook.url) expect(json_response['issues_events']).to eq(hook.issues_events) @@ -67,6 +69,7 @@ describe API::ProjectHooks, 'ProjectHooks', api: true do it "returns a 404 error if hook id is not available" do get api("/projects/#{project.id}/hooks/1234", user) + expect(response).to have_http_status(404) end end @@ -88,7 +91,8 @@ describe API::ProjectHooks, 'ProjectHooks', api: true do it "adds hook to project" do expect do post api("/projects/#{project.id}/hooks", user), - url: "http://example.com", issues_events: true, wiki_page_events: true + url: "http://example.com", issues_events: true, wiki_page_events: true, + job_events: true end.to change {project.hooks.count}.by(1) expect(response).to have_http_status(201) @@ -98,7 +102,7 @@ describe API::ProjectHooks, 'ProjectHooks', api: true do expect(json_response['merge_requests_events']).to eq(false) expect(json_response['tag_push_events']).to eq(false) expect(json_response['note_events']).to eq(false) - expect(json_response['job_events']).to eq(false) + expect(json_response['job_events']).to eq(true) expect(json_response['pipeline_events']).to eq(false) expect(json_response['wiki_page_events']).to eq(true) expect(json_response['enable_ssl_verification']).to eq(true) @@ -136,7 +140,8 @@ describe API::ProjectHooks, 'ProjectHooks', api: true do describe "PUT /projects/:id/hooks/:hook_id" do it "updates an existing project hook" do put api("/projects/#{project.id}/hooks/#{hook.id}", user), - url: 'http://example.org', push_events: false + url: 'http://example.org', push_events: false, job_events: true + expect(response).to have_http_status(200) expect(json_response['url']).to eq('http://example.org') expect(json_response['issues_events']).to eq(hook.issues_events) diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 2e291eb3cea..74bc4847247 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1076,6 +1076,13 @@ describe API::Projects, :api do before { project_member3 } before { project_member2 } + it 'returns 400 when nothing sent' do + project_param = {} + put api("/projects/#{project.id}", user), project_param + expect(response).to have_http_status(400) + expect(json_response['error']).to match('at least one parameter must be provided') + end + context 'when unauthenticated' do it 'returns authentication error' do project_param = { name: 'bar' } diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 4eb5b150af5..a3665795452 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -59,6 +59,10 @@ RSpec.configure do |config| TestEnv.init end + config.after(:suite) do + TestEnv.cleanup + end + if ENV['CI'] # Retry only on feature specs that use JS config.around :each, :js do |ex| diff --git a/spec/features/discussion_comments_spec.rb b/spec/support/features/discussion_comments_shared_example.rb index ae778118c5c..1a061ef069e 100644 --- a/spec/features/discussion_comments_spec.rb +++ b/spec/support/features/discussion_comments_shared_example.rb @@ -1,5 +1,3 @@ -require 'spec_helper' - shared_examples 'discussion comments' do |resource_name| let(:form_selector) { '.js-main-target-form' } let(:dropdown_selector) { "#{form_selector} .comment-type-dropdown" } @@ -9,11 +7,9 @@ shared_examples 'discussion comments' do |resource_name| let(:close_selector) { "#{form_selector} .btn-comment-and-close" } let(:comments_selector) { '.timeline > .note.timeline-entry' } - it 'should show a comment type toggle' do + it 'clicking "Comment" will post a comment' do expect(page).to have_selector toggle_selector - end - it 'clicking "Comment" will post a comment' do find("#{form_selector} .note-textarea").send_keys('a') find(submit_selector).click @@ -49,44 +45,29 @@ shared_examples 'discussion comments' do |resource_name| find(toggle_selector).click end - it 'opens a comment type dropdown with "Comment" and "Start discussion"' do + it 'has a "Comment" item (selected by default) and "Start discussion" item' do expect(page).to have_selector menu_selector - end - - it 'has a "Comment" item' do - menu = find(menu_selector) - - expect(menu).to have_content 'Comment' - expect(menu).to have_content "Add a general comment to this #{resource_name}." - end - it 'has a "Start discussion" item' do - menu = find(menu_selector) - - expect(menu).to have_content 'Start discussion' - expect(menu).to have_content "Discuss a specific suggestion or question#{' that needs to be resolved' if resource_name == 'merge request'}." - end - - it 'has the "Comment" item selected by default' do find("#{menu_selector} li", match: :first) items = all("#{menu_selector} li") expect(items.first).to have_content 'Comment' + expect(items.first).to have_content "Add a general comment to this #{resource_name}." expect(items.first).to have_selector '.fa-check' expect(items.first['class']).to match 'droplab-item-selected' expect(items.last).to have_content 'Start discussion' + expect(items.last).to have_content "Discuss a specific suggestion or question#{' that needs to be resolved' if resource_name == 'merge request'}." expect(items.last).not_to have_selector '.fa-check' expect(items.last['class']).not_to match 'droplab-item-selected' end - it 'closes the menu when clicking the toggle' do + it 'closes the menu when clicking the toggle or body' do find(toggle_selector).click expect(page).not_to have_selector menu_selector - end - it 'closes the menu when clicking the body' do + find(toggle_selector).click find('body').click expect(page).not_to have_selector menu_selector @@ -104,12 +85,10 @@ shared_examples 'discussion comments' do |resource_name| all("#{menu_selector} li").last.click end - it 'updates the note_type input to "DiscussionNote"' do - expect(find("#{form_selector} #note_type", visible: false).value).to eq('DiscussionNote') - end - - it 'updates the submit button text' do + it 'updates the submit button text, note_type input and closes the dropdown' do expect(find(dropdown_selector)).to have_content 'Start discussion' + expect(find("#{form_selector} #note_type", visible: false).value).to eq('DiscussionNote') + expect(page).not_to have_selector menu_selector end if resource_name =~ /(issue|merge request)/ @@ -124,10 +103,6 @@ shared_examples 'discussion comments' do |resource_name| end end - it 'closes the dropdown' do - expect(page).not_to have_selector menu_selector - end - it 'clicking "Start discussion" will post a discussion' do find(submit_selector).click @@ -176,12 +151,10 @@ shared_examples 'discussion comments' do |resource_name| find("#{menu_selector} li", match: :first).click end - it 'clears the note_type input"' do - expect(find("#{form_selector} #note_type", visible: false).value).to eq('') - end - - it 'updates the submit button text' do + it 'updates the submit button text, clears the note_type input and closes the dropdown' do expect(find(dropdown_selector)).to have_content 'Comment' + expect(find("#{form_selector} #note_type", visible: false).value).to eq('') + expect(page).not_to have_selector menu_selector end if resource_name =~ /(issue|merge request)/ @@ -196,10 +169,6 @@ shared_examples 'discussion comments' do |resource_name| end end - it 'closes the dropdown' do - expect(page).not_to have_selector menu_selector - end - it 'should have "Comment" selected when opening the menu' do find(toggle_selector).click @@ -242,54 +211,3 @@ shared_examples 'discussion comments' do |resource_name| end end end - -describe 'Discussion Comments', :feature, :js do - include RepoHelpers - - let(:user) { create(:user) } - let(:project) { create(:project) } - - before do - project.team << [user, :developer] - - login_as(user) - end - - describe 'on a merge request' do - let(:merge_request) { create(:merge_request, source_project: project) } - - before do - visit namespace_project_merge_request_path(project.namespace, project, merge_request) - end - - it_behaves_like 'discussion comments', 'merge request' - end - - describe 'on an issue' do - let(:issue) { create(:issue, project: project) } - - before do - visit namespace_project_issue_path(project.namespace, project, issue) - end - - it_behaves_like 'discussion comments', 'issue' - end - - describe 'on an snippet' do - let(:snippet) { create(:project_snippet, :private, project: project, author: user) } - - before do - visit namespace_project_snippet_path(project.namespace, project, snippet) - end - - it_behaves_like 'discussion comments', 'snippet' - end - - describe 'on a commit' do - before do - visit namespace_project_commit_path(project.namespace, project, sample_commit.id) - end - - it_behaves_like 'discussion comments', 'commit' - end -end diff --git a/spec/support/gitaly.rb b/spec/support/gitaly.rb new file mode 100644 index 00000000000..7aca902fc61 --- /dev/null +++ b/spec/support/gitaly.rb @@ -0,0 +1,7 @@ +if Gitlab::GitalyClient.enabled? + RSpec.configure do |config| + config.before(:each) do + allow(Gitlab::GitalyClient).to receive(:feature_enabled?).and_return(true) + end + end +end diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index af1defb5a2e..eb0f1efe55b 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -65,6 +65,8 @@ module TestEnv # Setup GitLab shell for test instance setup_gitlab_shell + setup_gitaly if Gitlab::GitalyClient.enabled? + # Create repository for FactoryGirl.create(:project) setup_factory_repo @@ -72,6 +74,10 @@ module TestEnv setup_forked_repo end + def cleanup + stop_gitaly + end + def disable_mailer allow_any_instance_of(NotificationService).to receive(:mailer). and_return(double.as_null_object) @@ -93,7 +99,7 @@ module TestEnv tmp_test_path = Rails.root.join('tmp', 'tests', '**') Dir[tmp_test_path].each do |entry| - unless File.basename(entry) =~ /\Agitlab-(shell|test|test_bare|test-fork|test-fork_bare)\z/ + unless File.basename(entry) =~ /\A(gitaly|gitlab-(shell|test|test_bare|test-fork|test-fork_bare))\z/ FileUtils.rm_rf(entry) end end @@ -111,6 +117,28 @@ module TestEnv end end + def setup_gitaly + socket_path = Gitlab::GitalyClient.get_address('default').sub(/\Aunix:/, '') + gitaly_dir = File.dirname(socket_path) + + unless File.directory?(gitaly_dir) || system('rake', "gitlab:gitaly:install[#{gitaly_dir}]") + raise "Can't clone gitaly" + end + + start_gitaly(gitaly_dir, socket_path) + end + + def start_gitaly(gitaly_dir, socket_path) + gitaly_exec = File.join(gitaly_dir, 'gitaly') + @gitaly_pid = spawn({ "GITALY_SOCKET_PATH" => socket_path }, gitaly_exec, [:out, :err] => '/dev/null') + end + + def stop_gitaly + return unless @gitaly_pid + + Process.kill('KILL', @gitaly_pid) + end + def setup_factory_repo setup_repo(factory_repo_path, factory_repo_path_bare, factory_repo_name, BRANCH_SHA) |