diff options
43 files changed, 512 insertions, 142 deletions
diff --git a/app/assets/stylesheets/highlight/common.scss b/app/assets/stylesheets/highlight/common.scss index 5c6668814cb..6c050f33b07 100644 --- a/app/assets/stylesheets/highlight/common.scss +++ b/app/assets/stylesheets/highlight/common.scss @@ -36,11 +36,11 @@ transition: border-left 0.1s ease-out; &.coverage { - border-left: 3px solid $coverage; + border-left: 4px solid $coverage; } &.no-coverage { - border-left: 3px solid $no-coverage; + border-left: 2px solid $no-coverage; } } diff --git a/app/assets/stylesheets/highlight/themes/dark.scss b/app/assets/stylesheets/highlight/themes/dark.scss index ecfff50726a..64387fbce09 100644 --- a/app/assets/stylesheets/highlight/themes/dark.scss +++ b/app/assets/stylesheets/highlight/themes/dark.scss @@ -24,8 +24,8 @@ $dark-pre-hll-bg: #373b41; $dark-hll-bg: #373b41; $dark-over-bg: #9f9ab5; $dark-expanded-bg: #3e3e3e; -$dark-coverage: #b5bd68; -$dark-no-coverage: #de935f; +$dark-coverage: #b3e841; +$dark-no-coverage: #ff4f33; $dark-c: #969896; $dark-err: #c66; $dark-k: #b294bb; diff --git a/app/assets/stylesheets/highlight/white_base.scss b/app/assets/stylesheets/highlight/white_base.scss index e54dc685515..777332881f3 100644 --- a/app/assets/stylesheets/highlight/white_base.scss +++ b/app/assets/stylesheets/highlight/white_base.scss @@ -214,7 +214,7 @@ pre.code, } .line-coverage { - @include line-coverage-border-color($green-500, $orange-500); + @include line-coverage-border-color($green-400, $red-400); &.old { background-color: $line-removed; diff --git a/app/finders/alert_management/alerts_finder.rb b/app/finders/alert_management/alerts_finder.rb index be3b329fb6f..8e0444d324a 100644 --- a/app/finders/alert_management/alerts_finder.rb +++ b/app/finders/alert_management/alerts_finder.rb @@ -32,6 +32,8 @@ module AlertManagement attr_reader :current_user, :project, :params def by_domain(collection) + return collection if params[:iid].present? + collection.with_operations_alerts end diff --git a/app/graphql/resolvers/concerns/resolves_merge_requests.rb b/app/graphql/resolvers/concerns/resolves_merge_requests.rb index ab83476ddea..31444b0c592 100644 --- a/app/graphql/resolvers/concerns/resolves_merge_requests.rb +++ b/app/graphql/resolvers/concerns/resolves_merge_requests.rb @@ -40,6 +40,8 @@ module ResolvesMergeRequests def preloads { assignees: [:assignees], + reviewers: [:reviewers], + participants: MergeRequest.participant_includes, labels: [:labels], author: [:author], merged_at: [:metrics], @@ -47,6 +49,7 @@ module ResolvesMergeRequests diff_stats_summary: [:metrics], approved_by: [:approved_by_users], milestone: [:milestone], + security_auto_fix: [:author], head_pipeline: [:merge_request_diff, { head_pipeline: [:merge_request] }] } end diff --git a/app/graphql/types/merge_request_type.rb b/app/graphql/types/merge_request_type.rb index 816160e58f7..e7d6ce4ffcb 100644 --- a/app/graphql/types/merge_request_type.rb +++ b/app/graphql/types/merge_request_type.rb @@ -126,10 +126,12 @@ module Types description: 'The milestone of the merge request' field :assignees, Types::UserType.connection_type, null: true, complexity: 5, description: 'Assignees of the merge request' + field :reviewers, Types::UserType.connection_type, null: true, complexity: 5, + description: 'Users from whom a review has been requested.' field :author, Types::UserType, null: true, description: 'User who created this merge request' field :participants, Types::UserType.connection_type, null: true, complexity: 5, - description: 'Participants in the merge request' + description: 'Participants in the merge request. This includes the author, assignees, reviewers, and users mentioned in notes.' field :subscribed, GraphQL::BOOLEAN_TYPE, method: :subscribed?, null: false, complexity: 5, description: 'Indicates if the currently logged in user is subscribed to this merge request' field :labels, Types::LabelType.connection_type, null: true, complexity: 5, @@ -235,6 +237,10 @@ module Types def security_auto_fix object.author == User.security_bot end + + def reviewers + object.reviewers if object.allows_reviewers? + end end end diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index ee9c2501bfc..b0aecbfc86d 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -159,6 +159,12 @@ class CommitStatus < ApplicationRecord commit_status.failure_reason = CommitStatus.failure_reasons[failure_reason] end + before_transition [:skipped, :manual] => :created do |commit_status, transition| + transition.args.first.try do |user| + commit_status.user = user + end + end + after_transition do |commit_status, transition| next if transition.loopback? next if commit_status.processed? diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index c3a394c1ca5..f04fcf34f43 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -216,6 +216,10 @@ module Issuable end class_methods do + def participant_includes + [:assignees, :author, { notes: [:author, :award_emoji] }] + end + # Searches for records with a matching title. # # This method uses ILIKE on PostgreSQL. diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index cbb647765fd..5b0b8dc4e55 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -493,6 +493,10 @@ class MergeRequest < ApplicationRecord work_in_progress?(title) ? title : "Draft: #{title}" end + def self.participant_includes + [:reviewers, :award_emoji] + super + end + def committers @committers ||= commits.committers end diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index f397ada0696..e5e79f70616 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -2,6 +2,8 @@ module Ci class RetryBuildService < ::BaseService + include Gitlab::OptimisticLocking + def self.clone_accessors %i[pipeline project ref tag options name allow_failure stage stage_id stage_idx trigger_request @@ -65,8 +67,8 @@ module Ci end def mark_subsequent_stages_as_processable(build) - build.pipeline.processables.skipped.after_stage(build.stage_idx).find_each do |processable| - Gitlab::OptimisticLocking.retry_lock(processable, &:process) + build.pipeline.processables.skipped.after_stage(build.stage_idx).find_each do |skipped| + retry_optimistic_lock(skipped) { |build| build.process(current_user) } end end end diff --git a/app/services/ci/retry_pipeline_service.rb b/app/services/ci/retry_pipeline_service.rb index 45244d16393..dea4bf73a4c 100644 --- a/app/services/ci/retry_pipeline_service.rb +++ b/app/services/ci/retry_pipeline_service.rb @@ -23,7 +23,7 @@ module Ci end pipeline.builds.latest.skipped.find_each do |skipped| - retry_optimistic_lock(skipped) { |build| build.process } + retry_optimistic_lock(skipped) { |build| build.process(current_user) } end pipeline.reset_ancestor_bridges! diff --git a/changelogs/unreleased/278469-docs-feedback-hard-to-understand-code-coverage-colouring.yml b/changelogs/unreleased/278469-docs-feedback-hard-to-understand-code-coverage-colouring.yml new file mode 100644 index 00000000000..94463ee22dc --- /dev/null +++ b/changelogs/unreleased/278469-docs-feedback-hard-to-understand-code-coverage-colouring.yml @@ -0,0 +1,6 @@ +--- +title: Visually enhance the difference between code that has and does not have test + coverage +merge_request: 49724 +author: +type: other diff --git a/changelogs/unreleased/ajk-gql-merge-request-reviewers.yml b/changelogs/unreleased/ajk-gql-merge-request-reviewers.yml new file mode 100644 index 00000000000..69ad109aeb2 --- /dev/null +++ b/changelogs/unreleased/ajk-gql-merge-request-reviewers.yml @@ -0,0 +1,5 @@ +--- +title: Adds MergeRequest.reviewers to GraphQL API +merge_request: 49707 +author: +type: changed diff --git a/changelogs/unreleased/mc-feature-associate-restarted-pipeline-triggerer.yml b/changelogs/unreleased/mc-feature-associate-restarted-pipeline-triggerer.yml new file mode 100644 index 00000000000..9fa9e0312f6 --- /dev/null +++ b/changelogs/unreleased/mc-feature-associate-restarted-pipeline-triggerer.yml @@ -0,0 +1,5 @@ +--- +title: When retrying jobs associate subsequent jobs with triggering user. +merge_request: 49833 +author: +type: changed diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index 31ecc01fa71..0ec30272c51 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -13491,7 +13491,7 @@ type MergeRequest implements CurrentUserTodos & Noteable { ): NoteConnection! """ - Participants in the merge request + Participants in the merge request. This includes the author, assignees, reviewers, and users mentioned in notes. """ participants( """ @@ -13587,6 +13587,31 @@ type MergeRequest implements CurrentUserTodos & Noteable { ): String! """ + Users from whom a review has been requested. + """ + reviewers( + """ + Returns the elements in the list that come after the specified cursor. + """ + after: String + + """ + Returns the elements in the list that come before the specified cursor. + """ + before: String + + """ + Returns the first _n_ elements from the list. + """ + first: Int + + """ + Returns the last _n_ elements from the list. + """ + last: Int + ): UserConnection + + """ Indicates if the merge request is created by @GitLab-Security-Bot. """ securityAutoFix: Boolean diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index 48da77dfa14..3a83b0d0514 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -37252,7 +37252,7 @@ }, { "name": "participants", - "description": "Participants in the merge request", + "description": "Participants in the merge request. This includes the author, assignees, reviewers, and users mentioned in notes.", "args": [ { "name": "after", @@ -37482,6 +37482,59 @@ "deprecationReason": null }, { + "name": "reviewers", + "description": "Users from whom a review has been requested.", + "args": [ + { + "name": "after", + "description": "Returns the elements in the list that come after the specified cursor.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "before", + "description": "Returns the elements in the list that come before the specified cursor.", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "first", + "description": "Returns the first _n_ elements from the list.", + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "last", + "description": "Returns the last _n_ elements from the list.", + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "defaultValue": null + } + ], + "type": { + "kind": "OBJECT", + "name": "UserConnection", + "ofType": null + }, + "isDeprecated": false, + "deprecationReason": null + }, + { "name": "securityAutoFix", "description": "Indicates if the merge request is created by @GitLab-Security-Bot.", "args": [ diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index dade5be817d..4c7576696a1 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -2095,13 +2095,14 @@ Autogenerated return type of MarkAsSpamSnippet. | `mergedAt` | Time | Timestamp of when the merge request was merged, null if not merged | | `milestone` | Milestone | The milestone of the merge request | | `notes` | NoteConnection! | All notes on this noteable | -| `participants` | UserConnection | Participants in the merge request | +| `participants` | UserConnection | Participants in the merge request. This includes the author, assignees, reviewers, and users mentioned in notes. | | `pipelines` | PipelineConnection | Pipelines for the merge request. Note: for performance reasons, no more than the most recent 500 pipelines will be returned. | | `project` | Project! | Alias for target_project | | `projectId` | Int! | ID of the merge request project | | `rebaseCommitSha` | String | Rebase commit SHA of the merge request | | `rebaseInProgress` | Boolean! | Indicates if there is a rebase currently in progress for the merge request | | `reference` | String! | Internal reference of the merge request. Returned in shortened format by default | +| `reviewers` | UserConnection | Users from whom a review has been requested. | | `securityAutoFix` | Boolean | Indicates if the merge request is created by @GitLab-Security-Bot. | | `shouldBeRebased` | Boolean! | Indicates if the merge request will be rebased | | `shouldRemoveSourceBranch` | Boolean | Indicates if the source branch of the merge request will be deleted after merge | diff --git a/doc/user/project/issues/due_dates.md b/doc/user/project/issues/due_dates.md index 63cd784333a..34e9340067c 100644 --- a/doc/user/project/issues/due_dates.md +++ b/doc/user/project/issues/due_dates.md @@ -11,14 +11,14 @@ info: To determine the technical writer assigned to the Stage/Group associated w Please read through the [GitLab Issue Documentation](index.md) for an overview on GitLab Issues. Due dates can be used in issues to keep track of deadlines and make sure features are -shipped on time. Users must have at least [Reporter permissions](../../permissions.md) -to be able to edit them, but they can be seen by everybody with permission to view -the issue. +shipped on time. Users need at least [Reporter permissions](../../permissions.md) +to be able to edit the due date. All users with permission to view +the issue can view the due date. ## Setting a due date -When creating or editing an issue, you can click in the **due date** field and a calendar -will appear to help you choose the date you want. To remove the date, select the date +When creating an issue, select the **Due date** field to make a calendar +appear for choosing the date. To remove the date, select the date text and delete it. The date is related to the server's timezone, not the timezone of the user setting the due date. @@ -37,18 +37,17 @@ The last way to set a due date is by using [quick actions](../quick_actions.md), ## Making use of due dates -Issues that have a due date can be easily seen in the issue tracker, -displaying a date next to them. Issues where the date is overdue will have -the icon and the date colored red. You can sort issues by those that are -`Due soon` or `Due later` from the dropdown menu on the right. - - +You can see issues with their due dates in the [issues list](index.md#issues-list). +Overdue issues have their icon and date colored red. +To sort issues by their due dates, select **Due date** from the dropdown menu on the right. +Issues are then sorted from the earliest due date to the latest. +To display isses with the latest due dates at the top, select **Sort direction** (**{sort-lowest}**). Due dates also appear in your [to-do list](../../todos.md).  -The day before an open issue is due, an email will be sent to all participants +The day before an open issue is due, an email is sent to all participants of the issue. Like the due date, the "day before the due date" is determined by the server's timezone. diff --git a/doc/user/project/issues/img/due_dates_issues_index_page.png b/doc/user/project/issues/img/due_dates_issues_index_page.png Binary files differdeleted file mode 100644 index 94679436b32..00000000000 --- a/doc/user/project/issues/img/due_dates_issues_index_page.png +++ /dev/null diff --git a/lib/gitlab/usage_data_counters/hll_redis_counter.rb b/lib/gitlab/usage_data_counters/hll_redis_counter.rb index 573ad1dce35..b61720c7638 100644 --- a/lib/gitlab/usage_data_counters/hll_redis_counter.rb +++ b/lib/gitlab/usage_data_counters/hll_redis_counter.rb @@ -336,12 +336,10 @@ module Gitlab end def weekly_redis_keys(events:, start_date:, end_date:, context: '') - weeks = end_date.to_date.cweek - start_date.to_date.cweek - weeks = 1 if weeks == 0 - - (0..(weeks - 1)).map do |week_increment| - events.map { |event| redis_key(event, start_date + week_increment * 7.days, context) } - end.flatten + end_date = end_date.end_of_week - 1.week + (start_date.to_date..end_date.to_date).map do |date| + events.map { |event| redis_key(event, date, context) } + end.flatten.uniq end end end diff --git a/spec/finders/alert_management/alerts_finder_spec.rb b/spec/finders/alert_management/alerts_finder_spec.rb index 87a5da38dd1..3a88db5d854 100644 --- a/spec/finders/alert_management/alerts_finder_spec.rb +++ b/spec/finders/alert_management/alerts_finder_spec.rb @@ -42,6 +42,12 @@ RSpec.describe AlertManagement::AlertsFinder, '#execute' do it { is_expected.to contain_exactly(resolved_alert, ignored_alert) } end + + context 'skips domain if iid is given' do + let(:params) { { iid: resolved_alert.iid, domain: 'threat_monitoring' } } + + it { is_expected.to contain_exactly(resolved_alert) } + end end context 'empty params' do diff --git a/spec/frontend/alert_management/components/alert_details_spec.js b/spec/frontend/alert_management/components/alert_details_spec.js index e2d913398f9..976e50625a6 100644 --- a/spec/frontend/alert_management/components/alert_details_spec.js +++ b/spec/frontend/alert_management/components/alert_details_spec.js @@ -3,6 +3,7 @@ import { mount, shallowMount } from '@vue/test-utils'; import axios from 'axios'; import MockAdapter from 'axios-mock-adapter'; import { extendedWrapper } from 'helpers/vue_test_utils_helper'; +import waitForPromises from 'helpers/wait_for_promises'; import AlertDetails from '~/alert_management/components/alert_details.vue'; import AlertSummaryRow from '~/alert_management/components/alert_summary_row.vue'; import { @@ -201,6 +202,11 @@ describe('AlertDetails', () => { it('calls `$apollo.mutate` with `createIssueQuery`', () => { const issueIid = '10'; + mountComponent({ + mountMethod: mount, + data: { alert: { ...mockAlert } }, + }); + jest .spyOn(wrapper.vm.$apollo, 'mutate') .mockResolvedValue({ data: { createAlertIssue: { issue: { iid: issueIid } } } }); @@ -215,7 +221,7 @@ describe('AlertDetails', () => { }); }); - it('shows error alert when incident creation fails ', () => { + it('shows error alert when incident creation fails ', async () => { const errorMsg = 'Something went wrong'; mountComponent({ mountMethod: mount, @@ -225,9 +231,8 @@ describe('AlertDetails', () => { jest.spyOn(wrapper.vm.$apollo, 'mutate').mockRejectedValue(errorMsg); findCreateIncidentBtn().trigger('click'); - setImmediate(() => { - expect(findIncidentCreationAlert().text()).toBe(errorMsg); - }); + await waitForPromises(); + expect(findIncidentCreationAlert().text()).toBe(errorMsg); }); }); diff --git a/spec/frontend/clusters/components/uninstall_application_button_spec.js b/spec/frontend/clusters/components/uninstall_application_button_spec.js index 387e2188572..c106292965e 100644 --- a/spec/frontend/clusters/components/uninstall_application_button_spec.js +++ b/spec/frontend/clusters/components/uninstall_application_button_spec.js @@ -24,7 +24,7 @@ describe('UninstallApplicationButton', () => { ${UPDATING} | ${false} | ${true} | ${'Uninstall'} ${UNINSTALLING} | ${true} | ${true} | ${'Uninstalling'} `('when app status is $status', ({ loading, disabled, status, text }) => { - beforeAll(() => { + beforeEach(() => { createComponent({ status }); }); diff --git a/spec/frontend/design_management/components/toolbar/index_spec.js b/spec/frontend/design_management/components/toolbar/index_spec.js index 2914365b0df..6ac088a2c53 100644 --- a/spec/frontend/design_management/components/toolbar/index_spec.js +++ b/spec/frontend/design_management/components/toolbar/index_spec.js @@ -116,6 +116,8 @@ describe('Design management toolbar component', () => { }); it('renders download button with correct link', () => { + createComponent(); + expect(wrapper.find(GlButton).attributes('href')).toBe( '/-/designs/306/7f747adcd4693afadbe968d7ba7d983349b9012d', ); diff --git a/spec/frontend/logs/components/log_advanced_filters_spec.js b/spec/frontend/logs/components/log_advanced_filters_spec.js index 3a3c23c95b8..c865629ce7e 100644 --- a/spec/frontend/logs/components/log_advanced_filters_spec.js +++ b/spec/frontend/logs/components/log_advanced_filters_spec.js @@ -73,6 +73,8 @@ describe('LogAdvancedFilters', () => { }); it('displays search tokens', () => { + initWrapper(); + expect(getSearchToken(TOKEN_TYPE_POD_NAME)).toMatchObject({ title: 'Pod name', unique: true, diff --git a/spec/frontend/monitoring/components/alert_widget_form_spec.js b/spec/frontend/monitoring/components/alert_widget_form_spec.js index 6d71a9b09e5..6d87fb85f4d 100644 --- a/spec/frontend/monitoring/components/alert_widget_form_spec.js +++ b/spec/frontend/monitoring/components/alert_widget_form_spec.js @@ -76,11 +76,15 @@ describe('AlertWidgetForm', () => { }); it('shows correct title and button text', () => { + createComponent(); + expect(modalTitle()).toBe('Add alert'); expect(submitButton().text()).toBe('Add'); }); it('sets tracking options for create alert', () => { + createComponent(); + expect(submitButtonTrackingOpts()).toEqual(dataTrackingOptions.create); }); diff --git a/spec/frontend/operation_settings/components/metrics_settings_spec.js b/spec/frontend/operation_settings/components/metrics_settings_spec.js index c7ea23f9913..3216eece391 100644 --- a/spec/frontend/operation_settings/components/metrics_settings_spec.js +++ b/spec/frontend/operation_settings/components/metrics_settings_spec.js @@ -61,6 +61,7 @@ describe('operation settings external dashboard component', () => { describe('expand/collapse button', () => { it('renders as an expand button by default', () => { + mountComponent(); const button = wrapper.find(GlButton); expect(button.text()).toBe('Expand'); diff --git a/spec/frontend/vue_mr_widget/components/mr_widget_pipeline_container_spec.js b/spec/frontend/vue_mr_widget/components/mr_widget_pipeline_container_spec.js index d67f1adadf2..354adb8e776 100644 --- a/spec/frontend/vue_mr_widget/components/mr_widget_pipeline_container_spec.js +++ b/spec/frontend/vue_mr_widget/components/mr_widget_pipeline_container_spec.js @@ -94,6 +94,8 @@ describe('MrWidgetPipelineContainer', () => { describe('with artifacts path', () => { it('renders the artifacts app', () => { + factory(); + expect(wrapper.find(ArtifactsApp).isVisible()).toBe(true); }); }); diff --git a/spec/frontend/vue_mr_widget/components/states/mr_widget_commits_header_spec.js b/spec/frontend/vue_mr_widget/components/states/mr_widget_commits_header_spec.js index 62fc3330444..764d23e9066 100644 --- a/spec/frontend/vue_mr_widget/components/states/mr_widget_commits_header_spec.js +++ b/spec/frontend/vue_mr_widget/components/states/mr_widget_commits_header_spec.js @@ -98,6 +98,8 @@ describe('Commits header component', () => { }); it('does has merge commit part of the message', () => { + createComponent(); + expect(findHeaderWrapper().text()).toContain('1 merge commit'); }); }); diff --git a/spec/frontend/vue_shared/components/user_popover/user_popover_spec.js b/spec/frontend/vue_shared/components/user_popover/user_popover_spec.js index 7d58a865ba3..46ffa9044fb 100644 --- a/spec/frontend/vue_shared/components/user_popover/user_popover_spec.js +++ b/spec/frontend/vue_shared/components/user_popover/user_popover_spec.js @@ -81,6 +81,7 @@ describe('User Popover Component', () => { }); it('shows icon for location', () => { + createWrapper(); const iconEl = wrapper.find(GlIcon); expect(iconEl.props('name')).toEqual('location'); diff --git a/spec/graphql/types/merge_request_type_spec.rb b/spec/graphql/types/merge_request_type_spec.rb index 51e7b4029d5..eb3870cb192 100644 --- a/spec/graphql/types/merge_request_type_spec.rb +++ b/spec/graphql/types/merge_request_type_spec.rb @@ -25,7 +25,7 @@ RSpec.describe GitlabSchema.types['MergeRequest'] do merge_ongoing mergeable_discussions_state web_url source_branch_exists target_branch_exists upvotes downvotes head_pipeline pipelines task_completion_status - milestone assignees participants subscribed labels discussion_locked time_estimate + milestone assignees reviewers participants subscribed labels discussion_locked time_estimate total_time_spent reference author merged_at commit_count current_user_todos conflicts auto_merge_enabled approved_by source_branch_protected default_merge_commit_message_with_description squash_on_merge available_auto_merge_strategies diff --git a/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb b/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb index 70ee9871486..b6a60c09d3d 100644 --- a/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb +++ b/spec/lib/gitlab/usage_data_counters/hll_redis_counter_spec.rb @@ -283,6 +283,50 @@ RSpec.describe Gitlab::UsageDataCounters::HLLRedisCounter, :clean_gitlab_redis_s context 'when no slot is set' do it { expect(described_class.unique_events(event_names: [no_slot], start_date: 7.days.ago, end_date: Date.current)).to eq(1) } end + + context 'when data crosses into new year' do + it 'does not raise error' do + expect { described_class.unique_events(event_names: [weekly_event], start_date: DateTime.parse('2020-12-26'), end_date: DateTime.parse('2021-02-01')) } + .not_to raise_error + end + end + end + end + + describe '.weekly_redis_keys' do + using RSpec::Parameterized::TableSyntax + + let(:weekly_event) { 'g_compliance_dashboard' } + let(:redis_event) { described_class.send(:event_for, weekly_event) } + + subject(:weekly_redis_keys) { described_class.send(:weekly_redis_keys, events: [redis_event], start_date: DateTime.parse(start_date), end_date: DateTime.parse(end_date)) } + + where(:start_date, :end_date, :keys) do + '2020-12-21' | '2020-12-21' | [] + '2020-12-21' | '2020-12-20' | [] + '2020-12-21' | '2020-11-21' | [] + '2021-01-01' | '2020-12-28' | [] + '2020-12-21' | '2020-12-28' | ['g_{compliance}_dashboard-2020-52'] + '2020-12-21' | '2021-01-01' | ['g_{compliance}_dashboard-2020-52'] + '2020-12-27' | '2021-01-01' | ['g_{compliance}_dashboard-2020-52'] + '2020-12-26' | '2021-01-04' | ['g_{compliance}_dashboard-2020-52', 'g_{compliance}_dashboard-2020-53'] + '2020-12-26' | '2021-01-11' | ['g_{compliance}_dashboard-2020-52', 'g_{compliance}_dashboard-2020-53', 'g_{compliance}_dashboard-2021-01'] + '2020-12-26' | '2021-01-17' | ['g_{compliance}_dashboard-2020-52', 'g_{compliance}_dashboard-2020-53', 'g_{compliance}_dashboard-2021-01'] + '2020-12-26' | '2021-01-18' | ['g_{compliance}_dashboard-2020-52', 'g_{compliance}_dashboard-2020-53', 'g_{compliance}_dashboard-2021-01', 'g_{compliance}_dashboard-2021-02'] + end + + with_them do + it "returns the correct keys" do + expect(subject).to match(keys) + end + end + + it 'returns 1 key for last for week' do + expect(described_class.send(:weekly_redis_keys, events: [redis_event], start_date: 7.days.ago.to_date, end_date: Date.current).size).to eq 1 + end + + it 'returns 4 key for last for weeks' do + expect(described_class.send(:weekly_redis_keys, events: [redis_event], start_date: 4.weeks.ago.to_date, end_date: Date.current).size).to eq 4 end end diff --git a/spec/lib/gitlab/usage_data_spec.rb b/spec/lib/gitlab/usage_data_spec.rb index c2d96369425..4d12bb6bd8c 100644 --- a/spec/lib/gitlab/usage_data_spec.rb +++ b/spec/lib/gitlab/usage_data_spec.rb @@ -54,6 +54,32 @@ RSpec.describe Gitlab::UsageData, :aggregate_failures do expect { subject }.to raise_error('Stopped calculating recorded_at') end + + context 'when generating usage ping in critical weeks' do + it 'does not raise error when generated in last week of the year' do + travel_to(DateTime.parse('2020-12-29')) do + expect { subject }.not_to raise_error + end + end + + it 'does not raise error when generated in first week of the year' do + travel_to(DateTime.parse('2021-01-01')) do + expect { subject }.not_to raise_error + end + end + + it 'does not raise error when generated in second week of the year' do + travel_to(DateTime.parse('2021-01-07')) do + expect { subject }.not_to raise_error + end + end + + it 'does not raise error when generated in 3rd week of the year' do + travel_to(DateTime.parse('2021-01-14')) do + expect { subject }.not_to raise_error + end + end + end end describe 'usage_activity_by_stage_package' do diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index 9824eb91bc7..e17ea1f5e59 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -61,6 +61,22 @@ RSpec.describe CommitStatus do expect(commit_status.started_at).to be_present end end + + describe 'transitioning to created from skipped or manual' do + let(:commit_status) { create(:commit_status, :skipped) } + + it 'does not update user without parameter' do + commit_status.process! + + expect { commit_status.process }.not_to change { commit_status.reload.user } + end + + it 'updates user with user parameter' do + new_user = create(:user) + + expect { commit_status.process(new_user) }.to change { commit_status.reload.user }.to(new_user) + end + end end describe '#processed' do diff --git a/spec/requests/api/graphql/project/merge_request_spec.rb b/spec/requests/api/graphql/project/merge_request_spec.rb index fae52fe814d..d711376024a 100644 --- a/spec/requests/api/graphql/project/merge_request_spec.rb +++ b/spec/requests/api/graphql/project/merge_request_spec.rb @@ -9,12 +9,13 @@ RSpec.describe 'getting merge request information nested in a project' do let(:current_user) { create(:user) } let(:merge_request_graphql_data) { graphql_data['project']['mergeRequest'] } let!(:merge_request) { create(:merge_request, source_project: project) } + let(:mr_fields) { all_graphql_fields_for('MergeRequest') } let(:query) do graphql_query_for( 'project', { 'fullPath' => project.full_path }, - query_graphql_field('mergeRequest', iid: merge_request.iid.to_s) + query_graphql_field('mergeRequest', { iid: merge_request.iid.to_s }, mr_fields) ) end @@ -43,6 +44,38 @@ RSpec.describe 'getting merge request information nested in a project' do expect(merge_request_graphql_data['author']['username']).to eq(merge_request.author.username) end + context 'the merge_request has reviewers' do + let(:mr_fields) do + <<~SELECT + reviewers { nodes { id username } } + participants { nodes { id username } } + SELECT + end + + before do + merge_request.reviewers << create_list(:user, 2) + end + + it 'includes reviewers' do + expected = merge_request.reviewers.map do |r| + a_hash_including('id' => global_id_of(r), 'username' => r.username) + end + + post_graphql(query, current_user: current_user) + + expect(graphql_data_at(:project, :merge_request, :reviewers, :nodes)).to match_array(expected) + expect(graphql_data_at(:project, :merge_request, :participants, :nodes)).to include(*expected) + end + + it 'suppresses reviewers if reviewers are not allowed' do + stub_feature_flags(merge_request_reviewers: false) + + post_graphql(query, current_user: current_user) + + expect(graphql_data_at(:project, :merge_request, :reviewers)).to be_nil + end + end + it 'includes diff stats' do be_natural = an_instance_of(Integer).and(be >= 0) diff --git a/spec/requests/api/graphql/project/merge_requests_spec.rb b/spec/requests/api/graphql/project/merge_requests_spec.rb index c05a620bb62..c85cb8b2ffe 100644 --- a/spec/requests/api/graphql/project/merge_requests_spec.rb +++ b/spec/requests/api/graphql/project/merge_requests_spec.rb @@ -23,9 +23,7 @@ RSpec.describe 'getting merge request listings nested in a project' do graphql_query_for( :project, { full_path: project.full_path }, - query_graphql_field(:merge_requests, search_params, [ - query_graphql_field(:nodes, nil, fields) - ]) + query_nodes(:merge_requests, fields, args: search_params) ) end @@ -50,24 +48,22 @@ RSpec.describe 'getting merge request listings nested in a project' do project.repository.expire_branches_cache end + let(:graphql_data) do + GitlabSchema.execute(query, context: { current_user: current_user }).to_h['data'] + end + context 'selecting any single scalar field' do where(:field) do scalar_fields_of('MergeRequest').map { |name| [name] } end with_them do - it_behaves_like 'a working graphql query' do - let(:query) do - query_merge_requests([:iid, field].uniq) - end - - before do - post_graphql(query, current_user: current_user) - end - - it 'selects the correct MR' do - expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s)) - end + let(:query) do + query_merge_requests([:iid, field].uniq) + end + + it 'selects the correct MR' do + expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s)) end end end @@ -87,19 +83,13 @@ RSpec.describe 'getting merge request listings nested in a project' do end with_them do - it_behaves_like 'a working graphql query' do - let(:query) do - fld = is_connection ? query_graphql_field(:nodes, nil, [subfield]) : subfield - query_merge_requests([:iid, query_graphql_field(field, nil, [fld])]) - end - - before do - post_graphql(query, current_user: current_user) - end - - it 'selects the correct MR' do - expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s)) - end + let(:query) do + fld = is_connection ? query_graphql_field(:nodes, nil, [subfield]) : subfield + query_merge_requests([:iid, query_graphql_field(field, nil, [fld])]) + end + + it 'selects the correct MR' do + expect(results).to contain_exactly(a_hash_including('iid' => fresh_mr.iid.to_s)) end end end @@ -254,6 +244,115 @@ RSpec.describe 'getting merge request listings nested in a project' do include_examples 'N+1 query check' end + + context 'when requesting reviewers' do + let(:requested_fields) { ['reviewers { nodes { username } }'] } + + before do + merge_request_a.reviewers << create(:user) + merge_request_a.reviewers << create(:user) + merge_request_c.reviewers << create(:user) + end + + it 'returns the reviewers' do + execute_query + + expect(results).to include a_hash_including('reviewers' => { + 'nodes' => match_array(merge_request_a.reviewers.map do |r| + a_hash_including('username' => r.username) + end) + }) + end + + context 'the feature flag is disabled' do + before do + stub_feature_flags(merge_request_reviewers: false) + end + + it 'does not return reviewers' do + execute_query + + expect(results).to all(match a_hash_including('reviewers' => be_nil)) + end + end + + include_examples 'N+1 query check' + end + end + + describe 'performance' do + let(:mr_fields) do + <<~SELECT + assignees { nodes { username } } + reviewers { nodes { username } } + participants { nodes { username } } + headPipeline { status } + SELECT + end + + let(:query) do + <<~GQL + query($first: Int) { + project(fullPath: "#{project.full_path}") { + mergeRequests(first: $first) { + nodes { #{mr_fields} } + } + } + } + GQL + end + + before_all do + project.add_developer(current_user) + mrs = create_list(:merge_request, 10, :closed, :with_head_pipeline, + source_project: project, + author: current_user) + mrs.each do |mr| + mr.assignees << create(:user) + mr.assignees << current_user + mr.reviewers << create(:user) + mr.reviewers << current_user + end + end + + before do + # Confounding factor: makes DB calls in EE + allow(Gitlab::Database).to receive(:read_only?).and_return(false) + end + + def run_query(number) + # Ensure that we have a fresh request store and batch-context between runs + result = run_with_clean_state(query, + context: { current_user: current_user }, + variables: { first: number } + ) + + graphql_dig_at(result.to_h, :data, :project, :merge_requests, :nodes) + end + + def user_collection + { 'nodes' => all(match(a_hash_including('username' => be_present))) } + end + + it 'returns appropriate results' do + mrs = run_query(2) + + expect(mrs.size).to eq(2) + expect(mrs).to all( + match( + a_hash_including( + 'assignees' => user_collection, + 'reviewers' => user_collection, + 'participants' => user_collection, + 'headPipeline' => { 'status' => be_present } + ))) + end + + it 'can lookahead to eliminate N+1 queries' do + baseline = ActiveRecord::QueryRecorder.new { run_query(1) } + + expect { run_query(10) }.not_to exceed_query_limit(baseline) + end end describe 'sorting and pagination' do diff --git a/spec/requests/api/graphql/project_query_spec.rb b/spec/requests/api/graphql/project_query_spec.rb index b29f9ae913f..2cdd7273b18 100644 --- a/spec/requests/api/graphql/project_query_spec.rb +++ b/spec/requests/api/graphql/project_query_spec.rb @@ -6,26 +6,21 @@ RSpec.describe 'getting project information' do include GraphqlHelpers let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, :repository, group: group) } + let_it_be(:project, reload: true) { create(:project, :repository, group: group) } let_it_be(:current_user) { create(:user) } - let(:fields) { all_graphql_fields_for(Project, max_depth: 2, excluded: %w(jiraImports services)) } + let(:project_fields) { all_graphql_fields_for('project'.to_s.classify, max_depth: 1) } let(:query) do - graphql_query_for(:project, { full_path: project.full_path }, fields) + graphql_query_for(:project, { full_path: project.full_path }, project_fields) end context 'when the user has full access to the project' do - let(:full_access_query) do - graphql_query_for(:project, { full_path: project.full_path }, - all_graphql_fields_for('Project', max_depth: 2)) - end - before do project.add_maintainer(current_user) end it 'includes the project', :use_clean_rails_memory_store_caching, :request_store do - post_graphql(full_access_query, current_user: current_user) + post_graphql(query, current_user: current_user) expect(graphql_data['project']).not_to be_nil end @@ -49,12 +44,12 @@ RSpec.describe 'getting project information' do end context 'when there are pipelines present' do + let(:project_fields) { query_nodes(:pipelines) } + before do create(:ci_pipeline, project: project) end - let(:fields) { query_nodes(:pipelines) } - it 'is included in the pipelines connection' do post_graphql(query, current_user: current_user) @@ -108,55 +103,6 @@ RSpec.describe 'getting project information' do end end - describe 'performance' do - before_all do - project.add_developer(current_user) - mrs = create_list(:merge_request, 10, :closed, :with_head_pipeline, - source_project: project, - author: current_user) - mrs.each do |mr| - mr.assignees << create(:user) - mr.assignees << current_user - end - end - - def run_query(number) - q = <<~GQL - query { - project(fullPath: "#{project.full_path}") { - mergeRequests(first: #{number}) { - nodes { - assignees { nodes { username } } - headPipeline { status } - } - } - } - } - GQL - - post_graphql(q, current_user: current_user) - end - - it 'returns appropriate results' do - run_query(2) - - mrs = graphql_data.dig('project', 'mergeRequests', 'nodes') - - expect(mrs.size).to eq(2) - expect(mrs).to all( - match( - a_hash_including( - 'assignees' => { 'nodes' => all(match(a_hash_including('username' => be_present))) }, - 'headPipeline' => { 'status' => be_present } - ))) - end - - it 'can lookahead to eliminate N+1 queries' do - baseline = ActiveRecord::QueryRecorder.new { run_query(1) } - expect { run_query(10) }.not_to exceed_query_limit(baseline) - end - end - context 'when the user does not have access to the project' do it 'returns an empty field' do post_graphql(query, current_user: current_user) diff --git a/spec/services/ci/build_report_result_service_spec.rb b/spec/services/ci/build_report_result_service_spec.rb index 244ffbf4bbd..7c2702af086 100644 --- a/spec/services/ci/build_report_result_service_spec.rb +++ b/spec/services/ci/build_report_result_service_spec.rb @@ -6,12 +6,6 @@ RSpec.describe Ci::BuildReportResultService do describe '#execute', :clean_gitlab_redis_shared_state do subject(:build_report_result) { described_class.new.execute(build) } - around do |example| - travel_to(DateTime.parse('2020-07-01')) do - example.run - end - end - context 'when build is finished' do let(:build) { create(:ci_build, :success, :test_reports) } diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 81d56a0e42a..bdf60bb3fdc 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -206,6 +206,22 @@ RSpec.describe Ci::RetryBuildService do expect(subsequent_build.reload).to be_created expect(subsequent_bridge.reload).to be_created end + + it 'updates ownership for subsequent builds' do + expect { service.execute(build) }.to change { subsequent_build.reload.user }.to(user) + end + + it 'updates ownership for subsequent bridges' do + expect { service.execute(build) }.to change { subsequent_bridge.reload.user }.to(user) + end + + it 'does not cause n+1 when updaing build ownership' do + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { service.execute(build) }.count + + create_list(:ci_build, 2, :skipped, stage_idx: build.stage_idx + 1, pipeline: pipeline, stage: 'deploy') + + expect { service.execute(build) }.not_to exceed_all_query_limit(control_count) + end end context 'when pipeline has other builds' do diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb index 526c2f39b46..3c6a99efbf8 100644 --- a/spec/services/ci/retry_pipeline_service_spec.rb +++ b/spec/services/ci/retry_pipeline_service_spec.rb @@ -64,6 +64,18 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do expect(build('spinach 1')).to be_created expect(pipeline.reload).to be_running end + + it 'changes ownership of subsequent builds' do + expect(build('rspec 2').user).not_to eq(user) + expect(build('rspec 3').user).not_to eq(user) + expect(build('spinach 1').user).not_to eq(user) + + service.execute(pipeline) + + expect(build('rspec 2').user).to eq(user) + expect(build('rspec 3').user).to eq(user) + expect(build('spinach 1').user).to eq(user) + end end context 'when there is failed build present which was run on failure' do @@ -161,6 +173,16 @@ RSpec.describe Ci::RetryPipelineService, '#execute' do expect(build('rspec 2')).to be_created expect(pipeline.reload).to be_running end + + it 'changes ownership of subsequent builds' do + expect(build('staging').user).not_to eq(user) + expect(build('rspec 2').user).not_to eq(user) + + service.execute(pipeline) + + expect(build('staging').user).to eq(user) + expect(build('rspec 2').user).to eq(user) + end end end diff --git a/spec/support/helpers/graphql_helpers.rb b/spec/support/helpers/graphql_helpers.rb index b20801bd3c4..35c298a4d48 100644 --- a/spec/support/helpers/graphql_helpers.rb +++ b/spec/support/helpers/graphql_helpers.rb @@ -67,14 +67,16 @@ module GraphqlHelpers end end + def with_clean_batchloader_executor(&block) + BatchLoader::Executor.ensure_current + yield + ensure + BatchLoader::Executor.clear_current + end + # Runs a block inside a BatchLoader::Executor wrapper def batch(max_queries: nil, &blk) - wrapper = proc do - BatchLoader::Executor.ensure_current - yield - ensure - BatchLoader::Executor.clear_current - end + wrapper = -> { with_clean_batchloader_executor(&blk) } if max_queries result = nil @@ -85,6 +87,32 @@ module GraphqlHelpers end end + # Use this when writing N+1 tests. + # + # It does not use the controller, so it avoids confounding factors due to + # authentication (token set-up, license checks) + # It clears the request store, rails cache, and BatchLoader Executor between runs. + def run_with_clean_state(query, **args) + ::Gitlab::WithRequestStore.with_request_store do + with_clean_rails_cache do + with_clean_batchloader_executor do + ::GitlabSchema.execute(query, **args) + end + end + end + end + + # Basically a combination of use_sql_query_cache and use_clean_rails_memory_store_caching, + # but more fine-grained, suitable for comparing two runs in the same example. + def with_clean_rails_cache(&blk) + caching_store = Rails.cache + Rails.cache = ActiveSupport::Cache::MemoryStore.new + + ActiveRecord::Base.cache(&blk) + ensure + Rails.cache = caching_store + end + # BatchLoader::GraphQL returns a wrapper, so we need to :sync in order # to get the actual values def batch_sync(max_queries: nil, &blk) @@ -245,7 +273,7 @@ module GraphqlHelpers return if max_depth <= 0 allow_unlimited_graphql_complexity - allow_unlimited_graphql_depth + allow_unlimited_graphql_depth if max_depth > 1 allow_high_graphql_recursion allow_high_graphql_transaction_threshold diff --git a/spec/support/shared_examples/graphql/projects/merge_request_n_plus_one_query_examples.rb b/spec/support/shared_examples/graphql/projects/merge_request_n_plus_one_query_examples.rb index 397e22ace28..738edd43c92 100644 --- a/spec/support/shared_examples/graphql/projects/merge_request_n_plus_one_query_examples.rb +++ b/spec/support/shared_examples/graphql/projects/merge_request_n_plus_one_query_examples.rb @@ -2,10 +2,12 @@ shared_examples 'N+1 query check' do it 'prevents N+1 queries' do execute_query # "warm up" to prevent undeterministic counts + expect(graphql_errors).to be_blank # Sanity check - ex falso quodlibet! - control_count = ActiveRecord::QueryRecorder.new { execute_query }.count + control = ActiveRecord::QueryRecorder.new { execute_query } + expect(control.count).to be > 0 search_params[:iids] << extra_iid_for_second_query - expect { execute_query }.not_to exceed_query_limit(control_count) + expect { execute_query }.not_to exceed_query_limit(control) end end diff --git a/spec/support/shared_examples/lib/gitlab/usage_data_counters/incident_management_activity_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/usage_data_counters/incident_management_activity_shared_examples.rb index 4e35e388b23..788c35dd5bf 100644 --- a/spec/support/shared_examples/lib/gitlab/usage_data_counters/incident_management_activity_shared_examples.rb +++ b/spec/support/shared_examples/lib/gitlab/usage_data_counters/incident_management_activity_shared_examples.rb @@ -3,8 +3,8 @@ RSpec.shared_examples 'an incident management tracked event' do |event| describe ".track_event", :clean_gitlab_redis_shared_state do let(:counter) { Gitlab::UsageDataCounters::HLLRedisCounter } - let(:start_time) { 1.minute.ago } - let(:end_time) { 1.minute.from_now } + let(:start_time) { 1.week.ago } + let(:end_time) { 1.week.from_now } it "tracks the event using redis" do # Allow other subsequent calls |
