diff options
Diffstat (limited to 'spec')
55 files changed, 946 insertions, 574 deletions
diff --git a/spec/controllers/metrics_controller_spec.rb b/spec/controllers/metrics_controller_spec.rb index 4aed2a25baa..9e8a37171ec 100644 --- a/spec/controllers/metrics_controller_spec.rb +++ b/spec/controllers/metrics_controller_spec.rb @@ -67,7 +67,8 @@ describe MetricsController do it 'returns proper response' do get :index - expect(response.status).to eq(404) + expect(response.status).to eq(200) + expect(response.body).to eq("# Metrics are disabled, see: http://test.host/help/administration/monitoring/prometheus/gitlab_metrics#gitlab-prometheus-metrics\n") end end end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 8016176110e..4dbbaecdd6d 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -861,7 +861,7 @@ describe Projects::IssuesController do end it 'delegates the update of the todos count cache to TodoService' do - expect_any_instance_of(TodoService).to receive(:destroy_issue).with(issue, owner).once + expect_any_instance_of(TodoService).to receive(:destroy_issuable).with(issue, owner).once delete :destroy, namespace_id: project.namespace, project_id: project, id: issue.iid end diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 14021b8ca50..bfdad85c082 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -456,7 +456,7 @@ describe Projects::MergeRequestsController do end it 'delegates the update of the todos count cache to TodoService' do - expect_any_instance_of(TodoService).to receive(:destroy_merge_request).with(merge_request, owner).once + expect_any_instance_of(TodoService).to receive(:destroy_issuable).with(merge_request, owner).once delete :destroy, namespace_id: project.namespace, project_id: project, id: merge_request.iid end diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 7c4a22c94c2..cc6cef63b47 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -83,10 +83,10 @@ FactoryGirl.define do target_project = merge_request.target_project source_project = merge_request.source_project - # Fake `write_ref` if we don't have repository + # Fake `fetch_ref!` if we don't have repository # We have too many existing tests replying on this behaviour unless [target_project, source_project].all?(&:repository_exists?) - allow(merge_request).to receive(:write_ref) + allow(merge_request).to receive(:fetch_ref!) end end diff --git a/spec/features/issues/create_branch_merge_request_spec.rb b/spec/features/issues/create_branch_merge_request_spec.rb index 546dc7e8a49..edea95c6699 100644 --- a/spec/features/issues/create_branch_merge_request_spec.rb +++ b/spec/features/issues/create_branch_merge_request_spec.rb @@ -64,6 +64,19 @@ feature 'Create Branch/Merge Request Dropdown on issue page', :feature, :js do end end + context 'when merge requests are disabled' do + before do + project.project_feature.update(merge_requests_access_level: 0) + + visit project_issue_path(project, issue) + end + + it 'shows only create branch button' do + expect(page).not_to have_button('Create a merge request') + expect(page).to have_button('Create a branch') + end + end + context 'when issue is confidential' do it 'disables the create branch button' do issue = create(:issue, :confidential, project: project) diff --git a/spec/helpers/events_helper_spec.rb b/spec/helpers/events_helper_spec.rb index d5536fcb22b..8a80b88da5d 100644 --- a/spec/helpers/events_helper_spec.rb +++ b/spec/helpers/events_helper_spec.rb @@ -1,96 +1,6 @@ require 'spec_helper' describe EventsHelper do - describe '#event_note' do - let(:user) { build(:user) } - - before do - allow(helper).to receive(:current_user).and_return(user) - end - - it 'displays one line of plain text without alteration' do - input = 'A short, plain note' - expect(helper.event_note(input)).to match(input) - expect(helper.event_note(input)).not_to match(/\.\.\.\z/) - end - - it 'displays inline code' do - input = 'A note with `inline code`' - expected = 'A note with <code>inline code</code>' - - expect(helper.event_note(input)).to match(expected) - end - - it 'truncates a note with multiple paragraphs' do - input = "Paragraph 1\n\nParagraph 2" - expected = 'Paragraph 1...' - - expect(helper.event_note(input)).to match(expected) - end - - it 'displays the first line of a code block' do - input = "```\nCode block\nwith two lines\n```" - expected = %r{<pre.+><code><span class="line">Code block\.\.\.</span>\n</code></pre>} - - expect(helper.event_note(input)).to match(expected) - end - - it 'truncates a single long line of text' do - text = 'The quick brown fox jumped over the lazy dog twice' # 50 chars - input = text * 4 - expected = (text * 2).sub(/.{3}/, '...') - - expect(helper.event_note(input)).to match(expected) - end - - it 'preserves a link href when link text is truncated' do - text = 'The quick brown fox jumped over the lazy dog' # 44 chars - input = "#{text}#{text}#{text} " # 133 chars - link_url = 'http://example.com/foo/bar/baz' # 30 chars - input << link_url - expected_link_text = 'http://example...</a>' - - expect(helper.event_note(input)).to match(link_url) - expect(helper.event_note(input)).to match(expected_link_text) - end - - it 'preserves code color scheme' do - input = "```ruby\ndef test\n 'hello world'\nend\n```" - expected = "\n<pre class=\"code highlight js-syntax-highlight ruby\">" \ - "<code><span class=\"line\"><span class=\"k\">def</span> <span class=\"nf\">test</span>...</span>\n" \ - "</code></pre>" - expect(helper.event_note(input)).to eq(expected) - end - - it 'preserves data-src for lazy images' do - input = "" - image_url = "data-src=\"/uploads/test.png\"" - expect(helper.event_note(input)).to match(image_url) - end - - context 'labels formatting' do - let(:input) { 'this should be ~label_1' } - - def format_event_note(project) - create(:label, title: 'label_1', project: project) - - helper.event_note(input, { project: project }) - end - - it 'preserves style attribute for a label that can be accessed by current_user' do - project = create(:project, :public) - - expect(format_event_note(project)).to match(/span class=.*style=.*/) - end - - it 'does not style a label that can not be accessed by current_user' do - project = create(:project, :private) - - expect(format_event_note(project)).to eq("<p>#{input}</p>") - end - end - end - describe '#event_commit_title' do let(:message) { "foo & bar " + "A" * 70 + "\n" + "B" * 80 } subject { helper.event_commit_title(message) } diff --git a/spec/helpers/markup_helper_spec.rb b/spec/helpers/markup_helper_spec.rb index 03d706062b7..62ea6d48542 100644 --- a/spec/helpers/markup_helper_spec.rb +++ b/spec/helpers/markup_helper_spec.rb @@ -67,7 +67,7 @@ describe MarkupHelper do describe 'without redacted attribute' do it 'renders the markdown value' do - expect(Banzai).to receive(:render_field).with(commit, attribute).and_call_original + expect(Banzai).to receive(:render_field).with(commit, attribute, {}).and_call_original helper.markdown_field(commit, attribute) end @@ -252,38 +252,141 @@ describe MarkupHelper do end describe '#first_line_in_markdown' do - it 'truncates Markdown properly' do - text = "@#{user.username}, can you look at this?\nHello world\n" - actual = first_line_in_markdown(text, 100, project: project) + shared_examples_for 'common markdown examples' do + let(:project_base) { build(:project, :repository) } - doc = Nokogiri::HTML.parse(actual) + it 'displays inline code' do + object = create_object('Text with `inline code`') + expected = 'Text with <code>inline code</code>' - # Make sure we didn't create invalid markup - expect(doc.errors).to be_empty + expect(first_line_in_markdown(object, attribute, 100, project: project)).to match(expected) + end - # Leading user link - expect(doc.css('a').length).to eq(1) - expect(doc.css('a')[0].attr('href')).to eq user_path(user) - expect(doc.css('a')[0].text).to eq "@#{user.username}" + it 'truncates the text with multiple paragraphs' do + object = create_object("Paragraph 1\n\nParagraph 2") + expected = 'Paragraph 1...' - expect(doc.content).to eq "@#{user.username}, can you look at this?..." - end + expect(first_line_in_markdown(object, attribute, 100, project: project)).to match(expected) + end - it 'truncates Markdown with emoji properly' do - text = "foo :wink:\nbar :grinning:" - actual = first_line_in_markdown(text, 100, project: project) + it 'displays the first line of a code block' do + object = create_object("```\nCode block\nwith two lines\n```") + expected = %r{<pre.+><code><span class="line">Code block\.\.\.</span>\n</code></pre>} - doc = Nokogiri::HTML.parse(actual) + expect(first_line_in_markdown(object, attribute, 100, project: project)).to match(expected) + end - # Make sure we didn't create invalid markup - # But also account for the 2 errors caused by the unknown `gl-emoji` elements - expect(doc.errors.length).to eq(2) + it 'truncates a single long line of text' do + text = 'The quick brown fox jumped over the lazy dog twice' # 50 chars + object = create_object(text * 4) + expected = (text * 2).sub(/.{3}/, '...') + + expect(first_line_in_markdown(object, attribute, 150, project: project)).to match(expected) + end + + it 'preserves a link href when link text is truncated' do + text = 'The quick brown fox jumped over the lazy dog' # 44 chars + input = "#{text}#{text}#{text} " # 133 chars + link_url = 'http://example.com/foo/bar/baz' # 30 chars + input << link_url + object = create_object(input) + expected_link_text = 'http://example...</a>' + + expect(first_line_in_markdown(object, attribute, 150, project: project)).to match(link_url) + expect(first_line_in_markdown(object, attribute, 150, project: project)).to match(expected_link_text) + end + + it 'preserves code color scheme' do + object = create_object("```ruby\ndef test\n 'hello world'\nend\n```") + expected = "\n<pre class=\"code highlight js-syntax-highlight ruby\">" \ + "<code><span class=\"line\"><span class=\"k\">def</span> <span class=\"nf\">test</span>...</span>\n" \ + "</code></pre>" + + expect(first_line_in_markdown(object, attribute, 150, project: project)).to eq(expected) + end + + it 'preserves data-src for lazy images' do + object = create_object("") + image_url = "data-src=\".*/uploads/test.png\"" + + expect(first_line_in_markdown(object, attribute, 150, project: project)).to match(image_url) + end + + context 'labels formatting' do + let(:label_title) { 'this should be ~label_1' } + + def create_and_format_label(project) + create(:label, title: 'label_1', project: project) + object = create_object(label_title, project: project) - expect(doc.css('gl-emoji').length).to eq(2) - expect(doc.css('gl-emoji')[0].attr('data-name')).to eq 'wink' - expect(doc.css('gl-emoji')[1].attr('data-name')).to eq 'grinning' + first_line_in_markdown(object, attribute, 150, project: project) + end - expect(doc.content).to eq "foo 😉\nbar 😀" + it 'preserves style attribute for a label that can be accessed by current_user' do + project = create(:project, :public) + + expect(create_and_format_label(project)).to match(/span class=.*style=.*/) + end + + it 'does not style a label that can not be accessed by current_user' do + project = create(:project, :private) + + expect(create_and_format_label(project)).to eq("<p>#{label_title}</p>") + end + end + + it 'truncates Markdown properly' do + object = create_object("@#{user.username}, can you look at this?\nHello world\n") + actual = first_line_in_markdown(object, attribute, 100, project: project) + + doc = Nokogiri::HTML.parse(actual) + + # Make sure we didn't create invalid markup + expect(doc.errors).to be_empty + + # Leading user link + expect(doc.css('a').length).to eq(1) + expect(doc.css('a')[0].attr('href')).to eq user_path(user) + expect(doc.css('a')[0].text).to eq "@#{user.username}" + + expect(doc.content).to eq "@#{user.username}, can you look at this?..." + end + + it 'truncates Markdown with emoji properly' do + object = create_object("foo :wink:\nbar :grinning:") + actual = first_line_in_markdown(object, attribute, 100, project: project) + + doc = Nokogiri::HTML.parse(actual) + + # Make sure we didn't create invalid markup + # But also account for the 2 errors caused by the unknown `gl-emoji` elements + expect(doc.errors.length).to eq(2) + + expect(doc.css('gl-emoji').length).to eq(2) + expect(doc.css('gl-emoji')[0].attr('data-name')).to eq 'wink' + expect(doc.css('gl-emoji')[1].attr('data-name')).to eq 'grinning' + + expect(doc.content).to eq "foo 😉\nbar 😀" + end + end + + context 'when the asked attribute can be redacted' do + include_examples 'common markdown examples' do + let(:attribute) { :note } + def create_object(title, project: project_base) + build(:note, note: title, project: project) + end + end + end + + context 'when the asked attribute can not be redacted' do + include_examples 'common markdown examples' do + let(:attribute) { :body } + def create_object(title, project: project_base) + issue = build(:issue, title: title) + build(:todo, :done, project: project_base, author: user, target: issue) + end + end end end diff --git a/spec/initializers/8_metrics_spec.rb b/spec/initializers/8_metrics_spec.rb index 4e6052a9f80..80c77057065 100644 --- a/spec/initializers/8_metrics_spec.rb +++ b/spec/initializers/8_metrics_spec.rb @@ -3,7 +3,6 @@ require 'spec_helper' describe 'instrument_classes' do let(:config) { double(:config) } - let(:unicorn_sampler) { double(:unicorn_sampler) } let(:influx_sampler) { double(:influx_sampler) } before do @@ -11,9 +10,7 @@ describe 'instrument_classes' do allow(config).to receive(:instrument_methods) allow(config).to receive(:instrument_instance_method) allow(config).to receive(:instrument_instance_methods) - allow(Gitlab::Metrics::UnicornSampler).to receive(:initialize_instance).and_return(unicorn_sampler) - allow(Gitlab::Metrics::InfluxSampler).to receive(:initialize_instance).and_return(influx_sampler) - allow(unicorn_sampler).to receive(:start) + allow(Gitlab::Metrics::Samplers::InfluxSampler).to receive(:initialize_instance).and_return(influx_sampler) allow(influx_sampler).to receive(:start) allow(Gitlab::Application).to receive(:configure) end diff --git a/spec/javascripts/fixtures/search_autocomplete.html.haml b/spec/javascripts/fixtures/search_autocomplete.html.haml index 7785120da5b..0421ed2182f 100644 --- a/spec/javascripts/fixtures/search_autocomplete.html.haml +++ b/spec/javascripts/fixtures/search_autocomplete.html.haml @@ -8,3 +8,4 @@ %input#search.search-input.dropdown-menu-toggle .dropdown-menu.dropdown-select .dropdown-content + %input{ type: "hidden", class: "js-search-project-options" } diff --git a/spec/javascripts/lib/utils/datefix_spec.js b/spec/javascripts/lib/utils/datefix_spec.js index 0b9fde2be67..e58ac4300ba 100644 --- a/spec/javascripts/lib/utils/datefix_spec.js +++ b/spec/javascripts/lib/utils/datefix_spec.js @@ -1,4 +1,4 @@ -import { pad, parsePikadayDate, pikadayToString } from '~/lib/utils/datefix'; +import { pad, pikadayToString } from '~/lib/utils/datefix'; describe('datefix', () => { describe('pad', () => { @@ -16,9 +16,7 @@ describe('datefix', () => { }); describe('parsePikadayDate', () => { - it('should return a UTC date', () => { - expect(parsePikadayDate('2020-01-29')).toEqual(new Date('2020-01-29')); - }); + // removed because of https://gitlab.com/gitlab-org/gitlab-ce/issues/39834 }); describe('pikadayToString', () => { diff --git a/spec/javascripts/search_autocomplete_spec.js b/spec/javascripts/search_autocomplete_spec.js index 5e55a5d2686..a2394857b82 100644 --- a/spec/javascripts/search_autocomplete_spec.js +++ b/spec/javascripts/search_autocomplete_spec.js @@ -57,6 +57,10 @@ import '~/lib/utils/common_utils'; } }; + const disableProjectIssues = function() { + document.querySelector('.js-search-project-options').setAttribute('data-issues-disabled', true); + }; + // Mock `gl` object in window for dashboard specific page. App code will need it. mockDashboardOptions = function() { window.gl || (window.gl = {}); @@ -91,18 +95,20 @@ import '~/lib/utils/common_utils'; assertLinks = function(list, issuesPath, mrsPath) { var a1, a2, a3, a4, issuesAssignedToMeLink, issuesIHaveCreatedLink, mrsAssignedToMeLink, mrsIHaveCreatedLink; - issuesAssignedToMeLink = issuesPath + "/?assignee_username=" + userName; - issuesIHaveCreatedLink = issuesPath + "/?author_username=" + userName; + if (issuesPath) { + issuesAssignedToMeLink = issuesPath + "/?assignee_username=" + userName; + issuesIHaveCreatedLink = issuesPath + "/?author_username=" + userName; + a1 = "a[href='" + issuesAssignedToMeLink + "']"; + a2 = "a[href='" + issuesIHaveCreatedLink + "']"; + expect(list.find(a1).length).toBe(1); + expect(list.find(a1).text()).toBe('Issues assigned to me'); + expect(list.find(a2).length).toBe(1); + expect(list.find(a2).text()).toBe("Issues I've created"); + } mrsAssignedToMeLink = mrsPath + "/?assignee_username=" + userName; mrsIHaveCreatedLink = mrsPath + "/?author_username=" + userName; - a1 = "a[href='" + issuesAssignedToMeLink + "']"; - a2 = "a[href='" + issuesIHaveCreatedLink + "']"; a3 = "a[href='" + mrsAssignedToMeLink + "']"; a4 = "a[href='" + mrsIHaveCreatedLink + "']"; - expect(list.find(a1).length).toBe(1); - expect(list.find(a1).text()).toBe('Issues assigned to me'); - expect(list.find(a2).length).toBe(1); - expect(list.find(a2).text()).toBe("Issues I've created"); expect(list.find(a3).length).toBe(1); expect(list.find(a3).text()).toBe('Merge requests assigned to me'); expect(list.find(a4).length).toBe(1); @@ -153,6 +159,14 @@ import '~/lib/utils/common_utils'; list = widget.wrap.find('.dropdown-menu').find('ul'); return assertLinks(list, projectIssuesPath, projectMRsPath); }); + it('should show only Project mergeRequest dropdown menu items when project issues are disabled', function() { + addBodyAttributes('project'); + disableProjectIssues(); + mockProjectOptions(); + widget.searchInput.triggerHandler('focus'); + const list = widget.wrap.find('.dropdown-menu').find('ul'); + assertLinks(list, null, projectMRsPath); + }); it('should not show category related menu if there is text in the input', function() { var link, list; addBodyAttributes('project'); diff --git a/spec/javascripts/test_bundle.js b/spec/javascripts/test_bundle.js index d4e134583c7..fd7aa332d17 100644 --- a/spec/javascripts/test_bundle.js +++ b/spec/javascripts/test_bundle.js @@ -11,6 +11,12 @@ const isHeadlessChrome = /\bHeadlessChrome\//.test(navigator.userAgent); Vue.config.devtools = !isHeadlessChrome; Vue.config.productionTip = false; +let hasVueWarnings = false; +Vue.config.warnHandler = (msg, vm, trace) => { + hasVueWarnings = true; + fail(`${msg}${trace}`); +}; + Vue.use(VueResource); // enable test fixtures @@ -34,11 +40,6 @@ window.addEventListener('unhandledrejection', (event) => { console.error(event.reason.stack || event.reason); }); -const checkUnhandledPromiseRejections = (done) => { - expect(hasUnhandledPromiseRejections).toBe(false); - done(); -}; - // HACK: Chrome 59 disconnects if there are too many synchronous tests in a row // because it appears to lock up the thread that communicates to Karma's socket // This async beforeEach gets called on every spec and releases the JS thread long @@ -47,17 +48,6 @@ const checkUnhandledPromiseRejections = (done) => { // to run our unit tests. beforeEach(done => done()); -beforeAll(() => { - const origError = console.error; - spyOn(console, 'error').and.callFake((message) => { - if (/^\[Vue warn\]/.test(message)) { - fail(message); - } else { - origError(message); - } - }); -}); - const builtinVueHttpInterceptors = Vue.http.interceptors.slice(); beforeEach(() => { @@ -80,8 +70,22 @@ testsContext.keys().forEach(function (path) { } }); -it('has no unhandled Promise rejections', (done) => { - setTimeout(checkUnhandledPromiseRejections(done), 1000); +describe('test errors', () => { + beforeAll((done) => { + if (hasUnhandledPromiseRejections || hasVueWarnings) { + setTimeout(done, 1000); + } else { + done(); + } + }); + + it('has no unhandled Promise rejections', () => { + expect(hasUnhandledPromiseRejections).toBe(false); + }); + + it('has no Vue warnings', () => { + expect(hasVueWarnings).toBe(false); + }); }); // if we're generating coverage reports, make sure to include all files so diff --git a/spec/javascripts/vue_shared/components/markdown/field_spec.js b/spec/javascripts/vue_shared/components/markdown/field_spec.js index 65c49b9f30b..24209be83fe 100644 --- a/spec/javascripts/vue_shared/components/markdown/field_spec.js +++ b/spec/javascripts/vue_shared/components/markdown/field_spec.js @@ -1,6 +1,12 @@ import Vue from 'vue'; import fieldComponent from '~/vue_shared/components/markdown/field.vue'; +function assertMarkdownTabs(isWrite, writeLink, previewLink, vm) { + expect(writeLink.parentNode.classList.contains('active')).toEqual(isWrite); + expect(previewLink.parentNode.classList.contains('active')).toEqual(!isWrite); + expect(vm.$el.querySelector('.md-preview').style.display).toEqual(isWrite ? 'none' : ''); +} + describe('Markdown field component', () => { let vm; @@ -39,6 +45,7 @@ describe('Markdown field component', () => { describe('markdown preview', () => { let previewLink; + let writeLink; beforeEach(() => { spyOn(Vue.http, 'post').and.callFake(() => new Promise((resolve) => { @@ -53,7 +60,8 @@ describe('Markdown field component', () => { }); })); - previewLink = vm.$el.querySelector('.nav-links li:nth-child(2) a'); + previewLink = vm.$el.querySelector('.nav-links .js-preview-link'); + writeLink = vm.$el.querySelector('.nav-links .js-write-link'); }); it('sets preview link as active', (done) => { @@ -105,6 +113,23 @@ describe('Markdown field component', () => { done(); }, 0); }); + + it('clicking already active write or preview link does nothing', (done) => { + writeLink.click(); + Vue.nextTick() + .then(() => assertMarkdownTabs(true, writeLink, previewLink, vm)) + .then(() => writeLink.click()) + .then(() => Vue.nextTick()) + .then(() => assertMarkdownTabs(true, writeLink, previewLink, vm)) + .then(() => previewLink.click()) + .then(() => Vue.nextTick()) + .then(() => assertMarkdownTabs(false, writeLink, previewLink, vm)) + .then(() => previewLink.click()) + .then(() => Vue.nextTick()) + .then(() => assertMarkdownTabs(false, writeLink, previewLink, vm)) + .then(done) + .catch(done.fail); + }); }); describe('markdown buttons', () => { diff --git a/spec/javascripts/vue_shared/components/markdown/header_spec.js b/spec/javascripts/vue_shared/components/markdown/header_spec.js index 7110ff36937..edebd822295 100644 --- a/spec/javascripts/vue_shared/components/markdown/header_spec.js +++ b/spec/javascripts/vue_shared/components/markdown/header_spec.js @@ -43,11 +43,13 @@ describe('Markdown field header component', () => { it('emits toggle markdown event when clicking preview', () => { spyOn(vm, '$emit'); - vm.$el.querySelector('li:nth-child(2) a').click(); + vm.$el.querySelector('.js-preview-link').click(); - expect( - vm.$emit, - ).toHaveBeenCalledWith('toggle-markdown'); + expect(vm.$emit).toHaveBeenCalledWith('preview-markdown'); + + vm.$el.querySelector('.js-write-link').click(); + + expect(vm.$emit).toHaveBeenCalledWith('write-markdown'); }); it('blurs preview link after click', (done) => { diff --git a/spec/lib/banzai/commit_renderer_spec.rb b/spec/lib/banzai/commit_renderer_spec.rb index 049d025a5b9..84adaebdcbe 100644 --- a/spec/lib/banzai/commit_renderer_spec.rb +++ b/spec/lib/banzai/commit_renderer_spec.rb @@ -10,7 +10,7 @@ describe Banzai::CommitRenderer do described_class::ATTRIBUTES.each do |attr| expect_any_instance_of(Banzai::ObjectRenderer).to receive(:render).with([project.commit], attr).once.and_call_original - expect(Banzai::Renderer).to receive(:cacheless_render_field).with(project.commit, attr) + expect(Banzai::Renderer).to receive(:cacheless_render_field).with(project.commit, attr, {}) end described_class.render([project.commit], project, user) diff --git a/spec/lib/banzai/filter/absolute_link_filter_spec.rb b/spec/lib/banzai/filter/absolute_link_filter_spec.rb new file mode 100644 index 00000000000..a3ad056efcd --- /dev/null +++ b/spec/lib/banzai/filter/absolute_link_filter_spec.rb @@ -0,0 +1,58 @@ +require 'spec_helper' + +describe Banzai::Filter::AbsoluteLinkFilter do + def filter(doc, context = {}) + described_class.call(doc, context) + end + + context 'with html links' do + context 'if only_path is false' do + let(:only_path_context) do + { only_path: false } + end + let(:fake_url) { 'http://www.example.com' } + + before do + allow(Gitlab.config.gitlab).to receive(:url).and_return(fake_url) + end + + context 'has the .gfm class' do + it 'converts a relative url into absolute' do + doc = filter(link('/foo', 'gfm'), only_path_context) + expect(doc.at_css('a')['href']).to eq "#{fake_url}/foo" + end + + it 'does not change the url if it already absolute' do + doc = filter(link("#{fake_url}/foo", 'gfm'), only_path_context) + expect(doc.at_css('a')['href']).to eq "#{fake_url}/foo" + end + + context 'if relative_url_root is set' do + it 'joins the url without without doubling the path' do + allow(Gitlab.config.gitlab).to receive(:url).and_return("#{fake_url}/gitlab/") + doc = filter(link("/gitlab/foo", 'gfm'), only_path_context) + expect(doc.at_css('a')['href']).to eq "#{fake_url}/gitlab/foo" + end + end + end + + context 'has not the .gfm class' do + it 'does not convert a relative url into absolute' do + doc = filter(link('/foo'), only_path_context) + expect(doc.at_css('a')['href']).to eq '/foo' + end + end + end + + context 'if only_path is not false' do + it 'does not convert a relative url into absolute' do + expect(filter(link('/foo', 'gfm')).at_css('a')['href']).to eq '/foo' + expect(filter(link('/foo')).at_css('a')['href']).to eq '/foo' + end + end + end + + def link(path, css_class = '') + %(<a class="#{css_class}" href="#{path}">example</a>) + end +end diff --git a/spec/lib/banzai/note_renderer_spec.rb b/spec/lib/banzai/note_renderer_spec.rb deleted file mode 100644 index 32764bee5eb..00000000000 --- a/spec/lib/banzai/note_renderer_spec.rb +++ /dev/null @@ -1,24 +0,0 @@ -require 'spec_helper' - -describe Banzai::NoteRenderer do - describe '.render' do - it 'renders a Note' do - note = double(:note) - project = double(:project) - wiki = double(:wiki) - user = double(:user) - - expect(Banzai::ObjectRenderer).to receive(:new) - .with(project, user, - requested_path: 'foo', - project_wiki: wiki, - ref: 'bar') - .and_call_original - - expect_any_instance_of(Banzai::ObjectRenderer) - .to receive(:render).with([note], :note) - - described_class.render([note], project, user, 'foo', wiki, 'bar') - end - end -end diff --git a/spec/lib/banzai/object_renderer_spec.rb b/spec/lib/banzai/object_renderer_spec.rb index b172a1b718c..074d521a5c6 100644 --- a/spec/lib/banzai/object_renderer_spec.rb +++ b/spec/lib/banzai/object_renderer_spec.rb @@ -22,7 +22,7 @@ describe Banzai::ObjectRenderer do end it 'retrieves field content using Banzai::Renderer.render_field' do - expect(Banzai::Renderer).to receive(:render_field).with(object, :note).and_call_original + expect(Banzai::Renderer).to receive(:render_field).with(object, :note, {}).and_call_original renderer.render([object], :note) end @@ -68,7 +68,7 @@ describe Banzai::ObjectRenderer do end it 'retrieves field content using Banzai::Renderer.cacheless_render_field' do - expect(Banzai::Renderer).to receive(:cacheless_render_field).with(commit, :title).and_call_original + expect(Banzai::Renderer).to receive(:cacheless_render_field).with(commit, :title, {}).and_call_original renderer.render([commit], :title) end diff --git a/spec/lib/banzai/renderer_spec.rb b/spec/lib/banzai/renderer_spec.rb index 81a04a2d46d..650cecfc778 100644 --- a/spec/lib/banzai/renderer_spec.rb +++ b/spec/lib/banzai/renderer_spec.rb @@ -18,7 +18,7 @@ describe Banzai::Renderer do let(:commit) { create(:project, :repository).commit } it 'returns cacheless render field' do - expect(renderer).to receive(:cacheless_render_field).with(commit, :title) + expect(renderer).to receive(:cacheless_render_field).with(commit, :title, {}) renderer.render_field(commit, :title) end diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index 6c25b7349e1..74a24a4424b 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -11,13 +11,13 @@ describe Gitlab::Checks::ChangeAccess do let(:changes) { { oldrev: oldrev, newrev: newrev, ref: ref } } let(:protocol) { 'ssh' } - subject do + subject(:change_access) do described_class.new( changes, project: project, user_access: user_access, protocol: protocol - ).exec + ) end before do @@ -26,7 +26,7 @@ describe Gitlab::Checks::ChangeAccess do context 'without failed checks' do it "doesn't raise an error" do - expect { subject }.not_to raise_error + expect { subject.exec }.not_to raise_error end end @@ -34,7 +34,7 @@ describe Gitlab::Checks::ChangeAccess do it 'raises an error' do expect(user_access).to receive(:can_do_action?).with(:push_code).and_return(false) - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.') + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to this project.') end end @@ -45,7 +45,7 @@ describe Gitlab::Checks::ChangeAccess do allow(user_access).to receive(:can_do_action?).with(:push_code).and_return(true) expect(user_access).to receive(:can_do_action?).with(:admin_project).and_return(false) - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.') + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to change existing tags on this project.') end context 'with protected tag' do @@ -61,7 +61,7 @@ describe Gitlab::Checks::ChangeAccess do let(:newrev) { '0000000000000000000000000000000000000000' } it 'is prevented' do - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be deleted/) + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be deleted/) end end @@ -70,7 +70,7 @@ describe Gitlab::Checks::ChangeAccess do let(:newrev) { '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51' } it 'is prevented' do - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be updated/) + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /cannot be updated/) end end end @@ -81,14 +81,14 @@ describe Gitlab::Checks::ChangeAccess do let(:ref) { 'refs/tags/v9.1.0' } it 'prevents creation below access level' do - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /allowed to create this tag as it is protected/) + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /allowed to create this tag as it is protected/) end context 'when user has access' do let!(:protected_tag) { create(:protected_tag, :developers_can_create, project: project, name: 'v*') } it 'allows tag creation' do - expect { subject }.not_to raise_error + expect { subject.exec }.not_to raise_error end end end @@ -101,7 +101,7 @@ describe Gitlab::Checks::ChangeAccess do let(:ref) { 'refs/heads/master' } it 'raises an error' do - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'The default branch of a project cannot be deleted.') + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'The default branch of a project cannot be deleted.') end end @@ -114,7 +114,7 @@ describe Gitlab::Checks::ChangeAccess do it 'raises an error if the user is not allowed to do forced pushes to protected branches' do expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true) - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to force push code to a protected branch on this project.') + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to force push code to a protected branch on this project.') end it 'raises an error if the user is not allowed to merge to protected branches' do @@ -122,13 +122,13 @@ describe Gitlab::Checks::ChangeAccess do expect(user_access).to receive(:can_merge_to_branch?).and_return(false) expect(user_access).to receive(:can_push_to_branch?).and_return(false) - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to merge code into protected branches on this project.') + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to merge code into protected branches on this project.') end it 'raises an error if the user is not allowed to push to protected branches' do expect(user_access).to receive(:can_push_to_branch?).and_return(false) - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to protected branches on this project.') + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to push code to protected branches on this project.') end context 'branch deletion' do @@ -137,7 +137,7 @@ describe Gitlab::Checks::ChangeAccess do context 'if the user is not allowed to delete protected branches' do it 'raises an error' do - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.') + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.') end end @@ -150,18 +150,63 @@ describe Gitlab::Checks::ChangeAccess do let(:protocol) { 'web' } it 'allows branch deletion' do - expect { subject }.not_to raise_error + expect { subject.exec }.not_to raise_error end end context 'over SSH or HTTP' do it 'raises an error' do - expect { subject }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only delete protected branches using the web interface.') + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, 'You can only delete protected branches using the web interface.') end end end end end end + + context 'LFS integrity check' do + let(:blob_object) { project.repository.blob_at_branch('lfs', 'files/lfs/lfs_object.iso') } + + before do + allow_any_instance_of(Gitlab::Git::RevList).to receive(:new_objects) do |&lazy_block| + lazy_block.call([blob_object.id]) + end + end + + context 'with LFS not enabled' do + it 'skips integrity check' do + expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects) + + subject.exec + end + end + + context 'with LFS enabled' do + before do + allow(project).to receive(:lfs_enabled?).and_return(true) + end + + context 'deletion' do + let(:changes) { { oldrev: oldrev, ref: ref } } + + it 'skips integrity check' do + expect_any_instance_of(Gitlab::Git::RevList).not_to receive(:new_objects) + + subject.exec + end + end + + it 'fails if any LFS blobs are missing' do + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, /LFS objects are missing/) + end + + it 'succeeds if LFS objects have already been uploaded' do + lfs_object = create(:lfs_object, oid: blob_object.lfs_oid) + create(:lfs_objects_project, project: project, lfs_object: lfs_object) + + expect { subject.exec }.not_to raise_error + end + end + end end end diff --git a/spec/lib/gitlab/git/lfs_changes_spec.rb b/spec/lib/gitlab/git/lfs_changes_spec.rb index c2d2c6e1bc8..c9007d7d456 100644 --- a/spec/lib/gitlab/git/lfs_changes_spec.rb +++ b/spec/lib/gitlab/git/lfs_changes_spec.rb @@ -9,7 +9,7 @@ describe Gitlab::Git::LfsChanges do describe 'new_pointers' do before do - allow_any_instance_of(Gitlab::Git::RevList).to receive(:new_objects).and_return([blob_object_id]) + allow_any_instance_of(Gitlab::Git::RevList).to receive(:new_objects).and_yield([blob_object_id]) end it 'uses rev-list to find new objects' do @@ -38,7 +38,7 @@ describe Gitlab::Git::LfsChanges do it 'uses rev-list to find all objects' do rev_list = double allow(Gitlab::Git::RevList).to receive(:new).and_return(rev_list) - allow(rev_list).to receive(:all_objects).and_return([blob_object_id]) + allow(rev_list).to receive(:all_objects).and_yield([blob_object_id]) expect(Gitlab::Git::Blob).to receive(:batch_lfs_pointers).with(project.repository, [blob_object_id]) diff --git a/spec/lib/gitlab/git/popen_spec.rb b/spec/lib/gitlab/git/popen_spec.rb index 2b65bc1cf15..b033ede9062 100644 --- a/spec/lib/gitlab/git/popen_spec.rb +++ b/spec/lib/gitlab/git/popen_spec.rb @@ -53,6 +53,23 @@ describe 'Gitlab::Git::Popen' do it { expect(status).to be_zero } it { expect(output).to eq('hello') } end + + context 'with lazy block' do + it 'yields a lazy io' do + expect_lazy_io = lambda do |io| + expect(io).to be_a Enumerator::Lazy + expect(io.inspect).to include('#<IO:fd') + end + + klass.new.popen(%w[ls], path, lazy_block: expect_lazy_io) + end + + it "doesn't wait for process exit" do + Timeout.timeout(2) do + klass.new.popen(%w[yes], path, lazy_block: ->(io) {}) + end + end + end end context 'popen_with_timeout' do diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index 1d4d0c300eb..96e162ac087 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -1521,7 +1521,7 @@ describe Gitlab::Git::Repository, seed_helper: true do end end - describe '#fetch_source_branch' do + describe '#fetch_source_branch!' do let(:local_ref) { 'refs/merge-requests/1/head' } context 'when the branch exists' do @@ -1530,11 +1530,11 @@ describe Gitlab::Git::Repository, seed_helper: true do it 'writes the ref' do expect(repository).to receive(:write_ref).with(local_ref, /\h{40}/) - repository.fetch_source_branch(repository, source_branch, local_ref) + repository.fetch_source_branch!(repository, source_branch, local_ref) end it 'returns true' do - expect(repository.fetch_source_branch(repository, source_branch, local_ref)).to eq(true) + expect(repository.fetch_source_branch!(repository, source_branch, local_ref)).to eq(true) end end @@ -1544,11 +1544,11 @@ describe Gitlab::Git::Repository, seed_helper: true do it 'does not write the ref' do expect(repository).not_to receive(:write_ref) - repository.fetch_source_branch(repository, source_branch, local_ref) + repository.fetch_source_branch!(repository, source_branch, local_ref) end it 'returns false' do - expect(repository.fetch_source_branch(repository, source_branch, local_ref)).to eq(false) + expect(repository.fetch_source_branch!(repository, source_branch, local_ref)).to eq(false) end end end diff --git a/spec/lib/gitlab/git/rev_list_spec.rb b/spec/lib/gitlab/git/rev_list_spec.rb index 643a4b2d03e..eaf74951b0e 100644 --- a/spec/lib/gitlab/git/rev_list_spec.rb +++ b/spec/lib/gitlab/git/rev_list_spec.rb @@ -3,26 +3,44 @@ require 'spec_helper' describe Gitlab::Git::RevList do let(:project) { create(:project, :repository) } let(:rev_list) { described_class.new(newrev: 'newrev', path_to_repo: project.repository.path_to_repo) } + let(:env_hash) do + { + 'GIT_OBJECT_DIRECTORY' => 'foo', + 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar' + } + end before do - allow(Gitlab::Git::Env).to receive(:all).and_return({ - GIT_OBJECT_DIRECTORY: 'foo', - GIT_ALTERNATE_OBJECT_DIRECTORIES: 'bar' - }) + allow(Gitlab::Git::Env).to receive(:all).and_return(env_hash.symbolize_keys) end - def stub_popen_rev_list(*additional_args, output:) - expect(rev_list).to receive(:popen).with([ + def args_for_popen(args_list) + [ Gitlab.config.git.bin_path, "--git-dir=#{project.repository.path_to_repo}", 'rev-list', - *additional_args - ], - nil, - { - 'GIT_OBJECT_DIRECTORY' => 'foo', - 'GIT_ALTERNATE_OBJECT_DIRECTORIES' => 'bar' - }).and_return([output, 0]) + *args_list + ] + end + + def stub_popen_rev_list(*additional_args, output:) + args = args_for_popen(additional_args) + + expect(rev_list).to receive(:popen).with(args, nil, env_hash) + .and_return([output, 0]) + end + + def stub_lazy_popen_rev_list(*additional_args, output:) + params = [ + args_for_popen(additional_args), + nil, + env_hash, + hash_including(lazy_block: anything) + ] + + expect(rev_list).to receive(:popen).with(*params) do |*_, lazy_block:| + lazy_block.call(output.split("\n").lazy) + end end context "#new_refs" do @@ -46,10 +64,22 @@ describe Gitlab::Git::RevList do expect(rev_list.new_objects(require_path: true)).to eq(%w[sha2]) end - it 'can return a lazy enumerator' do - stub_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2") + it 'can yield a lazy enumerator' do + stub_lazy_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2") + + rev_list.new_objects do |object_ids| + expect(object_ids).to be_a Enumerator::Lazy + end + end + + it 'returns the result of the block when given' do + stub_lazy_popen_rev_list('newrev', '--not', '--all', '--objects', output: "sha1\nsha2") + + objects = rev_list.new_objects do |object_ids| + object_ids.first + end - expect(rev_list.new_objects(lazy: true)).to be_a Enumerator::Lazy + expect(objects).to eq 'sha1' end it 'can accept list of references to exclude' do @@ -69,7 +99,7 @@ describe Gitlab::Git::RevList do it 'fetches list of all pushed objects using rev-list' do stub_popen_rev_list('--all', '--objects', output: "sha1\nsha2") - expect(rev_list.all_objects.force).to eq(%w[sha1 sha2]) + expect(rev_list.all_objects).to eq(%w[sha1 sha2]) end end diff --git a/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb b/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb index 92bf87bbad4..78475403f9e 100644 --- a/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb +++ b/spec/lib/gitlab/hook_data/merge_request_builder_spec.rb @@ -26,7 +26,6 @@ describe Gitlab::HookData::MergeRequestBuilder do merge_user_id merge_when_pipeline_succeeds milestone_id - ref_fetched source_branch source_project_id state diff --git a/spec/lib/gitlab/import_export/fork_spec.rb b/spec/lib/gitlab/import_export/fork_spec.rb index dd0ce0dae41..cfb15ee7e8b 100644 --- a/spec/lib/gitlab/import_export/fork_spec.rb +++ b/spec/lib/gitlab/import_export/fork_spec.rb @@ -46,7 +46,7 @@ describe 'forked project import' do end it 'can access the MR' do - project.merge_requests.first.ensure_ref_fetched + project.merge_requests.first.fetch_ref! expect(project.repository.ref_exists?('refs/merge-requests/1/head')).to be_truthy end diff --git a/spec/lib/gitlab/metrics/background_transaction_spec.rb b/spec/lib/gitlab/metrics/background_transaction_spec.rb new file mode 100644 index 00000000000..96052b8dc2f --- /dev/null +++ b/spec/lib/gitlab/metrics/background_transaction_spec.rb @@ -0,0 +1,13 @@ +require 'spec_helper' + +describe Gitlab::Metrics::BackgroundTransaction do + let(:test_worker_class) { double(:class, name: 'TestWorker') } + + subject { described_class.new(test_worker_class) } + + describe '#action' do + it 'returns transaction action name' do + expect(subject.action).to eq('TestWorker#perform') + end + end +end diff --git a/spec/lib/gitlab/metrics/instrumentation_spec.rb b/spec/lib/gitlab/metrics/instrumentation_spec.rb index 4b19ee19103..977bc250049 100644 --- a/spec/lib/gitlab/metrics/instrumentation_spec.rb +++ b/spec/lib/gitlab/metrics/instrumentation_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Gitlab::Metrics::Instrumentation do - let(:transaction) { Gitlab::Metrics::Transaction.new } + let(:env) { {} } + let(:transaction) { Gitlab::Metrics::WebTransaction.new(env) } before do @dummy = Class.new do diff --git a/spec/lib/gitlab/metrics/method_call_spec.rb b/spec/lib/gitlab/metrics/method_call_spec.rb index a247f03b2da..f1e9e414e0d 100644 --- a/spec/lib/gitlab/metrics/method_call_spec.rb +++ b/spec/lib/gitlab/metrics/method_call_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Gitlab::Metrics::MethodCall do - let(:method_call) { described_class.new('Foo#bar', 'foo') } + let(:transaction) { double(:transaction, labels: {}) } + let(:method_call) { described_class.new('Foo#bar', :Foo, '#bar', transaction) } describe '#measure' do it 'measures the performance of the supplied block' do @@ -11,6 +12,18 @@ describe Gitlab::Metrics::MethodCall do expect(method_call.cpu_time).to be_a_kind_of(Numeric) expect(method_call.call_count).to eq(1) end + + it 'observes the performance of the supplied block' do + expect(described_class.call_real_duration_histogram) + .to receive(:observe) + .with({ module: :Foo, method: '#bar' }, be_a_kind_of(Numeric)) + + expect(described_class.call_cpu_duration_histogram) + .to receive(:observe) + .with({ module: :Foo, method: '#bar' }, be_a_kind_of(Numeric)) + + method_call.measure { 'foo' } + end end describe '#to_metric' do @@ -19,7 +32,7 @@ describe Gitlab::Metrics::MethodCall do metric = method_call.to_metric expect(metric).to be_an_instance_of(Gitlab::Metrics::Metric) - expect(metric.series).to eq('foo') + expect(metric.series).to eq('rails_method_calls') expect(metric.values[:duration]).to be_a_kind_of(Numeric) expect(metric.values[:cpu_duration]).to be_a_kind_of(Numeric) diff --git a/spec/lib/gitlab/metrics/rack_middleware_spec.rb b/spec/lib/gitlab/metrics/rack_middleware_spec.rb index ec415f2bd85..b84387204ee 100644 --- a/spec/lib/gitlab/metrics/rack_middleware_spec.rb +++ b/spec/lib/gitlab/metrics/rack_middleware_spec.rb @@ -18,34 +18,6 @@ describe Gitlab::Metrics::RackMiddleware do expect(middleware.call(env)).to eq('yay') end - it 'tags a transaction with the name and action of a controller' do - klass = double(:klass, name: 'TestController', content_type: 'text/html') - controller = double(:controller, class: klass, action_name: 'show') - - env['action_controller.instance'] = controller - - allow(app).to receive(:call).with(env) - - expect(middleware).to receive(:tag_controller) - .with(an_instance_of(Gitlab::Metrics::Transaction), env) - - middleware.call(env) - end - - it 'tags a transaction with the method and path of the route in the grape endpoint' do - route = double(:route, request_method: "GET", path: "/:version/projects/:id/archive(.:format)") - endpoint = double(:endpoint, route: route) - - env['api.endpoint'] = endpoint - - allow(app).to receive(:call).with(env) - - expect(middleware).to receive(:tag_endpoint) - .with(an_instance_of(Gitlab::Metrics::Transaction), env) - - middleware.call(env) - end - it 'tracks any raised exceptions' do expect(app).to receive(:call).with(env).and_raise(RuntimeError) @@ -60,7 +32,7 @@ describe Gitlab::Metrics::RackMiddleware do let(:transaction) { middleware.transaction_from_env(env) } it 'returns a Transaction' do - expect(transaction).to be_an_instance_of(Gitlab::Metrics::Transaction) + expect(transaction).to be_an_instance_of(Gitlab::Metrics::WebTransaction) end it 'stores the request method and URI in the transaction as values' do @@ -84,58 +56,4 @@ describe Gitlab::Metrics::RackMiddleware do end end end - - describe '#tag_controller' do - let(:transaction) { middleware.transaction_from_env(env) } - let(:content_type) { 'text/html' } - - before do - klass = double(:klass, name: 'TestController') - controller = double(:controller, class: klass, action_name: 'show', content_type: content_type) - - env['action_controller.instance'] = controller - end - - it 'tags a transaction with the name and action of a controller' do - middleware.tag_controller(transaction, env) - - expect(transaction.action).to eq('TestController#show') - end - - context 'when the response content type is not :html' do - let(:content_type) { 'application/json' } - - it 'appends the mime type to the transaction action' do - middleware.tag_controller(transaction, env) - - expect(transaction.action).to eq('TestController#show.json') - end - end - end - - describe '#tag_endpoint' do - let(:transaction) { middleware.transaction_from_env(env) } - - it 'tags a transaction with the method and path of the route in the grape endpount' do - route = double(:route, request_method: "GET", path: "/:version/projects/:id/archive(.:format)") - endpoint = double(:endpoint, route: route) - - env['api.endpoint'] = endpoint - - middleware.tag_endpoint(transaction, env) - - expect(transaction.action).to eq('Grape#GET /projects/:id/archive') - end - - it 'does not tag a transaction if route infos are missing' do - endpoint = double(:endpoint) - allow(endpoint).to receive(:route).and_raise - - env['api.endpoint'] = endpoint - - middleware.tag_endpoint(transaction, env) - - expect(transaction.action).to be_nil - end - end end diff --git a/spec/lib/gitlab/metrics/influx_sampler_spec.rb b/spec/lib/gitlab/metrics/samplers/influx_sampler_spec.rb index 999a9536d82..667e4747897 100644 --- a/spec/lib/gitlab/metrics/influx_sampler_spec.rb +++ b/spec/lib/gitlab/metrics/samplers/influx_sampler_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Metrics::InfluxSampler do +describe Gitlab::Metrics::Samplers::InfluxSampler do let(:sampler) { described_class.new(5) } after do diff --git a/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb b/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb new file mode 100644 index 00000000000..53699327da1 --- /dev/null +++ b/spec/lib/gitlab/metrics/samplers/ruby_sampler_spec.rb @@ -0,0 +1,90 @@ +require 'spec_helper' + +describe Gitlab::Metrics::Samplers::RubySampler do + let(:sampler) { described_class.new(5) } + + after do + Allocations.stop if Gitlab::Metrics.mri? + end + + describe '#sample' do + it 'samples various statistics' do + expect(Gitlab::Metrics::System).to receive(:memory_usage) + expect(Gitlab::Metrics::System).to receive(:file_descriptor_count) + expect(sampler).to receive(:sample_objects) + expect(sampler).to receive(:sample_gc) + + sampler.sample + end + + it 'adds a metric containing the memory usage' do + expect(Gitlab::Metrics::System).to receive(:memory_usage) + .and_return(9000) + + expect(sampler.metrics[:memory_usage]).to receive(:set) + .with({}, 9000) + .and_call_original + + sampler.sample + end + + it 'adds a metric containing the amount of open file descriptors' do + expect(Gitlab::Metrics::System).to receive(:file_descriptor_count) + .and_return(4) + + expect(sampler.metrics[:file_descriptors]).to receive(:set) + .with({}, 4) + .and_call_original + + sampler.sample + end + + it 'clears any GC profiles' do + expect(GC::Profiler).to receive(:clear) + + sampler.sample + end + end + + describe '#sample_gc' do + it 'adds a metric containing garbage collection time statistics' do + expect(GC::Profiler).to receive(:total_time).and_return(0.24) + + expect(sampler.metrics[:total_time]).to receive(:set) + .with({}, 240) + .and_call_original + + sampler.sample + end + + it 'adds a metric containing garbage collection statistics' do + GC.stat.keys.each do |key| + expect(sampler.metrics[key]).to receive(:set).with({}, anything).and_call_original + end + + sampler.sample + end + end + + if Gitlab::Metrics.mri? + describe '#sample_objects' do + it 'adds a metric containing the amount of allocated objects' do + expect(sampler.metrics[:objects_total]).to receive(:set) + .with(include(class: anything), be > 0) + .at_least(:once) + .and_call_original + + sampler.sample + end + + it 'ignores classes without a name' do + expect(Allocations).to receive(:to_hash).and_return({ Class.new => 4 }) + + expect(sampler.metrics[:objects_total]).not_to receive(:set) + .with(include(class: 'object_counts'), anything) + + sampler.sample + end + end + end +end diff --git a/spec/lib/gitlab/metrics/unicorn_sampler_spec.rb b/spec/lib/gitlab/metrics/samplers/unicorn_sampler_spec.rb index dc0d1f2e940..771b633a2b9 100644 --- a/spec/lib/gitlab/metrics/unicorn_sampler_spec.rb +++ b/spec/lib/gitlab/metrics/samplers/unicorn_sampler_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::Metrics::UnicornSampler do +describe Gitlab::Metrics::Samplers::UnicornSampler do subject { described_class.new(1.second) } describe '#sample' do diff --git a/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb b/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb index 0803ce42fac..6d69b5305d2 100644 --- a/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb +++ b/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb @@ -5,8 +5,8 @@ describe Gitlab::Metrics::SidekiqMiddleware do let(:message) { { 'args' => ['test'], 'enqueued_at' => Time.new(2016, 6, 23, 6, 59).to_f } } def run(worker, message) - expect(Gitlab::Metrics::Transaction).to receive(:new) - .with('TestWorker#perform') + expect(Gitlab::Metrics::BackgroundTransaction).to receive(:new) + .with(worker.class) .and_call_original expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:set) @@ -18,21 +18,18 @@ describe Gitlab::Metrics::SidekiqMiddleware do end describe '#call' do - it 'tracks the transaction' do - worker = double(:worker, class: double(:class, name: 'TestWorker')) + let(:test_worker_class) { double(:class, name: 'TestWorker') } + let(:worker) { double(:worker, class: test_worker_class) } + it 'tracks the transaction' do run(worker, message) end it 'tracks the transaction (for messages without `enqueued_at`)' do - worker = double(:worker, class: double(:class, name: 'TestWorker')) - run(worker, {}) end it 'tracks any raised exceptions' do - worker = double(:worker, class: double(:class, name: 'TestWorker')) - expect_any_instance_of(Gitlab::Metrics::Transaction) .to receive(:run).and_raise(RuntimeError) @@ -45,18 +42,5 @@ describe Gitlab::Metrics::SidekiqMiddleware do expect { middleware.call(worker, message, :test) } .to raise_error(RuntimeError) end - - it 'tags the metrics accordingly' do - tags = { one: 1, two: 2 } - worker = double(:worker, class: double(:class, name: 'TestWorker')) - allow(worker).to receive(:metrics_tags).and_return(tags) - - tags.each do |tag, value| - expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:add_tag) - .with(tag, value) - end - - run(worker, message) - end end end diff --git a/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb b/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb index e7b595405a8..eca75a4fac1 100644 --- a/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb +++ b/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Gitlab::Metrics::Subscribers::ActionView do - let(:transaction) { Gitlab::Metrics::Transaction.new } + let(:env) { {} } + let(:transaction) { Gitlab::Metrics::WebTransaction.new(env) } let(:subscriber) { described_class.new } @@ -29,5 +30,13 @@ describe Gitlab::Metrics::Subscribers::ActionView do subscriber.render_template(event) end + + it 'observes view rendering time' do + expect(subscriber.send(:metric_view_rendering_duration_seconds)) + .to receive(:observe) + .with({ view: 'app/views/x.html.haml' }, 2.1) + + subscriber.render_template(event) + end end end diff --git a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb index ce6587e993f..9b3698fb4a8 100644 --- a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb +++ b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb @@ -1,11 +1,12 @@ require 'spec_helper' describe Gitlab::Metrics::Subscribers::ActiveRecord do - let(:transaction) { Gitlab::Metrics::Transaction.new } + let(:env) { {} } + let(:transaction) { Gitlab::Metrics::WebTransaction.new(env) } let(:subscriber) { described_class.new } let(:event) do - double(:event, duration: 0.2, + double(:event, duration: 2, payload: { sql: 'SELECT * FROM users WHERE id = 10' }) end @@ -20,16 +21,24 @@ describe Gitlab::Metrics::Subscribers::ActiveRecord do end describe 'with a current transaction' do + it 'observes sql_duration metric' do + expect(subscriber).to receive(:current_transaction) + .at_least(:once) + .and_return(transaction) + expect(subscriber.send(:metric_sql_duration_seconds)).to receive(:observe).with({}, 0.002) + subscriber.sql(event) + end + it 'increments the :sql_duration value' do expect(subscriber).to receive(:current_transaction) .at_least(:once) .and_return(transaction) expect(transaction).to receive(:increment) - .with(:sql_duration, 0.2) + .with(:sql_duration, 2, false) expect(transaction).to receive(:increment) - .with(:sql_count, 1) + .with(:sql_count, 1, false) subscriber.sql(event) end diff --git a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb index f04dc8dcc02..58e28592cf9 100644 --- a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb +++ b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb @@ -1,15 +1,16 @@ require 'spec_helper' describe Gitlab::Metrics::Subscribers::RailsCache do - let(:transaction) { Gitlab::Metrics::Transaction.new } + let(:env) { {} } + let(:transaction) { Gitlab::Metrics::WebTransaction.new(env) } let(:subscriber) { described_class.new } let(:event) { double(:event, duration: 15.2) } describe '#cache_read' do it 'increments the cache_read duration' do - expect(subscriber).to receive(:increment) - .with(:cache_read, event.duration) + expect(subscriber).to receive(:observe) + .with(:read, event.duration) subscriber.cache_read(event) end @@ -17,7 +18,7 @@ describe Gitlab::Metrics::Subscribers::RailsCache do context 'with a transaction' do before do allow(subscriber).to receive(:current_transaction) - .and_return(transaction) + .and_return(transaction) end context 'with hit event' do @@ -25,9 +26,9 @@ describe Gitlab::Metrics::Subscribers::RailsCache do it 'increments the cache_read_hit count' do expect(transaction).to receive(:increment) - .with(:cache_read_hit_count, 1) + .with(:cache_read_hit_count, 1, false) expect(transaction).to receive(:increment) - .with(any_args).at_least(1) # Other calls + .with(any_args).at_least(1) # Other calls subscriber.cache_read(event) end @@ -37,7 +38,7 @@ describe Gitlab::Metrics::Subscribers::RailsCache do it 'does not increment cache read miss' do expect(transaction).not_to receive(:increment) - .with(:cache_read_hit_count, 1) + .with(:cache_read_hit_count, 1) subscriber.cache_read(event) end @@ -49,9 +50,15 @@ describe Gitlab::Metrics::Subscribers::RailsCache do it 'increments the cache_read_miss count' do expect(transaction).to receive(:increment) - .with(:cache_read_miss_count, 1) + .with(:cache_read_miss_count, 1, false) expect(transaction).to receive(:increment) - .with(any_args).at_least(1) # Other calls + .with(any_args).at_least(1) # Other calls + + subscriber.cache_read(event) + end + + it 'increments the cache_read_miss total' do + expect(subscriber.send(:metric_cache_misses_total)).to receive(:increment).with({}) subscriber.cache_read(event) end @@ -61,7 +68,13 @@ describe Gitlab::Metrics::Subscribers::RailsCache do it 'does not increment cache read miss' do expect(transaction).not_to receive(:increment) - .with(:cache_read_miss_count, 1) + .with(:cache_read_miss_count, 1) + + subscriber.cache_read(event) + end + + it 'does not increment cache_read_miss total' do + expect(subscriber.send(:metric_cache_misses_total)).not_to receive(:increment).with({}) subscriber.cache_read(event) end @@ -71,27 +84,27 @@ describe Gitlab::Metrics::Subscribers::RailsCache do end describe '#cache_write' do - it 'increments the cache_write duration' do - expect(subscriber).to receive(:increment) - .with(:cache_write, event.duration) + it 'observes write duration' do + expect(subscriber).to receive(:observe) + .with(:write, event.duration) subscriber.cache_write(event) end end describe '#cache_delete' do - it 'increments the cache_delete duration' do - expect(subscriber).to receive(:increment) - .with(:cache_delete, event.duration) + it 'observes delete duration' do + expect(subscriber).to receive(:observe) + .with(:delete, event.duration) subscriber.cache_delete(event) end end describe '#cache_exist?' do - it 'increments the cache_exists duration' do - expect(subscriber).to receive(:increment) - .with(:cache_exists, event.duration) + it 'observes the exists duration' do + expect(subscriber).to receive(:observe) + .with(:exists, event.duration) subscriber.cache_exist?(event) end @@ -109,12 +122,12 @@ describe Gitlab::Metrics::Subscribers::RailsCache do context 'with a transaction' do before do allow(subscriber).to receive(:current_transaction) - .and_return(transaction) + .and_return(transaction) end it 'increments the cache_read_hit count' do expect(transaction).to receive(:increment) - .with(:cache_read_hit_count, 1) + .with(:cache_read_hit_count, 1) subscriber.cache_fetch_hit(event) end @@ -133,47 +146,61 @@ describe Gitlab::Metrics::Subscribers::RailsCache do context 'with a transaction' do before do allow(subscriber).to receive(:current_transaction) - .and_return(transaction) + .and_return(transaction) end it 'increments the cache_fetch_miss count' do expect(transaction).to receive(:increment) - .with(:cache_read_miss_count, 1) + .with(:cache_read_miss_count, 1) + + subscriber.cache_generate(event) + end + + it 'increments the cache_read_miss total' do + expect(subscriber.send(:metric_cache_misses_total)).to receive(:increment).with({}) subscriber.cache_generate(event) end end end - describe '#increment' do + describe '#observe' do context 'without a transaction' do it 'returns' do expect(transaction).not_to receive(:increment) - subscriber.increment(:foo, 15.2) + subscriber.observe(:foo, 15.2) end end context 'with a transaction' do before do allow(subscriber).to receive(:current_transaction) - .and_return(transaction) + .and_return(transaction) end it 'increments the total and specific cache duration' do expect(transaction).to receive(:increment) - .with(:cache_duration, event.duration) + .with(:cache_duration, event.duration, false) expect(transaction).to receive(:increment) - .with(:cache_count, 1) + .with(:cache_count, 1, false) expect(transaction).to receive(:increment) - .with(:cache_delete_duration, event.duration) + .with(:cache_delete_duration, event.duration, false) expect(transaction).to receive(:increment) - .with(:cache_delete_count, 1) + .with(:cache_delete_count, 1, false) + + subscriber.observe(:delete, event.duration) + end + + it 'observes cache metric' do + expect(subscriber.send(:metric_cache_operation_duration_seconds)) + .to receive(:observe) + .with(transaction.labels.merge(operation: :delete), event.duration / 1000.0) - subscriber.increment(:cache_delete, event.duration) + subscriber.observe(:delete, event.duration) end end end diff --git a/spec/lib/gitlab/metrics/transaction_spec.rb b/spec/lib/gitlab/metrics/web_transaction_spec.rb index 3779af81512..1d162f53a13 100644 --- a/spec/lib/gitlab/metrics/transaction_spec.rb +++ b/spec/lib/gitlab/metrics/web_transaction_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' -describe Gitlab::Metrics::Transaction do - let(:transaction) { described_class.new } +describe Gitlab::Metrics::WebTransaction do + let(:env) { {} } + let(:transaction) { described_class.new(env) } describe '#duration' do it 'returns the duration of a transaction in seconds' do @@ -48,7 +49,7 @@ describe Gitlab::Metrics::Transaction do describe '#method_call_for' do it 'returns a MethodCall' do - method = transaction.method_call_for('Foo#bar') + method = transaction.method_call_for('Foo#bar', :Foo, '#bar') expect(method).to be_an_instance_of(Gitlab::Metrics::MethodCall) end @@ -85,14 +86,6 @@ describe Gitlab::Metrics::Transaction do end end - describe '#add_tag' do - it 'adds a tag' do - transaction.add_tag(:foo, 'bar') - - expect(transaction.tags).to eq({ foo: 'bar' }) - end - end - describe '#finish' do it 'tracks the transaction details and submits them to Sidekiq' do expect(transaction).to receive(:track_self) @@ -127,7 +120,7 @@ describe Gitlab::Metrics::Transaction do end it 'adds the action as a tag for every metric' do - transaction.action = 'Foo#bar' + allow(transaction).to receive(:labels).and_return(controller: 'Foo', action: 'bar') transaction.track_self hash = { @@ -144,7 +137,8 @@ describe Gitlab::Metrics::Transaction do end it 'does not add an action tag for events' do - transaction.action = 'Foo#bar' + allow(transaction).to receive(:labels).and_return(controller: 'Foo', action: 'bar') + transaction.add_event(:meow) hash = { @@ -161,6 +155,61 @@ describe Gitlab::Metrics::Transaction do end end + describe '#labels' do + context 'when request goes to Grape endpoint' do + before do + route = double(:route, request_method: 'GET', path: '/:version/projects/:id/archive(.:format)') + endpoint = double(:endpoint, route: route) + + env['api.endpoint'] = endpoint + end + it 'provides labels with the method and path of the route in the grape endpoint' do + expect(transaction.labels).to eq({ controller: 'Grape', action: 'GET /projects/:id/archive' }) + expect(transaction.action).to eq('Grape#GET /projects/:id/archive') + end + + it 'does not provide labels if route infos are missing' do + endpoint = double(:endpoint) + allow(endpoint).to receive(:route).and_raise + + env['api.endpoint'] = endpoint + + expect(transaction.labels).to eq({}) + expect(transaction.action).to be_nil + end + end + + context 'when request goes to ActionController' do + let(:content_type) { 'text/html' } + + before do + klass = double(:klass, name: 'TestController') + controller = double(:controller, class: klass, action_name: 'show', content_type: content_type) + + env['action_controller.instance'] = controller + end + + it 'tags a transaction with the name and action of a controller' do + expect(transaction.labels).to eq({ controller: 'TestController', action: 'show' }) + expect(transaction.action).to eq('TestController#show') + end + + context 'when the response content type is not :html' do + let(:content_type) { 'application/json' } + + it 'appends the mime type to the transaction action' do + expect(transaction.labels).to eq({ controller: 'TestController', action: 'show.json' }) + expect(transaction.action).to eq('TestController#show.json') + end + end + end + + it 'returns no labels when no route information is present in env' do + expect(transaction.labels).to eq({}) + expect(transaction.action).to eq(nil) + end + end + describe '#add_event' do it 'adds a metric' do transaction.add_event(:meow) diff --git a/spec/lib/gitlab/metrics_spec.rb b/spec/lib/gitlab/metrics_spec.rb index 599b8807d8d..1619fbd88b1 100644 --- a/spec/lib/gitlab/metrics_spec.rb +++ b/spec/lib/gitlab/metrics_spec.rb @@ -115,7 +115,7 @@ describe Gitlab::Metrics do end context 'with a transaction' do - let(:transaction) { Gitlab::Metrics::Transaction.new } + let(:transaction) { Gitlab::Metrics::WebTransaction.new({}) } before do allow(described_class).to receive(:current_transaction) @@ -124,13 +124,13 @@ describe Gitlab::Metrics do it 'adds a metric to the current transaction' do expect(transaction).to receive(:increment) - .with('foo_real_time', a_kind_of(Numeric)) + .with('foo_real_time', a_kind_of(Numeric), false) expect(transaction).to receive(:increment) - .with('foo_cpu_time', a_kind_of(Numeric)) + .with('foo_cpu_time', a_kind_of(Numeric), false) expect(transaction).to receive(:increment) - .with('foo_call_count', 1) + .with('foo_call_count', 1, false) described_class.measure(:foo) { 10 } end @@ -143,31 +143,6 @@ describe Gitlab::Metrics do end end - describe '.tag_transaction' do - context 'without a transaction' do - it 'does nothing' do - expect_any_instance_of(Gitlab::Metrics::Transaction) - .not_to receive(:add_tag) - - described_class.tag_transaction(:foo, 'bar') - end - end - - context 'with a transaction' do - let(:transaction) { Gitlab::Metrics::Transaction.new } - - it 'adds the tag to the transaction' do - expect(described_class).to receive(:current_transaction) - .and_return(transaction) - - expect(transaction).to receive(:add_tag) - .with(:foo, 'bar') - - described_class.tag_transaction(:foo, 'bar') - end - end - end - describe '.action=' do context 'without a transaction' do it 'does nothing' do @@ -180,7 +155,7 @@ describe Gitlab::Metrics do context 'with a transaction' do it 'sets the action of a transaction' do - trans = Gitlab::Metrics::Transaction.new + trans = Gitlab::Metrics::WebTransaction.new({}) expect(described_class).to receive(:current_transaction) .and_return(trans) @@ -210,7 +185,7 @@ describe Gitlab::Metrics do context 'with a transaction' do it 'adds an event' do - transaction = Gitlab::Metrics::Transaction.new + transaction = Gitlab::Metrics::WebTransaction.new({}) expect(transaction).to receive(:add_event).with(:meow) @@ -224,7 +199,7 @@ describe Gitlab::Metrics do shared_examples 'prometheus metrics API' do describe '#counter' do - subject { described_class.counter(:couter, 'doc') } + subject { described_class.counter(:counter, 'doc') } describe '#increment' do it 'successfully calls #increment without arguments' do @@ -280,7 +255,7 @@ describe Gitlab::Metrics do it_behaves_like 'prometheus metrics API' describe '#null_metric' do - subject { described_class.provide_metric(:test) } + subject { described_class.send(:provide_metric, :test) } it { is_expected.to be_a(Gitlab::Metrics::NullMetric) } end @@ -321,7 +296,7 @@ describe Gitlab::Metrics do it_behaves_like 'prometheus metrics API' describe '#null_metric' do - subject { described_class.provide_metric(:test) } + subject { described_class.send(:provide_metric, :test) } it { is_expected.to be_nil } end diff --git a/spec/lib/gitlab/middleware/rails_queue_duration_spec.rb b/spec/lib/gitlab/middleware/rails_queue_duration_spec.rb index 88107536c9e..14f2c3cb86f 100644 --- a/spec/lib/gitlab/middleware/rails_queue_duration_spec.rb +++ b/spec/lib/gitlab/middleware/rails_queue_duration_spec.rb @@ -4,7 +4,7 @@ describe Gitlab::Middleware::RailsQueueDuration do let(:app) { double(:app) } let(:middleware) { described_class.new(app) } let(:env) { {} } - let(:transaction) { double(:transaction) } + let(:transaction) { Gitlab::Metrics::WebTransaction.new(env) } before do expect(app).to receive(:call).with(env).and_return('yay') @@ -30,6 +30,16 @@ describe Gitlab::Middleware::RailsQueueDuration do expect(transaction).to receive(:set).with(:rails_queue_duration, an_instance_of(Float)) expect(middleware.call(env)).to eq('yay') end + + it 'observes rails queue duration metrics and calls the app when the header is present' do + env['HTTP_GITLAB_WORKHORSE_PROXY_START'] = '2000000000' + + expect(middleware.send(:metric_rails_queue_duration_seconds)).to receive(:observe).with(transaction.labels, 1) + + Timecop.freeze(Time.at(3)) do + expect(middleware.call(env)).to eq('yay') + end + end end end end diff --git a/spec/lib/gitlab/path_regex_spec.rb b/spec/lib/gitlab/path_regex_spec.rb index f1f188cbfb5..ee63c9338c5 100644 --- a/spec/lib/gitlab/path_regex_spec.rb +++ b/spec/lib/gitlab/path_regex_spec.rb @@ -68,14 +68,27 @@ describe Gitlab::PathRegex do message end - let(:all_routes) do + let(:all_non_legacy_routes) do route_set = Rails.application.routes routes_collection = route_set.routes routes_array = routes_collection.routes - routes_array.map { |route| route.path.spec.to_s } + + non_legacy_routes = routes_array.reject do |route| + route.name.to_s =~ /legacy_(\w*)_redirect/ + end + + non_deprecated_redirect_routes = non_legacy_routes.reject do |route| + app = route.app + # `app.app` is either another app, or `self`. We want to find the final app. + app = app.app while app.try(:app) && app.app != app + + app.is_a?(ActionDispatch::Routing::PathRedirect) && app.block.include?('/-/') + end + + non_deprecated_redirect_routes.map { |route| route.path.spec.to_s } end - let(:routes_without_format) { all_routes.map { |path| without_format(path) } } + let(:routes_without_format) { all_non_legacy_routes.map { |path| without_format(path) } } # Routes not starting with `/:` or `/*` # all routes not starting with a param diff --git a/spec/models/concerns/ignorable_column_spec.rb b/spec/models/concerns/ignorable_column_spec.rb index dba9fe43327..b70f2331a0e 100644 --- a/spec/models/concerns/ignorable_column_spec.rb +++ b/spec/models/concerns/ignorable_column_spec.rb @@ -5,7 +5,11 @@ describe IgnorableColumn 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')] + [ + Struct.new(:name).new('id'), + Struct.new(:name).new('title'), + Struct.new(:name).new('date') + ] end end end @@ -18,7 +22,7 @@ describe IgnorableColumn do describe '.columns' do it 'returns the columns, excluding the ignored ones' do - model.ignore_column(:title) + model.ignore_column(:title, :date) expect(model.columns.map(&:name)).to eq(%w(id)) end @@ -30,9 +34,9 @@ describe IgnorableColumn do end it 'returns the names of the ignored columns' do - model.ignore_column(:title) + model.ignore_column(:title, :date) - expect(model.ignored_columns).to eq(Set.new(%w(title))) + expect(model.ignored_columns).to eq(Set.new(%w(title date))) end end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 476a2697605..d022dae3476 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1755,39 +1755,12 @@ describe MergeRequest do end end - describe '#fetch_ref' do - it 'sets "ref_fetched" flag to true' do - subject.update!(ref_fetched: nil) + describe '#fetch_ref!' do + it 'fetches the ref correctly' do + expect { subject.target_project.repository.delete_refs(subject.ref_path) }.not_to raise_error - subject.fetch_ref - - expect(subject.reload.ref_fetched).to be_truthy - end - end - - describe '#ref_fetched?' do - it 'does not perform git operation when value is cached' do - subject.ref_fetched = true - - expect_any_instance_of(Repository).not_to receive(:ref_exists?) - expect(subject.ref_fetched?).to be_truthy - end - - it 'caches the value when ref exists but value is not cached' do - subject.update!(ref_fetched: nil) - allow_any_instance_of(Repository).to receive(:ref_exists?) - .and_return(true) - - expect(subject.ref_fetched?).to be_truthy - expect(subject.reload.ref_fetched).to be_truthy - end - - it 'returns false when ref does not exist' do - subject.update!(ref_fetched: nil) - allow_any_instance_of(Repository).to receive(:ref_exists?) - .and_return(false) - - expect(subject.ref_fetched?).to be_falsey + subject.fetch_ref! + expect(subject.target_project.repository.ref_exists?(subject.ref_path)).to be_truthy end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e8588975118..0e50909988b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -276,6 +276,12 @@ describe Project do expect(project).to be_valid end + + it 'allows a path ending in a period' do + project = build(:project, path: 'foo.') + + expect(project).to be_valid + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e0896d64c8f..d2f97009ad9 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -788,14 +788,16 @@ describe User do end it "creates external user by default" do - user = build(:user) + user = create(:user) expect(user.external).to be_truthy + expect(user.can_create_group).to be_falsey + expect(user.projects_limit).to be 0 end describe 'with default overrides' do it "creates a non-external user" do - user = build(:user, external: false) + user = create(:user, external: false) expect(user.external).to be_falsey end diff --git a/spec/requests/api/jobs_spec.rb b/spec/requests/api/jobs_spec.rb index 3b7b9c889e7..1765907c1b4 100644 --- a/spec/requests/api/jobs_spec.rb +++ b/spec/requests/api/jobs_spec.rb @@ -165,7 +165,17 @@ describe API::Jobs do context 'authorized user' do it 'returns specific job data' do expect(response).to have_gitlab_http_status(200) - expect(json_response['name']).to eq('test') + expect(json_response['id']).to eq(job.id) + expect(json_response['status']).to eq(job.status) + expect(json_response['stage']).to eq(job.stage) + expect(json_response['name']).to eq(job.name) + expect(json_response['ref']).to eq(job.ref) + expect(json_response['tag']).to eq(job.tag) + expect(json_response['coverage']).to eq(job.coverage) + expect(Time.parse(json_response['created_at'])).to be_like_time(job.created_at) + expect(Time.parse(json_response['started_at'])).to be_like_time(job.started_at) + expect(Time.parse(json_response['finished_at'])).to be_like_time(job.finished_at) + expect(json_response['duration']).to eq(job.duration) end it 'returns pipeline data' do diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index 024cfe8b372..e16be3c46e1 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -623,8 +623,6 @@ describe API::MergeRequests do before do forked_project.add_reporter(user2) - - allow_any_instance_of(MergeRequest).to receive(:write_ref) end it "returns merge_request" do diff --git a/spec/requests/api/v3/merge_requests_spec.rb b/spec/requests/api/v3/merge_requests_spec.rb index 26251b95680..91897e5ee01 100644 --- a/spec/requests/api/v3/merge_requests_spec.rb +++ b/spec/requests/api/v3/merge_requests_spec.rb @@ -319,8 +319,6 @@ describe API::MergeRequests do before do forked_project.add_reporter(user2) - - allow_any_instance_of(MergeRequest).to receive(:write_ref) end it "returns merge_request" do diff --git a/spec/services/events/render_service_spec.rb b/spec/services/events/render_service_spec.rb new file mode 100644 index 00000000000..b4a4a44d07b --- /dev/null +++ b/spec/services/events/render_service_spec.rb @@ -0,0 +1,37 @@ +require 'spec_helper' + +describe Events::RenderService do + describe '#execute' do + let!(:note) { build(:note) } + let!(:event) { build(:event, target: note, project: note.project) } + let!(:user) { build(:user) } + + context 'when the request format is atom' do + it 'renders the note inside events' do + expect(Banzai::ObjectRenderer).to receive(:new) + .with(event.project, user, + only_path: false, + xhtml: true) + .and_call_original + + expect_any_instance_of(Banzai::ObjectRenderer) + .to receive(:render).with([note], :note) + + described_class.new(user).execute([event], atom_request: true) + end + end + + context 'when the request format is not atom' do + it 'renders the note inside events' do + expect(Banzai::ObjectRenderer).to receive(:new) + .with(event.project, user, {}) + .and_call_original + + expect_any_instance_of(Banzai::ObjectRenderer) + .to receive(:render).with([note], :note) + + described_class.new(user).execute([event], atom_request: false) + end + end + end +end diff --git a/spec/services/notes/render_service_spec.rb b/spec/services/notes/render_service_spec.rb new file mode 100644 index 00000000000..faac498037f --- /dev/null +++ b/spec/services/notes/render_service_spec.rb @@ -0,0 +1,31 @@ +require 'spec_helper' + +describe Notes::RenderService do + describe '#execute' do + it 'renders a Note' do + note = double(:note) + project = double(:project) + wiki = double(:wiki) + user = double(:user) + + expect(Banzai::ObjectRenderer).to receive(:new) + .with(project, user, + requested_path: 'foo', + project_wiki: wiki, + ref: 'bar', + only_path: nil, + xhtml: false) + .and_call_original + + expect_any_instance_of(Banzai::ObjectRenderer) + .to receive(:render).with([note], :note) + + described_class.new(user).execute([note], project, + requested_path: 'foo', + project_wiki: wiki, + ref: 'bar', + only_path: nil, + xhtml: false) + end + end +end diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index a9b34a5258a..dc2673abc73 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -248,11 +248,11 @@ describe TodoService do end end - describe '#destroy_issue' do + describe '#destroy_issuable' do it 'refresh the todos count cache for the user' do expect(john_doe).to receive(:update_todos_count_cache).and_call_original - service.destroy_issue(issue, john_doe) + service.destroy_issuable(issue, john_doe) end end @@ -643,14 +643,6 @@ describe TodoService do end end - describe '#destroy_merge_request' do - it 'refresh the todos count cache for the user' do - expect(john_doe).to receive(:update_todos_count_cache).and_call_original - - service.destroy_merge_request(mr_assigned, john_doe) - end - end - describe '#reassigned_merge_request' do it 'creates a pending todo for new assignee' do mr_unassigned.update_attribute(:assignee, john_doe) diff --git a/spec/validators/dynamic_path_validator_spec.rb b/spec/validators/dynamic_path_validator_spec.rb deleted file mode 100644 index 08e1c5a728a..00000000000 --- a/spec/validators/dynamic_path_validator_spec.rb +++ /dev/null @@ -1,97 +0,0 @@ -require 'spec_helper' - -describe DynamicPathValidator do - let(:validator) { described_class.new(attributes: [:path]) } - - def expect_handles_invalid_utf8 - expect { yield('\255invalid') }.to be_falsey - end - - describe '.valid_user_path' do - it 'handles invalid utf8' do - expect(described_class.valid_user_path?("a\0weird\255path")).to be_falsey - end - end - - describe '.valid_group_path' do - it 'handles invalid utf8' do - expect(described_class.valid_group_path?("a\0weird\255path")).to be_falsey - end - end - - describe '.valid_project_path' do - it 'handles invalid utf8' do - expect(described_class.valid_project_path?("a\0weird\255path")).to be_falsey - end - end - - describe '#path_valid_for_record?' do - context 'for project' do - it 'calls valid_project_path?' do - project = build(:project, path: 'activity') - - expect(described_class).to receive(:valid_project_path?).with(project.full_path).and_call_original - - expect(validator.path_valid_for_record?(project, 'activity')).to be_truthy - end - end - - context 'for group' do - it 'calls valid_group_path?' do - group = build(:group, :nested, path: 'activity') - - expect(described_class).to receive(:valid_group_path?).with(group.full_path).and_call_original - - expect(validator.path_valid_for_record?(group, 'activity')).to be_falsey - end - end - - context 'for user' do - it 'calls valid_user_path?' do - user = build(:user, username: 'activity') - - expect(described_class).to receive(:valid_user_path?).with(user.full_path).and_call_original - - expect(validator.path_valid_for_record?(user, 'activity')).to be_truthy - end - end - - context 'for user namespace' do - it 'calls valid_user_path?' do - user = create(:user, username: 'activity') - namespace = user.namespace - - expect(described_class).to receive(:valid_user_path?).with(namespace.full_path).and_call_original - - expect(validator.path_valid_for_record?(namespace, 'activity')).to be_truthy - end - end - end - - describe '#validates_each' do - it 'adds a message when the path is not in the correct format' do - group = build(:group) - - validator.validate_each(group, :path, "Path with spaces, and comma's!") - - expect(group.errors[:path]).to include(Gitlab::PathRegex.namespace_format_message) - end - - it 'adds a message when the path is not in the correct format' do - group = build(:group, path: 'users') - - validator.validate_each(group, :path, 'users') - - expect(group.errors[:path]).to include('users is a reserved name') - end - - it 'updating to an invalid path is not allowed' do - project = create(:project) - project.path = 'update' - - validator.validate_each(project, :path, 'update') - - expect(project.errors[:path]).to include('update is a reserved name') - end - end -end diff --git a/spec/validators/namespace_path_validator_spec.rb b/spec/validators/namespace_path_validator_spec.rb new file mode 100644 index 00000000000..61e2845f35f --- /dev/null +++ b/spec/validators/namespace_path_validator_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe NamespacePathValidator do + let(:validator) { described_class.new(attributes: [:path]) } + + describe '.valid_path?' do + it 'handles invalid utf8' do + expect(described_class.valid_path?("a\0weird\255path")).to be_falsey + end + end + + describe '#validates_each' do + it 'adds a message when the path is not in the correct format' do + group = build(:group) + + validator.validate_each(group, :path, "Path with spaces, and comma's!") + + expect(group.errors[:path]).to include(Gitlab::PathRegex.namespace_format_message) + end + + it 'adds a message when the path is reserved when creating' do + group = build(:group, path: 'help') + + validator.validate_each(group, :path, 'help') + + expect(group.errors[:path]).to include('help is a reserved name') + end + + it 'adds a message when the path is reserved when updating' do + group = create(:group) + group.path = 'help' + + validator.validate_each(group, :path, 'help') + + expect(group.errors[:path]).to include('help is a reserved name') + end + end +end diff --git a/spec/validators/project_path_validator_spec.rb b/spec/validators/project_path_validator_spec.rb new file mode 100644 index 00000000000..8bb5e72dc22 --- /dev/null +++ b/spec/validators/project_path_validator_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe ProjectPathValidator do + let(:validator) { described_class.new(attributes: [:path]) } + + describe '.valid_path?' do + it 'handles invalid utf8' do + expect(described_class.valid_path?("a\0weird\255path")).to be_falsey + end + end + + describe '#validates_each' do + it 'adds a message when the path is not in the correct format' do + project = build(:project) + + validator.validate_each(project, :path, "Path with spaces, and comma's!") + + expect(project.errors[:path]).to include(Gitlab::PathRegex.project_path_format_message) + end + + it 'adds a message when the path is reserved when creating' do + project = build(:project, path: 'blob') + + validator.validate_each(project, :path, 'blob') + + expect(project.errors[:path]).to include('blob is a reserved name') + end + + it 'adds a message when the path is reserved when updating' do + project = create(:project) + project.path = 'blob' + + validator.validate_each(project, :path, 'blob') + + expect(project.errors[:path]).to include('blob is a reserved name') + end + end +end diff --git a/spec/validators/user_path_validator_spec.rb b/spec/validators/user_path_validator_spec.rb new file mode 100644 index 00000000000..a46089cc24f --- /dev/null +++ b/spec/validators/user_path_validator_spec.rb @@ -0,0 +1,38 @@ +require 'spec_helper' + +describe UserPathValidator do + let(:validator) { described_class.new(attributes: [:username]) } + + describe '.valid_path?' do + it 'handles invalid utf8' do + expect(described_class.valid_path?("a\0weird\255path")).to be_falsey + end + end + + describe '#validates_each' do + it 'adds a message when the path is not in the correct format' do + user = build(:user) + + validator.validate_each(user, :username, "Path with spaces, and comma's!") + + expect(user.errors[:username]).to include(Gitlab::PathRegex.namespace_format_message) + end + + it 'adds a message when the path is reserved when creating' do + user = build(:user, username: 'help') + + validator.validate_each(user, :username, 'help') + + expect(user.errors[:username]).to include('help is a reserved name') + end + + it 'adds a message when the path is reserved when updating' do + user = create(:user) + user.username = 'help' + + validator.validate_each(user, :username, 'help') + + expect(user.errors[:username]).to include('help is a reserved name') + end + end +end |