From d2091d1e924e2887eb9db4fad761965a24d024f1 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 10 Mar 2021 15:09:11 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- spec/experiments/application_experiment_spec.rb | 40 +- spec/frontend/experiment_tracking_spec.js | 80 ---- .../experimentation/experiment_tracking_spec.js | 80 ++++ spec/frontend/experimentation/utils_spec.js | 38 ++ spec/frontend/graphql_shared/utils_spec.js | 34 ++ spec/frontend/lib/utils/experimentation_spec.js | 20 - .../projects/upload_file_experiment_spec.js | 18 +- spec/frontend/tracking_spec.js | 26 +- .../states/mr_widget_nothing_to_merge_spec.js | 12 +- spec/lib/gitlab/database/similarity_score_spec.rb | 2 +- spec/lib/gitlab/database_spec.rb | 108 ++++++ .../graphql/pagination/keyset/last_items_spec.rb | 2 +- .../graphql/pagination/keyset/order_info_spec.rb | 12 - .../pagination/keyset/query_builder_spec.rb | 38 -- .../gitlab/metrics/background_transaction_spec.rb | 67 ++++ .../metrics/subscribers/active_record_spec.rb | 33 +- .../keyset/column_order_definition_spec.rb | 188 +++++++++ spec/lib/gitlab/pagination/keyset/order_spec.rb | 420 +++++++++++++++++++++ .../sidekiq_middleware/server_metrics_spec.rb | 8 + .../project_services/discord_service_spec.rb | 11 + .../api/graphql/project/merge_requests_spec.rb | 48 ++- spec/support/gitlab_experiment.rb | 11 + .../active_record_subscriber_shared_examples.rb | 57 ++- spec/support/snowplow.rb | 19 +- spec/support/stub_snowplow.rb | 23 ++ 25 files changed, 1139 insertions(+), 256 deletions(-) delete mode 100644 spec/frontend/experiment_tracking_spec.js create mode 100644 spec/frontend/experimentation/experiment_tracking_spec.js create mode 100644 spec/frontend/experimentation/utils_spec.js delete mode 100644 spec/frontend/lib/utils/experimentation_spec.js create mode 100644 spec/lib/gitlab/metrics/background_transaction_spec.rb create mode 100644 spec/lib/gitlab/pagination/keyset/column_order_definition_spec.rb create mode 100644 spec/lib/gitlab/pagination/keyset/order_spec.rb create mode 100644 spec/support/stub_snowplow.rb (limited to 'spec') diff --git a/spec/experiments/application_experiment_spec.rb b/spec/experiments/application_experiment_spec.rb index 2595512eec3..a0fe9f0f310 100644 --- a/spec/experiments/application_experiment_spec.rb +++ b/spec/experiments/application_experiment_spec.rb @@ -64,21 +64,35 @@ RSpec.describe ApplicationExperiment, :experiment do subject.publish(nil) end - it "pushes the experiment knowledge into the client using Gon.global" do - expect(Gon.global).to receive(:push).with( - { - experiment: { - 'namespaced/stub' => { # string key because it can be namespaced - experiment: 'namespaced/stub', - key: '86208ac54ca798e11f127e8b23ec396a', - variant: 'control' + context "when inside a request cycle" do + before do + subject.context.instance_variable_set(:@request, double('Request', headers: 'true')) + end + + it "pushes the experiment knowledge into the client using Gon" do + expect(Gon).to receive(:push).with( + { + experiment: { + 'namespaced/stub' => { # string key because it can be namespaced + experiment: 'namespaced/stub', + key: '86208ac54ca798e11f127e8b23ec396a', + variant: 'control' + } } - } - }, - true - ) + }, + true + ) - subject.publish(nil) + subject.publish(nil) + end + end + + context "when outside a request cycle" do + it "does not push to gon when outside request cycle" do + expect(Gon).not_to receive(:push) + + subject.publish(nil) + end end end diff --git a/spec/frontend/experiment_tracking_spec.js b/spec/frontend/experiment_tracking_spec.js deleted file mode 100644 index db209b783cb..00000000000 --- a/spec/frontend/experiment_tracking_spec.js +++ /dev/null @@ -1,80 +0,0 @@ -import ExperimentTracking from '~/experiment_tracking'; -import Tracking from '~/tracking'; - -jest.mock('~/tracking'); - -const oldGon = window.gon; - -let newGon = {}; -let experimentTracking; -let label; -let property; - -const setup = () => { - window.gon = newGon; - experimentTracking = new ExperimentTracking('sidebar_experiment', { label, property }); -}; - -beforeEach(() => { - document.body.dataset.page = 'issues-page'; -}); - -afterEach(() => { - window.gon = oldGon; - Tracking.mockClear(); - label = undefined; - property = undefined; -}); - -describe('event', () => { - describe('when experiment data exists for experimentName', () => { - beforeEach(() => { - newGon = { global: { experiment: { sidebar_experiment: 'experiment-data' } } }; - setup(); - }); - - describe('when providing options', () => { - label = 'sidebar-drawer'; - property = 'dark-mode'; - - it('passes them to the tracking call', () => { - experimentTracking.event('click_sidebar_close'); - - expect(Tracking.event).toHaveBeenCalledTimes(1); - expect(Tracking.event).toHaveBeenCalledWith('issues-page', 'click_sidebar_close', { - label: 'sidebar-drawer', - property: 'dark-mode', - context: { - schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/1-0-0', - data: 'experiment-data', - }, - }); - }); - }); - - it('tracks with the correct context', () => { - experimentTracking.event('click_sidebar_trigger'); - - expect(Tracking.event).toHaveBeenCalledTimes(1); - expect(Tracking.event).toHaveBeenCalledWith('issues-page', 'click_sidebar_trigger', { - context: { - schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/1-0-0', - data: 'experiment-data', - }, - }); - }); - }); - - describe('when experiment data does NOT exists for the experimentName', () => { - beforeEach(() => { - newGon = { global: { experiment: { unrelated_experiment: 'not happening' } } }; - setup(); - }); - - it('does not track', () => { - experimentTracking.event('click_sidebar_close'); - - expect(Tracking.event).not.toHaveBeenCalled(); - }); - }); -}); diff --git a/spec/frontend/experimentation/experiment_tracking_spec.js b/spec/frontend/experimentation/experiment_tracking_spec.js new file mode 100644 index 00000000000..20f45a7015a --- /dev/null +++ b/spec/frontend/experimentation/experiment_tracking_spec.js @@ -0,0 +1,80 @@ +import { TRACKING_CONTEXT_SCHEMA } from '~/experimentation/constants'; +import ExperimentTracking from '~/experimentation/experiment_tracking'; +import { getExperimentData } from '~/experimentation/utils'; +import Tracking from '~/tracking'; + +let experimentTracking; +let label; +let property; + +jest.mock('~/tracking'); +jest.mock('~/experimentation/utils', () => ({ getExperimentData: jest.fn() })); + +const setup = () => { + experimentTracking = new ExperimentTracking('sidebar_experiment', { label, property }); +}; + +beforeEach(() => { + document.body.dataset.page = 'issues-page'; +}); + +afterEach(() => { + label = undefined; + property = undefined; +}); + +describe('event', () => { + beforeEach(() => { + getExperimentData.mockReturnValue(undefined); + }); + + describe('when experiment data exists for experimentName', () => { + beforeEach(() => { + getExperimentData.mockReturnValue('experiment-data'); + setup(); + }); + + describe('when providing options', () => { + label = 'sidebar-drawer'; + property = 'dark-mode'; + + it('passes them to the tracking call', () => { + experimentTracking.event('click_sidebar_close'); + + expect(Tracking.event).toHaveBeenCalledTimes(1); + expect(Tracking.event).toHaveBeenCalledWith('issues-page', 'click_sidebar_close', { + label: 'sidebar-drawer', + property: 'dark-mode', + context: { + schema: TRACKING_CONTEXT_SCHEMA, + data: 'experiment-data', + }, + }); + }); + }); + + it('tracks with the correct context', () => { + experimentTracking.event('click_sidebar_trigger'); + + expect(Tracking.event).toHaveBeenCalledTimes(1); + expect(Tracking.event).toHaveBeenCalledWith('issues-page', 'click_sidebar_trigger', { + context: { + schema: TRACKING_CONTEXT_SCHEMA, + data: 'experiment-data', + }, + }); + }); + }); + + describe('when experiment data does NOT exists for the experimentName', () => { + beforeEach(() => { + setup(); + }); + + it('does not track', () => { + experimentTracking.event('click_sidebar_close'); + + expect(Tracking.event).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/spec/frontend/experimentation/utils_spec.js b/spec/frontend/experimentation/utils_spec.js new file mode 100644 index 00000000000..87dd2d595ba --- /dev/null +++ b/spec/frontend/experimentation/utils_spec.js @@ -0,0 +1,38 @@ +import * as experimentUtils from '~/experimentation/utils'; + +const TEST_KEY = 'abc'; + +describe('experiment Utilities', () => { + const oldGon = window.gon; + + afterEach(() => { + window.gon = oldGon; + }); + + describe('getExperimentData', () => { + it.each` + gon | input | output + ${{ experiment: { [TEST_KEY]: '_data_' } }} | ${[TEST_KEY]} | ${'_data_'} + ${{}} | ${[TEST_KEY]} | ${undefined} + `('with input=$input and gon=$gon, returns $output', ({ gon, input, output }) => { + window.gon = gon; + + expect(experimentUtils.getExperimentData(...input)).toEqual(output); + }); + }); + + describe('isExperimentVariant', () => { + it.each` + gon | input | output + ${{ experiment: { [TEST_KEY]: { variant: 'control' } } }} | ${[TEST_KEY, 'control']} | ${true} + ${{ experiment: { [TEST_KEY]: { variant: '_variant_name' } } }} | ${[TEST_KEY, '_variant_name']} | ${true} + ${{ experiment: { [TEST_KEY]: { variant: '_variant_name' } } }} | ${[TEST_KEY, '_bogus_name']} | ${false} + ${{ experiment: { [TEST_KEY]: { variant: '_variant_name' } } }} | ${['boguskey', '_variant_name']} | ${false} + ${{}} | ${[TEST_KEY, '_variant_name']} | ${false} + `('with input=$input and gon=$gon, returns $output', ({ gon, input, output }) => { + window.gon = gon; + + expect(experimentUtils.isExperimentVariant(...input)).toEqual(output); + }); + }); +}); diff --git a/spec/frontend/graphql_shared/utils_spec.js b/spec/frontend/graphql_shared/utils_spec.js index d392b0f0575..56bfb02ea4a 100644 --- a/spec/frontend/graphql_shared/utils_spec.js +++ b/spec/frontend/graphql_shared/utils_spec.js @@ -2,6 +2,8 @@ import { getIdFromGraphQLId, convertToGraphQLId, convertToGraphQLIds, + convertFromGraphQLIds, + convertNodeIdsFromGraphQLIds, } from '~/graphql_shared/utils'; const mockType = 'Group'; @@ -81,3 +83,35 @@ describe('convertToGraphQLIds', () => { expect(() => convertToGraphQLIds(type, ids)).toThrow(new TypeError(message)); }); }); + +describe('convertFromGraphQLIds', () => { + it.each` + ids | expected + ${[mockGid]} | ${[mockId]} + ${[mockGid, 'invalid id']} | ${[mockId, null]} + `('converts $ids from GraphQL Ids', ({ ids, expected }) => { + expect(convertFromGraphQLIds(ids)).toEqual(expected); + }); + + it("throws TypeError if `ids` parameter isn't an array", () => { + expect(() => convertFromGraphQLIds('invalid')).toThrow( + new TypeError('ids must be an array; got string'), + ); + }); +}); + +describe('convertNodeIdsFromGraphQLIds', () => { + it.each` + nodes | expected + ${[{ id: mockGid, name: 'foo bar' }, { id: mockGid, name: 'baz' }]} | ${[{ id: mockId, name: 'foo bar' }, { id: mockId, name: 'baz' }]} + ${[{ name: 'foo bar' }]} | ${[{ name: 'foo bar' }]} + `('converts `id` properties in $nodes from GraphQL Id', ({ nodes, expected }) => { + expect(convertNodeIdsFromGraphQLIds(nodes)).toEqual(expected); + }); + + it("throws TypeError if `nodes` parameter isn't an array", () => { + expect(() => convertNodeIdsFromGraphQLIds('invalid')).toThrow( + new TypeError('nodes must be an array; got string'), + ); + }); +}); diff --git a/spec/frontend/lib/utils/experimentation_spec.js b/spec/frontend/lib/utils/experimentation_spec.js deleted file mode 100644 index 2c5d2f89297..00000000000 --- a/spec/frontend/lib/utils/experimentation_spec.js +++ /dev/null @@ -1,20 +0,0 @@ -import * as experimentUtils from '~/lib/utils/experimentation'; - -const TEST_KEY = 'abc'; - -describe('experiment Utilities', () => { - describe('isExperimentEnabled', () => { - it.each` - experiments | value - ${{ [TEST_KEY]: true }} | ${true} - ${{ [TEST_KEY]: false }} | ${false} - ${{ def: true }} | ${false} - ${{}} | ${false} - ${null} | ${false} - `('returns correct value of $value for experiments=$experiments', ({ experiments, value }) => { - window.gon = { experiments }; - - expect(experimentUtils.isExperimentEnabled(TEST_KEY)).toEqual(value); - }); - }); -}); diff --git a/spec/frontend/projects/upload_file_experiment_spec.js b/spec/frontend/projects/upload_file_experiment_spec.js index 8cad49425f4..aa1b1c44a31 100644 --- a/spec/frontend/projects/upload_file_experiment_spec.js +++ b/spec/frontend/projects/upload_file_experiment_spec.js @@ -1,21 +1,13 @@ -import ExperimentTracking from '~/experiment_tracking'; +import ExperimentTracking from '~/experimentation/experiment_tracking'; import * as UploadFileExperiment from '~/projects/upload_file_experiment'; -const mockExperimentTrackingEvent = jest.fn(); -jest.mock('~/experiment_tracking', () => - jest.fn().mockImplementation(() => ({ - event: mockExperimentTrackingEvent, - })), -); +jest.mock('~/experimentation/experiment_tracking'); const fixture = `
`; const findModal = () => document.querySelector('[aria-modal="true"]'); const findTrigger = () => document.querySelector('.js-upload-file-experiment-trigger'); beforeEach(() => { - ExperimentTracking.mockClear(); - mockExperimentTrackingEvent.mockClear(); - document.body.innerHTML = fixture; }); @@ -31,7 +23,9 @@ describe('trackUploadFileFormSubmitted', () => { label: 'blob-upload-modal', property: 'empty', }); - expect(mockExperimentTrackingEvent).toHaveBeenCalledWith('click_upload_modal_form_submit'); + expect(ExperimentTracking.prototype.event).toHaveBeenCalledWith( + 'click_upload_modal_form_submit', + ); }); it('initializes ExperimentTracking with the correct arguments when the project is not empty', () => { @@ -53,6 +47,6 @@ describe('initUploadFileTrigger', () => { expect(findModal()).not.toExist(); findTrigger().click(); expect(findModal()).toExist(); - expect(mockExperimentTrackingEvent).toHaveBeenCalledWith('click_upload_modal_trigger'); + expect(ExperimentTracking.prototype.event).toHaveBeenCalledWith('click_upload_modal_trigger'); }); }); diff --git a/spec/frontend/tracking_spec.js b/spec/frontend/tracking_spec.js index 726dc0edede..6a22de3be5c 100644 --- a/spec/frontend/tracking_spec.js +++ b/spec/frontend/tracking_spec.js @@ -1,12 +1,18 @@ import { setHTMLFixture } from 'helpers/fixtures'; +import { TRACKING_CONTEXT_SCHEMA } from '~/experimentation/constants'; +import { getExperimentData } from '~/experimentation/utils'; import Tracking, { initUserTracking, initDefaultTrackers, STANDARD_CONTEXT } from '~/tracking'; +jest.mock('~/experimentation/utils', () => ({ getExperimentData: jest.fn() })); + describe('Tracking', () => { let snowplowSpy; let bindDocumentSpy; let trackLoadEventsSpy; beforeEach(() => { + getExperimentData.mockReturnValue(undefined); + window.snowplow = window.snowplow || (() => {}); window.snowplowOptions = { namespace: '_namespace_', @@ -245,18 +251,18 @@ describe('Tracking', () => { }); it('brings in experiment data if linked to an experiment', () => { - const data = { + const mockExperimentData = { variant: 'candidate', experiment: 'repo_integrations_link', key: '2bff73f6bb8cc11156c50a8ba66b9b8b', }; + getExperimentData.mockReturnValue(mockExperimentData); - window.gon.global = { experiment: { example: data } }; document.querySelector('[data-track-event="click_input3"]').click(); expect(eventSpy).toHaveBeenCalledWith('_category_', 'click_input3', { value: '_value_', - context: { schema: 'iglu:com.gitlab/gitlab_experiment/jsonschema/1-0-0', data }, + context: { schema: TRACKING_CONTEXT_SCHEMA, data: mockExperimentData }, }); }); }); @@ -301,21 +307,21 @@ describe('Tracking', () => { describe('tracking mixin', () => { describe('trackingOptions', () => { - it('return the options defined on initialisation', () => { + it('returns the options defined on initialisation', () => { const mixin = Tracking.mixin({ foo: 'bar' }); expect(mixin.computed.trackingOptions()).toEqual({ foo: 'bar' }); }); - it('local tracking value override and extend options', () => { + it('lets local tracking value override and extend options', () => { const mixin = Tracking.mixin({ foo: 'bar' }); - // the value of this in the vue lifecyle is different, but this serve the tests purposes + // The value of this in the Vue lifecyle is different, but this serves the test's purposes mixin.computed.tracking = { foo: 'baz', baz: 'bar' }; expect(mixin.computed.trackingOptions()).toEqual({ foo: 'baz', baz: 'bar' }); }); }); describe('trackingCategory', () => { - it('return the category set in the component properties first', () => { + it('returns the category set in the component properties first', () => { const mixin = Tracking.mixin({ category: 'foo' }); mixin.computed.tracking = { category: 'bar', @@ -323,12 +329,12 @@ describe('Tracking', () => { expect(mixin.computed.trackingCategory()).toBe('bar'); }); - it('return the category set in the options', () => { + it('returns the category set in the options', () => { const mixin = Tracking.mixin({ category: 'foo' }); expect(mixin.computed.trackingCategory()).toBe('foo'); }); - it('if no category is selected returns undefined', () => { + it('returns undefined if no category is selected', () => { const mixin = Tracking.mixin(); expect(mixin.computed.trackingCategory()).toBe(undefined); }); @@ -363,7 +369,7 @@ describe('Tracking', () => { expect(eventSpy).toHaveBeenCalledWith(undefined, 'foo', {}); }); - it('give precedence to data for category and options', () => { + it('gives precedence to data for category and options', () => { mixin.trackingCategory = mixin.trackingCategory(); mixin.trackingOptions = mixin.trackingOptions(); const data = { category: 'foo', label: 'baz' }; diff --git a/spec/frontend/vue_mr_widget/components/states/mr_widget_nothing_to_merge_spec.js b/spec/frontend/vue_mr_widget/components/states/mr_widget_nothing_to_merge_spec.js index bd0bd36ebc2..2c04905d3a9 100644 --- a/spec/frontend/vue_mr_widget/components/states/mr_widget_nothing_to_merge_spec.js +++ b/spec/frontend/vue_mr_widget/components/states/mr_widget_nothing_to_merge_spec.js @@ -14,20 +14,14 @@ describe('NothingToMerge', () => { it('should have correct elements', () => { expect(vm.$el.classList.contains('mr-widget-body')).toBeTruthy(); - expect(vm.$el.querySelector('a').href).toContain(newBlobPath); - expect(vm.$el.innerText).toContain( - "Currently there are no changes in this merge request's source branch", - ); - - expect(vm.$el.innerText.replace(/\s\s+/g, ' ')).toContain( - 'Please push new commits or use a different branch.', - ); + expect(vm.$el.querySelector('[data-testid="createFileButton"]').href).toContain(newBlobPath); + expect(vm.$el.innerText).toContain('Use merge requests to propose changes to your project'); }); it('should not show new blob link if there is no link available', () => { vm.mr.newBlobPath = null; Vue.nextTick(() => { - expect(vm.$el.querySelector('a')).toEqual(null); + expect(vm.$el.querySelector('[data-testid="createFileButton"]')).toEqual(null); }); }); }); diff --git a/spec/lib/gitlab/database/similarity_score_spec.rb b/spec/lib/gitlab/database/similarity_score_spec.rb index cf75e5a72d9..b7b66494390 100644 --- a/spec/lib/gitlab/database/similarity_score_spec.rb +++ b/spec/lib/gitlab/database/similarity_score_spec.rb @@ -71,7 +71,7 @@ RSpec.describe Gitlab::Database::SimilarityScore do let(:search) { 'xyz' } it 'results have 0 similarity score' do - expect(query_result.map { |row| row['similarity'] }).to all(eq(0)) + expect(query_result.map { |row| row['similarity'].to_f }).to all(eq(0)) end end end diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index 3175040167b..1553a989dba 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -441,4 +441,112 @@ RSpec.describe Gitlab::Database do end end end + + describe 'ActiveRecordBaseTransactionMetrics' do + def subscribe_events + events = [] + + begin + subscriber = ActiveSupport::Notifications.subscribe('transaction.active_record') do |e| + events << e + end + + yield + ensure + ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber + end + + events + end + + context 'without a transaction block' do + it 'does not publish a transaction event' do + events = subscribe_events do + User.first + end + + expect(events).to be_empty + end + end + + context 'within a transaction block' do + it 'publishes a transaction event' do + events = subscribe_events do + ActiveRecord::Base.transaction do + User.first + end + end + + expect(events.length).to be(1) + + event = events.first + expect(event).not_to be_nil + expect(event.duration).to be > 0.0 + expect(event.payload).to a_hash_including( + connection: be_a(ActiveRecord::ConnectionAdapters::AbstractAdapter) + ) + end + end + + context 'within an empty transaction block' do + it 'publishes a transaction event' do + events = subscribe_events do + ActiveRecord::Base.transaction {} + end + + expect(events.length).to be(1) + + event = events.first + expect(event).not_to be_nil + expect(event.duration).to be > 0.0 + expect(event.payload).to a_hash_including( + connection: be_a(ActiveRecord::ConnectionAdapters::AbstractAdapter) + ) + end + end + + context 'within a nested transaction block' do + it 'publishes multiple transaction events' do + events = subscribe_events do + ActiveRecord::Base.transaction do + ActiveRecord::Base.transaction do + ActiveRecord::Base.transaction do + User.first + end + end + end + end + + expect(events.length).to be(3) + + events.each do |event| + expect(event).not_to be_nil + expect(event.duration).to be > 0.0 + expect(event.payload).to a_hash_including( + connection: be_a(ActiveRecord::ConnectionAdapters::AbstractAdapter) + ) + end + end + end + + context 'within a cancelled transaction block' do + it 'publishes multiple transaction events' do + events = subscribe_events do + ActiveRecord::Base.transaction do + User.first + raise ActiveRecord::Rollback + end + end + + expect(events.length).to be(1) + + event = events.first + expect(event).not_to be_nil + expect(event.duration).to be > 0.0 + expect(event.payload).to a_hash_including( + connection: be_a(ActiveRecord::ConnectionAdapters::AbstractAdapter) + ) + end + end + end end diff --git a/spec/lib/gitlab/graphql/pagination/keyset/last_items_spec.rb b/spec/lib/gitlab/graphql/pagination/keyset/last_items_spec.rb index b45bb8b79d9..ec2ec4bf50d 100644 --- a/spec/lib/gitlab/graphql/pagination/keyset/last_items_spec.rb +++ b/spec/lib/gitlab/graphql/pagination/keyset/last_items_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe Gitlab::Graphql::Pagination::Keyset::LastItems do let_it_be(:merge_request) { create(:merge_request) } - let(:scope) { MergeRequest.order_merged_at_asc.with_order_id_desc } + let(:scope) { MergeRequest.order_merged_at_asc } subject { described_class.take_items(*args) } diff --git a/spec/lib/gitlab/graphql/pagination/keyset/order_info_spec.rb b/spec/lib/gitlab/graphql/pagination/keyset/order_info_spec.rb index eb28e6c8c0a..40ee47ece49 100644 --- a/spec/lib/gitlab/graphql/pagination/keyset/order_info_spec.rb +++ b/spec/lib/gitlab/graphql/pagination/keyset/order_info_spec.rb @@ -52,18 +52,6 @@ RSpec.describe Gitlab::Graphql::Pagination::Keyset::OrderInfo do end end - context 'when ordering by SIMILARITY' do - let(:relation) { Project.sorted_by_similarity_desc('test', include_in_select: true) } - - it 'assigns the right attribute name, named function, and direction' do - expect(order_list.count).to eq 2 - expect(order_list.first.attribute_name).to eq 'similarity' - expect(order_list.first.named_function).to be_kind_of(Arel::Nodes::Addition) - expect(order_list.first.named_function.to_sql).to include 'SIMILARITY(' - expect(order_list.first.sort_direction).to eq :desc - end - end - context 'when ordering by CASE', :aggregate_failuers do let(:relation) { Project.order(Arel::Nodes::Case.new(Project.arel_table[:pending_delete]).when(true).then(100).else(1000).asc) } diff --git a/spec/lib/gitlab/graphql/pagination/keyset/query_builder_spec.rb b/spec/lib/gitlab/graphql/pagination/keyset/query_builder_spec.rb index fa631aa5666..31c02fd43e8 100644 --- a/spec/lib/gitlab/graphql/pagination/keyset/query_builder_spec.rb +++ b/spec/lib/gitlab/graphql/pagination/keyset/query_builder_spec.rb @@ -131,43 +131,5 @@ RSpec.describe Gitlab::Graphql::Pagination::Keyset::QueryBuilder do end end end - - context 'when sorting using SIMILARITY' do - let(:relation) { Project.sorted_by_similarity_desc('test', include_in_select: true) } - let(:arel_table) { Project.arel_table } - let(:decoded_cursor) { { 'similarity' => 0.5, 'id' => 100 } } - let(:similarity_function_call) { Gitlab::Database::SimilarityScore::SIMILARITY_FUNCTION_CALL_WITH_ANNOTATION } - let(:similarity_sql) do - [ - "(#{similarity_function_call}(COALESCE(\"projects\".\"path\", ''), 'test') * CAST('1' AS numeric))", - "(#{similarity_function_call}(COALESCE(\"projects\".\"name\", ''), 'test') * CAST('0.7' AS numeric))", - "(#{similarity_function_call}(COALESCE(\"projects\".\"description\", ''), 'test') * CAST('0.2' AS numeric))" - ].join(' + ') - end - - context 'when no values are nil' do - context 'when :after' do - it 'generates the correct condition' do - conditions = builder.conditions.gsub(/\s+/, ' ') - - expect(conditions).to include "(#{similarity_sql} < 0.5)" - expect(conditions).to include '"projects"."id" < 100' - expect(conditions).to include "OR (#{similarity_sql} IS NULL)" - end - end - - context 'when :before' do - let(:before_or_after) { :before } - - it 'generates the correct condition' do - conditions = builder.conditions.gsub(/\s+/, ' ') - - expect(conditions).to include "(#{similarity_sql} > 0.5)" - expect(conditions).to include '"projects"."id" > 100' - expect(conditions).to include "OR ( #{similarity_sql} = 0.5" - end - end - end - end end 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..b31a2f7549a --- /dev/null +++ b/spec/lib/gitlab/metrics/background_transaction_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::BackgroundTransaction do + let(:transaction) { described_class.new } + let(:prometheus_metric) { instance_double(Prometheus::Client::Metric, base_labels: {}) } + + before do + allow(described_class).to receive(:prometheus_metric).and_return(prometheus_metric) + end + + describe '#run' do + it 'yields the supplied block' do + expect { |b| transaction.run(&b) }.to yield_control + end + + it 'stores the transaction in the current thread' do + transaction.run do + expect(Thread.current[described_class::BACKGROUND_THREAD_KEY]).to eq(transaction) + end + end + + it 'removes the transaction from the current thread upon completion' do + transaction.run { } + + expect(Thread.current[described_class::BACKGROUND_THREAD_KEY]).to be_nil + end + end + + describe '#labels' do + it 'provides labels with endpoint_id and feature_category' do + Labkit::Context.with_context(feature_category: 'projects', caller_id: 'TestWorker') do + expect(transaction.labels).to eq({ endpoint_id: 'TestWorker', feature_category: 'projects' }) + end + end + end + + RSpec.shared_examples 'metric with labels' do |metric_method| + it 'measures with correct labels and value' do + value = 1 + expect(prometheus_metric).to receive(metric_method).with({ endpoint_id: 'TestWorker', feature_category: 'projects' }, value) + + Labkit::Context.with_context(feature_category: 'projects', caller_id: 'TestWorker') do + transaction.send(metric_method, :test_metric, value) + end + end + end + + describe '#increment' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Counter, :increment, base_labels: {}) } + + it_behaves_like 'metric with labels', :increment + end + + describe '#set' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Gauge, :set, base_labels: {}) } + + it_behaves_like 'metric with labels', :set + end + + describe '#observe' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Histogram, :observe, base_labels: {}) } + + it_behaves_like 'metric with labels', :observe + 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 b19b94152a2..dffd37eeb9d 100644 --- a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb +++ b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb @@ -6,8 +6,7 @@ RSpec.describe Gitlab::Metrics::Subscribers::ActiveRecord do using RSpec::Parameterized::TableSyntax let(:env) { {} } - let(:transaction) { Gitlab::Metrics::WebTransaction.new(env) } - let(:subscriber) { described_class.new } + let(:subscriber) { described_class.new } let(:connection) { double(:connection) } let(:payload) { { sql: 'SELECT * FROM users WHERE id = 10', connection: connection } } @@ -47,33 +46,15 @@ RSpec.describe Gitlab::Metrics::Subscribers::ActiveRecord do with_them do let(:payload) { { name: name, sql: sql(sql_query, comments: comments), connection: connection } } - describe 'with a current transaction' do - before do - allow(subscriber).to receive(:current_transaction) - .at_least(:once) - .and_return(transaction) - end + it 'marks the current thread as using the database' do + # since it would already have been toggled by other specs + Thread.current[:uses_db_connection] = nil - it 'marks the current thread as using the database' do - # since it would already have been toggled by other specs - Thread.current[:uses_db_connection] = nil - - expect { subscriber.sql(event) }.to change { Thread.current[:uses_db_connection] }.from(nil).to(true) - end - - it_behaves_like 'record ActiveRecord metrics' - it_behaves_like 'store ActiveRecord info in RequestStore' + expect { subscriber.sql(event) }.to change { Thread.current[:uses_db_connection] }.from(nil).to(true) end - describe 'without a current transaction' do - it 'does not track any metrics' do - expect_any_instance_of(Gitlab::Metrics::Transaction) - .not_to receive(:increment) - subscriber.sql(event) - end - - it_behaves_like 'store ActiveRecord info in RequestStore' - end + it_behaves_like 'record ActiveRecord metrics' + it_behaves_like 'store ActiveRecord info in RequestStore' end end diff --git a/spec/lib/gitlab/pagination/keyset/column_order_definition_spec.rb b/spec/lib/gitlab/pagination/keyset/column_order_definition_spec.rb new file mode 100644 index 00000000000..6e9e987f90c --- /dev/null +++ b/spec/lib/gitlab/pagination/keyset/column_order_definition_spec.rb @@ -0,0 +1,188 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Pagination::Keyset::ColumnOrderDefinition do + let_it_be(:project_name_column) do + described_class.new( + attribute_name: :name, + order_expression: Project.arel_table[:name].asc, + nullable: :not_nullable, + distinct: true + ) + end + + let_it_be(:project_name_lower_column) do + described_class.new( + attribute_name: :name, + order_expression: Project.arel_table[:name].lower.desc, + nullable: :not_nullable, + distinct: true + ) + end + + let_it_be(:project_calculated_column_expression) do + # COALESCE("projects"."description", 'No Description') + Arel::Nodes::NamedFunction.new('COALESCE', [ + Project.arel_table[:description], + Arel.sql("'No Description'") + ]) + end + + let_it_be(:project_calculated_column) do + described_class.new( + attribute_name: :name, + column_expression: project_calculated_column_expression, + order_expression: project_calculated_column_expression.asc, + nullable: :not_nullable, + distinct: true + ) + end + + describe '#order_direction' do + context 'inferring order_direction from order_expression' do + it { expect(project_name_column).to be_ascending_order } + it { expect(project_name_column).not_to be_descending_order } + + it { expect(project_name_lower_column).to be_descending_order } + it { expect(project_name_lower_column).not_to be_ascending_order } + + it { expect(project_calculated_column).to be_ascending_order } + it { expect(project_calculated_column).not_to be_descending_order } + + it 'raises error when order direction cannot be infered' do + expect do + described_class.new( + attribute_name: :name, + column_expression: Project.arel_table[:name], + order_expression: 'name asc', + reversed_order_expression: 'name desc', + nullable: :not_nullable, + distinct: true + ) + end.to raise_error(RuntimeError, /Invalid or missing `order_direction`/) + end + + it 'does not raise error when order direction is explicitly given' do + column_order_definition = described_class.new( + attribute_name: :name, + column_expression: Project.arel_table[:name], + order_expression: 'name asc', + reversed_order_expression: 'name desc', + order_direction: :asc, + nullable: :not_nullable, + distinct: true + ) + + expect(column_order_definition).to be_ascending_order + end + end + end + + describe '#column_expression' do + context 'inferring column_expression from order_expression' do + it 'infers the correct column expression' do + column_order_definition = described_class.new(attribute_name: :name, order_expression: Project.arel_table[:name].asc) + + expect(column_order_definition.column_expression).to eq(Project.arel_table[:name]) + end + + it 'raises error when raw string is given as order expression' do + expect do + described_class.new(attribute_name: :name, order_expression: 'name DESC') + end.to raise_error(RuntimeError, /Couldn't calculate the column expression. Please pass an ARel node/) + end + end + end + + describe '#reversed_order_expression' do + it 'raises error when order cannot be reversed automatically' do + expect do + described_class.new( + attribute_name: :name, + column_expression: Project.arel_table[:name], + order_expression: 'name asc', + order_direction: :asc, + nullable: :not_nullable, + distinct: true + ) + end.to raise_error(RuntimeError, /Couldn't determine reversed order/) + end + end + + describe '#reverse' do + it { expect(project_name_column.reverse.order_expression).to eq(Project.arel_table[:name].desc) } + it { expect(project_name_column.reverse).to be_descending_order } + + it { expect(project_calculated_column.reverse.order_expression).to eq(project_calculated_column_expression.desc) } + it { expect(project_calculated_column.reverse).to be_descending_order } + + context 'when reversed_order_expression is given' do + it 'uses the given expression' do + column_order_definition = described_class.new( + attribute_name: :name, + column_expression: Project.arel_table[:name], + order_expression: 'name asc', + reversed_order_expression: 'name desc', + order_direction: :asc, + nullable: :not_nullable, + distinct: true + ) + + expect(column_order_definition.reverse.order_expression).to eq('name desc') + end + end + end + + describe '#nullable' do + context 'when the column is nullable' do + let(:nulls_last_order) do + described_class.new( + attribute_name: :name, + column_expression: Project.arel_table[:name], + order_expression: Gitlab::Database.nulls_last_order('merge_request_metrics.merged_at', :desc), + reversed_order_expression: Gitlab::Database.nulls_first_order('merge_request_metrics.merged_at', :asc), + order_direction: :desc, + nullable: :nulls_last, # null values are always last + distinct: false + ) + end + + it 'requires the position of the null values in the result' do + expect(nulls_last_order).to be_nulls_last + end + + it 'reverses nullable correctly' do + expect(nulls_last_order.reverse).to be_nulls_first + end + + it 'raises error when invalid nullable value is given' do + expect do + described_class.new( + attribute_name: :name, + column_expression: Project.arel_table[:name], + order_expression: Gitlab::Database.nulls_last_order('merge_request_metrics.merged_at', :desc), + reversed_order_expression: Gitlab::Database.nulls_first_order('merge_request_metrics.merged_at', :asc), + order_direction: :desc, + nullable: true, + distinct: false + ) + end.to raise_error(RuntimeError, /Invalid `nullable` is given/) + end + + it 'raises error when the column is nullable and distinct' do + expect do + described_class.new( + attribute_name: :name, + column_expression: Project.arel_table[:name], + order_expression: Gitlab::Database.nulls_last_order('merge_request_metrics.merged_at', :desc), + reversed_order_expression: Gitlab::Database.nulls_first_order('merge_request_metrics.merged_at', :asc), + order_direction: :desc, + nullable: :nulls_last, + distinct: true + ) + end.to raise_error(RuntimeError, /Invalid column definition/) + end + end + end +end diff --git a/spec/lib/gitlab/pagination/keyset/order_spec.rb b/spec/lib/gitlab/pagination/keyset/order_spec.rb new file mode 100644 index 00000000000..665f790ee47 --- /dev/null +++ b/spec/lib/gitlab/pagination/keyset/order_spec.rb @@ -0,0 +1,420 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Pagination::Keyset::Order do + let(:table) { Arel::Table.new(:my_table) } + let(:order) { nil } + + def run_query(query) + ActiveRecord::Base.connection.execute(query).to_a + end + + def build_query(order:, where_conditions: nil, limit: nil) + <<-SQL + SELECT id, year, month + FROM (#{table_data}) my_table (id, year, month) + WHERE #{where_conditions || '1=1'} + ORDER BY #{order} + LIMIT #{limit || 999}; + SQL + end + + def iterate_and_collect(order:, page_size:, where_conditions: nil) + all_items = [] + + loop do + paginated_items = run_query(build_query(order: order, where_conditions: where_conditions, limit: page_size)) + break if paginated_items.empty? + + all_items.concat(paginated_items) + last_item = paginated_items.last + cursor_attributes = order.cursor_attributes_for_node(last_item) + where_conditions = order.build_where_values(cursor_attributes).to_sql + end + + all_items + end + + subject do + run_query(build_query(order: order)) + end + + shared_examples 'order examples' do + it { expect(subject).to eq(expected) } + + context 'when paginating forwards' do + subject { iterate_and_collect(order: order, page_size: 2) } + + it { expect(subject).to eq(expected) } + + context 'with different page size' do + subject { iterate_and_collect(order: order, page_size: 5) } + + it { expect(subject).to eq(expected) } + end + end + + context 'when paginating backwards' do + subject do + last_item = expected.last + cursor_attributes = order.cursor_attributes_for_node(last_item) + where_conditions = order.reversed_order.build_where_values(cursor_attributes) + + iterate_and_collect(order: order.reversed_order, page_size: 2, where_conditions: where_conditions.to_sql) + end + + it do + expect(subject).to eq(expected.reverse[1..-1]) # removing one item because we used it to calculate cursor data for the "last" page in subject + end + end + end + + context 'when ordering by a distinct column' do + let(:table_data) do + <<-SQL + VALUES (1, 0, 0), + (2, 0, 0), + (3, 0, 0), + (4, 0, 0), + (5, 0, 0), + (6, 0, 0), + (7, 0, 0), + (8, 0, 0), + (9, 0, 0) + SQL + end + + let(:order) do + Gitlab::Pagination::Keyset::Order.build([ + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'id', + column_expression: table['id'], + order_expression: table['id'].desc, + nullable: :not_nullable, + distinct: true + ) + ]) + end + + let(:expected) do + [ + { "id" => 9, "year" => 0, "month" => 0 }, + { "id" => 8, "year" => 0, "month" => 0 }, + { "id" => 7, "year" => 0, "month" => 0 }, + { "id" => 6, "year" => 0, "month" => 0 }, + { "id" => 5, "year" => 0, "month" => 0 }, + { "id" => 4, "year" => 0, "month" => 0 }, + { "id" => 3, "year" => 0, "month" => 0 }, + { "id" => 2, "year" => 0, "month" => 0 }, + { "id" => 1, "year" => 0, "month" => 0 } + ] + end + + it_behaves_like 'order examples' + end + + context 'when ordering by two non-nullable columns and a distinct column' do + let(:table_data) do + <<-SQL + VALUES (1, 2010, 2), + (2, 2011, 1), + (3, 2009, 2), + (4, 2011, 1), + (5, 2011, 1), + (6, 2009, 2), + (7, 2010, 3), + (8, 2012, 4), + (9, 2013, 5) + SQL + end + + let(:order) do + Gitlab::Pagination::Keyset::Order.build([ + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'year', + column_expression: table['year'], + order_expression: table['year'].asc, + nullable: :not_nullable, + distinct: false + ), + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'month', + column_expression: table['month'], + order_expression: table['month'].asc, + nullable: :not_nullable, + distinct: false + ), + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'id', + column_expression: table['id'], + order_expression: table['id'].asc, + nullable: :not_nullable, + distinct: true + ) + ]) + end + + let(:expected) do + [ + { 'year' => 2009, 'month' => 2, 'id' => 3 }, + { 'year' => 2009, 'month' => 2, 'id' => 6 }, + { 'year' => 2010, 'month' => 2, 'id' => 1 }, + { 'year' => 2010, 'month' => 3, 'id' => 7 }, + { 'year' => 2011, 'month' => 1, 'id' => 2 }, + { 'year' => 2011, 'month' => 1, 'id' => 4 }, + { 'year' => 2011, 'month' => 1, 'id' => 5 }, + { 'year' => 2012, 'month' => 4, 'id' => 8 }, + { 'year' => 2013, 'month' => 5, 'id' => 9 } + ] + end + + it_behaves_like 'order examples' + end + + context 'when ordering by nullable columns and a distinct column' do + let(:table_data) do + <<-SQL + VALUES (1, 2010, null), + (2, 2011, 2), + (3, null, null), + (4, null, 5), + (5, 2010, null), + (6, 2011, 2), + (7, 2010, 2), + (8, 2012, 2), + (9, null, 2), + (10, null, null), + (11, 2010, 2) + SQL + end + + let(:order) do + Gitlab::Pagination::Keyset::Order.build([ + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'year', + column_expression: table['year'], + order_expression: Gitlab::Database.nulls_last_order('year', :asc), + reversed_order_expression: Gitlab::Database.nulls_first_order('year', :desc), + order_direction: :asc, + nullable: :nulls_last, + distinct: false + ), + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'month', + column_expression: table['month'], + order_expression: Gitlab::Database.nulls_last_order('month', :asc), + reversed_order_expression: Gitlab::Database.nulls_first_order('month', :desc), + order_direction: :asc, + nullable: :nulls_last, + distinct: false + ), + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'id', + column_expression: table['id'], + order_expression: table['id'].asc, + nullable: :not_nullable, + distinct: true + ) + ]) + end + + let(:expected) do + [ + { "id" => 7, "year" => 2010, "month" => 2 }, + { "id" => 11, "year" => 2010, "month" => 2 }, + { "id" => 1, "year" => 2010, "month" => nil }, + { "id" => 5, "year" => 2010, "month" => nil }, + { "id" => 2, "year" => 2011, "month" => 2 }, + { "id" => 6, "year" => 2011, "month" => 2 }, + { "id" => 8, "year" => 2012, "month" => 2 }, + { "id" => 9, "year" => nil, "month" => 2 }, + { "id" => 4, "year" => nil, "month" => 5 }, + { "id" => 3, "year" => nil, "month" => nil }, + { "id" => 10, "year" => nil, "month" => nil } + ] + end + + it_behaves_like 'order examples' + end + + context 'when ordering by nullable columns with nulls first ordering and a distinct column' do + let(:table_data) do + <<-SQL + VALUES (1, 2010, null), + (2, 2011, 2), + (3, null, null), + (4, null, 5), + (5, 2010, null), + (6, 2011, 2), + (7, 2010, 2), + (8, 2012, 2), + (9, null, 2), + (10, null, null), + (11, 2010, 2) + SQL + end + + let(:order) do + Gitlab::Pagination::Keyset::Order.build([ + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'year', + column_expression: table['year'], + order_expression: Gitlab::Database.nulls_first_order('year', :asc), + reversed_order_expression: Gitlab::Database.nulls_last_order('year', :desc), + order_direction: :asc, + nullable: :nulls_first, + distinct: false + ), + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'month', + column_expression: table['month'], + order_expression: Gitlab::Database.nulls_first_order('month', :asc), + order_direction: :asc, + reversed_order_expression: Gitlab::Database.nulls_last_order('month', :desc), + nullable: :nulls_first, + distinct: false + ), + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'id', + column_expression: table['id'], + order_expression: table['id'].asc, + nullable: :not_nullable, + distinct: true + ) + ]) + end + + let(:expected) do + [ + { "id" => 3, "year" => nil, "month" => nil }, + { "id" => 10, "year" => nil, "month" => nil }, + { "id" => 9, "year" => nil, "month" => 2 }, + { "id" => 4, "year" => nil, "month" => 5 }, + { "id" => 1, "year" => 2010, "month" => nil }, + { "id" => 5, "year" => 2010, "month" => nil }, + { "id" => 7, "year" => 2010, "month" => 2 }, + { "id" => 11, "year" => 2010, "month" => 2 }, + { "id" => 2, "year" => 2011, "month" => 2 }, + { "id" => 6, "year" => 2011, "month" => 2 }, + { "id" => 8, "year" => 2012, "month" => 2 } + ] + end + + it_behaves_like 'order examples' + end + + context 'when ordering by non-nullable columns with mixed directions and a distinct column' do + let(:table_data) do + <<-SQL + VALUES (1, 2010, 0), + (2, 2011, 0), + (3, 2010, 0), + (4, 2010, 0), + (5, 2012, 0), + (6, 2012, 0), + (7, 2010, 0), + (8, 2011, 0), + (9, 2013, 0), + (10, 2014, 0), + (11, 2013, 0) + SQL + end + + let(:order) do + Gitlab::Pagination::Keyset::Order.build([ + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'year', + column_expression: table['year'], + order_expression: table['year'].asc, + nullable: :not_nullable, + distinct: false + ), + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'id', + column_expression: table['id'], + order_expression: table['id'].desc, + nullable: :not_nullable, + distinct: true + ) + ]) + end + + let(:expected) do + [ + { "id" => 7, "year" => 2010, "month" => 0 }, + { "id" => 4, "year" => 2010, "month" => 0 }, + { "id" => 3, "year" => 2010, "month" => 0 }, + { "id" => 1, "year" => 2010, "month" => 0 }, + { "id" => 8, "year" => 2011, "month" => 0 }, + { "id" => 2, "year" => 2011, "month" => 0 }, + { "id" => 6, "year" => 2012, "month" => 0 }, + { "id" => 5, "year" => 2012, "month" => 0 }, + { "id" => 11, "year" => 2013, "month" => 0 }, + { "id" => 9, "year" => 2013, "month" => 0 }, + { "id" => 10, "year" => 2014, "month" => 0 } + ] + end + + it 'takes out a slice between two cursors' do + after_cursor = { "id" => 8, "year" => 2011 } + before_cursor = { "id" => 5, "year" => 2012 } + + after_conditions = order.build_where_values(after_cursor) + reversed = order.reversed_order + before_conditions = reversed.build_where_values(before_cursor) + + query = build_query(order: order, where_conditions: "(#{after_conditions.to_sql}) AND (#{before_conditions.to_sql})", limit: 100) + + expect(run_query(query)).to eq([ + { "id" => 2, "year" => 2011, "month" => 0 }, + { "id" => 6, "year" => 2012, "month" => 0 } + ]) + end + end + + context 'when the passed cursor values do not match with the order definition' do + let(:order) do + Gitlab::Pagination::Keyset::Order.build([ + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'year', + column_expression: table['year'], + order_expression: table['year'].asc, + nullable: :not_nullable, + distinct: false + ), + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: 'id', + column_expression: table['id'], + order_expression: table['id'].desc, + nullable: :not_nullable, + distinct: true + ) + ]) + end + + context 'when values are missing' do + it 'raises error' do + expect { order.build_where_values(id: 1) }.to raise_error(/Missing items: year/) + end + end + + context 'when extra values are present' do + it 'raises error' do + expect { order.build_where_values(id: 1, year: 2, foo: 3) }.to raise_error(/Extra items: foo/) + end + end + + context 'when values are missing and extra values are present' do + it 'raises error' do + expect { order.build_where_values(year: 2, foo: 3) }.to raise_error(/Extra items: foo\. Missing items: id/) + end + end + + context 'when no values are passed' do + it 'returns nil' do + expect(order.build_where_values({})).to eq(nil) + end + end + end +end diff --git a/spec/lib/gitlab/sidekiq_middleware/server_metrics_spec.rb b/spec/lib/gitlab/sidekiq_middleware/server_metrics_spec.rb index de37b997d13..71f4f2a3b64 100644 --- a/spec/lib/gitlab/sidekiq_middleware/server_metrics_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware/server_metrics_spec.rb @@ -113,6 +113,14 @@ RSpec.describe Gitlab::SidekiqMiddleware::ServerMetrics do expect { |b| subject.call(worker, job, :test, &b) }.to yield_control.once end + it 'calls BackgroundTransaction' do + expect_next_instance_of(Gitlab::Metrics::BackgroundTransaction) do |instance| + expect(instance).to receive(:run) + end + + subject.call(worker, job, :test) {} + end + it 'sets queue specific metrics' do expect(running_jobs_metric).to receive(:increment).with(labels, -1) expect(running_jobs_metric).to receive(:increment).with(labels, 1) diff --git a/spec/models/project_services/discord_service_spec.rb b/spec/models/project_services/discord_service_spec.rb index 67e5ba79f01..ffe0a36dcdc 100644 --- a/spec/models/project_services/discord_service_spec.rb +++ b/spec/models/project_services/discord_service_spec.rb @@ -67,5 +67,16 @@ RSpec.describe DiscordService do expect { subject.execute(sample_data) }.to raise_error(ArgumentError, /is blocked/) end end + + context 'when the Discord request fails' do + before do + WebMock.stub_request(:post, webhook_url).to_return(status: 400) + end + + it 'logs an error and returns false' do + expect(subject).to receive(:log_error).with('400 Bad Request') + expect(subject.execute(sample_data)).to be(false) + end + end end end diff --git a/spec/requests/api/graphql/project/merge_requests_spec.rb b/spec/requests/api/graphql/project/merge_requests_spec.rb index ba40eec9b69..d97a0ed9399 100644 --- a/spec/requests/api/graphql/project/merge_requests_spec.rb +++ b/spec/requests/api/graphql/project/merge_requests_spec.rb @@ -381,29 +381,41 @@ RSpec.describe 'getting merge request listings nested in a project' do end context 'when sorting by merged_at DESC' do - it_behaves_like 'sorted paginated query' do - let(:sort_param) { :MERGED_AT_DESC } - let(:first_param) { 2 } + let(:sort_param) { :MERGED_AT_DESC } + let(:expected_results) do + [ + merge_request_b, + merge_request_d, + merge_request_c, + merge_request_e, + merge_request_a + ].map { |mr| global_id_of(mr) } + end - let(:expected_results) do - [ - merge_request_b, - merge_request_d, - merge_request_c, - merge_request_e, - merge_request_a - ].map { |mr| global_id_of(mr) } - end + before do + five_days_ago = 5.days.ago + + merge_request_d.metrics.update!(merged_at: five_days_ago) - before do - five_days_ago = 5.days.ago + # same merged_at, the second order column will decide (merge_request.id) + merge_request_c.metrics.update!(merged_at: five_days_ago) + + merge_request_b.metrics.update!(merged_at: 1.day.ago) + end + + it_behaves_like 'sorted paginated query' do + let(:first_param) { 2 } + end - merge_request_d.metrics.update!(merged_at: five_days_ago) + context 'when last parameter is given' do + let(:params) { graphql_args(sort: sort_param, last: 2) } + let(:page_info) { nil } - # same merged_at, the second order column will decide (merge_request.id) - merge_request_c.metrics.update!(merged_at: five_days_ago) + it 'takes the last 2 records' do + query = pagination_query(params) + post_graphql(query, current_user: current_user) - merge_request_b.metrics.update!(merged_at: 1.day.ago) + expect(results.map { |item| item["id"] }).to eq(expected_results.last(2)) end end end diff --git a/spec/support/gitlab_experiment.rb b/spec/support/gitlab_experiment.rb index 4a5b1fe73c9..4015db329fc 100644 --- a/spec/support/gitlab_experiment.rb +++ b/spec/support/gitlab_experiment.rb @@ -2,6 +2,7 @@ # Require the provided spec helper and matchers. require 'gitlab/experiment/rspec' +require_relative 'stub_snowplow' # This is a temporary fix until we have a larger discussion around the # challenges raised in https://gitlab.com/gitlab-org/gitlab/-/issues/300104 @@ -10,11 +11,21 @@ class ApplicationExperiment # rubocop:disable Gitlab/NamespacedClass super(...) Feature.persist_used!(feature_flag_name) end + + def should_track? + true + end end RSpec.configure do |config| + config.include StubSnowplow, :experiment + # Disable all caching for experiments in tests. config.before do allow(Gitlab::Experiment::Configuration).to receive(:cache).and_return(nil) end + + config.before(:each, :experiment) do + stub_snowplow + end end diff --git a/spec/support/shared_examples/metrics/active_record_subscriber_shared_examples.rb b/spec/support/shared_examples/metrics/active_record_subscriber_shared_examples.rb index 17f7b765bc5..7bf2456c548 100644 --- a/spec/support/shared_examples/metrics/active_record_subscriber_shared_examples.rb +++ b/spec/support/shared_examples/metrics/active_record_subscriber_shared_examples.rb @@ -42,7 +42,7 @@ RSpec.shared_examples 'store ActiveRecord info in RequestStore' do |db_role| end end -RSpec.shared_examples 'record ActiveRecord metrics' do |db_role| +RSpec.shared_examples 'record ActiveRecord metrics in a metrics transaction' do |db_role| it 'increments only db counters' do if record_query expect(transaction).to receive(:increment).with(:gitlab_transaction_db_count_total, 1) @@ -80,3 +80,58 @@ RSpec.shared_examples 'record ActiveRecord metrics' do |db_role| subscriber.sql(event) end end + +RSpec.shared_examples 'record ActiveRecord metrics' do |db_role| + context 'when both web and background transaction are available' do + let(:transaction) { double('Gitlab::Metrics::WebTransaction') } + let(:background_transaction) { double('Gitlab::Metrics::WebTransaction') } + + before do + allow(::Gitlab::Metrics::WebTransaction).to receive(:current) + .and_return(transaction) + allow(::Gitlab::Metrics::BackgroundTransaction).to receive(:current) + .and_return(background_transaction) + allow(transaction).to receive(:increment) + allow(transaction).to receive(:observe) + end + + it_behaves_like 'record ActiveRecord metrics in a metrics transaction', db_role + + it 'captures the metrics for web only' do + expect(background_transaction).not_to receive(:observe) + expect(background_transaction).not_to receive(:increment) + + subscriber.sql(event) + end + end + + context 'when web transaction is available' do + let(:transaction) { double('Gitlab::Metrics::WebTransaction') } + + before do + allow(::Gitlab::Metrics::WebTransaction).to receive(:current) + .and_return(transaction) + allow(::Gitlab::Metrics::BackgroundTransaction).to receive(:current) + .and_return(nil) + allow(transaction).to receive(:increment) + allow(transaction).to receive(:observe) + end + + it_behaves_like 'record ActiveRecord metrics in a metrics transaction', db_role + end + + context 'when background transaction is available' do + let(:transaction) { double('Gitlab::Metrics::BackgroundTransaction') } + + before do + allow(::Gitlab::Metrics::WebTransaction).to receive(:current) + .and_return(nil) + allow(::Gitlab::Metrics::BackgroundTransaction).to receive(:current) + .and_return(transaction) + allow(transaction).to receive(:increment) + allow(transaction).to receive(:observe) + end + + it_behaves_like 'record ActiveRecord metrics in a metrics transaction', db_role + end +end diff --git a/spec/support/snowplow.rb b/spec/support/snowplow.rb index 0d6102f1705..e58be667b37 100644 --- a/spec/support/snowplow.rb +++ b/spec/support/snowplow.rb @@ -1,24 +1,13 @@ # frozen_string_literal: true +require_relative 'stub_snowplow' + RSpec.configure do |config| config.include SnowplowHelpers, :snowplow + config.include StubSnowplow, :snowplow config.before(:each, :snowplow) do - # Using a high buffer size to not cause early flushes - buffer_size = 100 - # WebMock is set up to allow requests to `localhost` - host = 'localhost' - - allow_any_instance_of(Gitlab::Tracking::Destinations::ProductAnalytics).to receive(:event) - - allow_any_instance_of(Gitlab::Tracking::Destinations::Snowplow) - .to receive(:emitter) - .and_return(SnowplowTracker::Emitter.new(host, buffer_size: buffer_size)) - - stub_application_setting(snowplow_enabled: true) - - allow(SnowplowTracker::SelfDescribingJson).to receive(:new).and_call_original - allow(Gitlab::Tracking).to receive(:event).and_call_original # rubocop:disable RSpec/ExpectGitlabTracking + stub_snowplow end config.after(:each, :snowplow) do diff --git a/spec/support/stub_snowplow.rb b/spec/support/stub_snowplow.rb new file mode 100644 index 00000000000..a21ce2399d7 --- /dev/null +++ b/spec/support/stub_snowplow.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module StubSnowplow + def stub_snowplow + # Using a high buffer size to not cause early flushes + buffer_size = 100 + # WebMock is set up to allow requests to `localhost` + host = 'localhost' + + # rubocop:disable RSpec/AnyInstanceOf + allow_any_instance_of(Gitlab::Tracking::Destinations::ProductAnalytics).to receive(:event) + + allow_any_instance_of(Gitlab::Tracking::Destinations::Snowplow) + .to receive(:emitter) + .and_return(SnowplowTracker::Emitter.new(host, buffer_size: buffer_size)) + # rubocop:enable RSpec/AnyInstanceOf + + stub_application_setting(snowplow_enabled: true) + + allow(SnowplowTracker::SelfDescribingJson).to receive(:new).and_call_original + allow(Gitlab::Tracking).to receive(:event).and_call_original # rubocop:disable RSpec/ExpectGitlabTracking + end +end -- cgit v1.2.1