diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index 8a782ec2b68..fc68b8aa354 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -1263,16 +1263,12 @@ .rails:rules:detect-previous-failed-tests: rules: - - <<: *if-not-canonical-namespace - when: never - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *code-backstage-patterns .rails:rules:rerun-previous-failed-tests: rules: - - <<: *if-not-canonical-namespace - when: never - <<: *if-merge-request-labels-run-all-rspec - <<: *if-merge-request changes: *code-backstage-patterns diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index eaab807cd88..af2714fd660 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -39,7 +39,6 @@ Rails/SaveBang: - 'ee/spec/models/license_spec.rb' - 'ee/spec/models/merge_request_spec.rb' - 'ee/spec/models/merge_train_spec.rb' - - 'spec/models/packages/package_spec.rb' - 'ee/spec/models/project_ci_cd_setting_spec.rb' - 'ee/spec/models/project_spec.rb' - 'ee/spec/models/protected_environment_spec.rb' @@ -135,32 +134,6 @@ Rails/SaveBang: - 'spec/lib/gitlab/middleware/go_spec.rb' - 'spec/lib/gitlab/shard_health_cache_spec.rb' - 'spec/mailers/notify_spec.rb' - - 'spec/models/clusters/applications/helm_spec.rb' - - 'spec/models/design_management/version_spec.rb' - - 'spec/models/environment_spec.rb' - - 'spec/models/event_spec.rb' - - 'spec/models/fork_network_spec.rb' - - 'spec/models/generic_commit_status_spec.rb' - - 'spec/models/grafana_integration_spec.rb' - - 'spec/models/group_spec.rb' - - 'spec/models/identity_spec.rb' - - 'spec/models/jira_import_state_spec.rb' - - 'spec/models/namespace_spec.rb' - - 'spec/models/note_spec.rb' - - 'spec/models/notification_setting_spec.rb' - - 'spec/models/operations/feature_flag_scope_spec.rb' - - 'spec/models/operations/feature_flags/strategy_spec.rb' - - 'spec/models/operations/feature_flags/user_list_spec.rb' - - 'spec/models/pages_domain_spec.rb' - - 'spec/models/protectable_dropdown_spec.rb' - - 'spec/models/redirect_route_spec.rb' - - 'spec/models/release_spec.rb' - - 'spec/models/remote_mirror_spec.rb' - - 'spec/models/resource_milestone_event_spec.rb' - - 'spec/models/route_spec.rb' - - 'spec/models/sentry_issue_spec.rb' - - 'spec/models/snippet_spec.rb' - - 'spec/models/upload_spec.rb' Rails/TimeZone: Enabled: true diff --git a/CHANGELOG.md b/CHANGELOG.md index fd895fc2220..52d66b1ab22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3064,6 +3064,10 @@ No changes. - [Add missing metrics information](gitlab-org/gitlab@89cd7fe3b95323e635b2d73e08549b2e6153dc4d) ([merge request](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61772/edit)) - [Track usage of the resolve UI](gitlab-org/gitlab@35c8e30fce288cecefcf2f7c0077d4608e696519) ([merge request](gitlab-org/gitlab!61654)) +## 13.12.15 (2021-11-03) + +No changes. + ## 13.12.14 (2021-11-03) ### Fixed (2 changes) diff --git a/app/assets/javascripts/environments/components/new_environment_folder.vue b/app/assets/javascripts/environments/components/new_environment_folder.vue new file mode 100644 index 00000000000..0615bdef537 --- /dev/null +++ b/app/assets/javascripts/environments/components/new_environment_folder.vue @@ -0,0 +1,69 @@ + + diff --git a/app/assets/javascripts/environments/components/new_environments_app.vue b/app/assets/javascripts/environments/components/new_environments_app.vue index 11fa0e24572..a5526f9cd71 100644 --- a/app/assets/javascripts/environments/components/new_environments_app.vue +++ b/app/assets/javascripts/environments/components/new_environments_app.vue @@ -1,6 +1,47 @@ diff --git a/app/assets/javascripts/ide/components/pipelines/list.vue b/app/assets/javascripts/ide/components/pipelines/list.vue index 907ac496982..e1caf1ba44a 100644 --- a/app/assets/javascripts/ide/components/pipelines/list.vue +++ b/app/assets/javascripts/ide/components/pipelines/list.vue @@ -87,7 +87,7 @@ export default { v-if="!latestPipeline" :empty-state-svg-path="pipelinesEmptyStateSvgPath" :can-set-ci="true" - class="mb-auto mt-auto" + class="gl-p-5" />
-
+
diff --git a/doc/raketasks/import.md b/doc/raketasks/import.md index 7f817f9c422..45d51fe9dfa 100644 --- a/doc/raketasks/import.md +++ b/doc/raketasks/import.md @@ -58,7 +58,7 @@ To import bare repositories into a GitLab instance: - Omnibus Installation ```shell - sudo gitlab-rake gitlab:import:repos['/var/opt/gitlab/git-data/repository-import-$(date "+%Y-%m-%d")'] + sudo gitlab-rake gitlab:import:repos["/var/opt/gitlab/git-data/repository-import-$(date "+%Y-%m-%d")"] ``` - Installation from source. Before running this command you need to change to the directory where @@ -66,7 +66,7 @@ To import bare repositories into a GitLab instance: ```shell cd /home/git/gitlab - sudo -u git -H bundle exec rake gitlab:import:repos['/var/opt/gitlab/git-data/repository-import-$(date "+%Y-%m-%d")'] RAILS_ENV=production + sudo -u git -H bundle exec rake gitlab:import:repos["/var/opt/gitlab/git-data/repository-import-$(date "+%Y-%m-%d")"] RAILS_ENV=production ``` ## Example output diff --git a/doc/user/admin_area/merge_requests_approvals.md b/doc/user/admin_area/merge_requests_approvals.md index 9a96189d05e..0ecf76902e1 100644 --- a/doc/user/admin_area/merge_requests_approvals.md +++ b/doc/user/admin_area/merge_requests_approvals.md @@ -9,9 +9,13 @@ type: reference, concepts > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/39060) in GitLab 12.8. -Merge request approval rules prevent users from overriding certain settings on the project -level. When enabled at the instance level, these settings are no longer editable on the -project level. +Merge request approval rules prevent users from overriding certain settings on the project level. +When enabled at the instance level, these settings [cascade](../project/merge_requests/approvals/settings.md#settings-cascading) +and can no longer be changed: + +- In projects. +- In groups. Cascading to groups was [enabled by default](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) + in GitLab 14.5. To enable merge request approval settings for an instance: @@ -24,15 +28,14 @@ To enable merge request approval settings for an instance: Merge request approval settings that can be set at an instance level are: -- **Prevent approval by author**. Prevents project -maintainers from allowing request authors to merge their own merge requests. -- **Prevent approvals by users who add commits**. Prevents project -maintainers from allowing users to approve merge requests if they have submitted -any commits to the source branch. -- **Prevent editing approval rules in projects and merge requests**. Prevents users from -modifying the approvers list in project settings or in individual merge requests. +- **Prevent approval by author**. Prevents project maintainers from allowing request authors to + merge their own merge requests. +- **Prevent approvals by users who add commits**. Prevents project maintainers from allowing users + to approve merge requests if they have submitted any commits to the source branch. +- **Prevent editing approval rules in projects and merge requests**. Prevents users from modifying + the approvers list in project settings or in individual merge requests. See also the following, which are affected by instance-level rules: -- [Project-level merge request approval rules](../project/merge_requests/approvals/index.md). -- [Group-level merge request approval rules](../group/index.md#group-approval-rules) available in GitLab 13.9 and later. +- [Project merge request approval rules](../project/merge_requests/approvals/index.md). +- [Group merge request approval rules](../group/index.md#group-approval-rules) available in GitLab 13.9 and later. diff --git a/doc/user/group/import/index.md b/doc/user/group/import/index.md index 3d91fe44cf8..98057de7d37 100644 --- a/doc/user/group/import/index.md +++ b/doc/user/group/import/index.md @@ -7,9 +7,8 @@ info: To determine the technical writer assigned to the Stage/Group associated w # Migrate groups from another instance of GitLab **(FREE)** -> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/249160) in GitLab 13.7. -> - [Deployed behind a feature flag](../../feature_flags.md), disabled by default. -> - Enabled on GitLab.com. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/249160) in GitLab 13.7 [with a flag](../../feature_flags.md) named `bulk_import`. Disabled by default. +> - [Enabled on GitLab.com and self-managed](https://gitlab.com/gitlab-org/gitlab/-/issues/338985) in GitLab 14.3. NOTE: The importer migrates **only** the group data listed on this page. To leave feedback on this @@ -77,14 +76,9 @@ Any other items are **not** migrated. ## Enable or disable GitLab Group Migration -GitLab Migration is deployed behind a feature flag that is **enabled by default**. -[GitLab administrators with access to the GitLab Rails console](../../../administration/feature_flags.md) can enable it. - -To enable it: - -```ruby -Feature.enable(:bulk_import) -``` +GitLab Migration is deployed behind the `bulk_import` feature flag, which is **enabled by default**. +[GitLab administrators with access to the GitLab Rails console](../../../administration/feature_flags.md) +can disable it. To disable it: @@ -92,6 +86,12 @@ To disable it: Feature.disable(:bulk_import) ``` +To enable it: + +```ruby +Feature.enable(:bulk_import) +``` + ## Import your groups into GitLab Before you begin, ensure that the target instance of GitLab can communicate with the source diff --git a/doc/user/group/index.md b/doc/user/group/index.md index ca33a2fd02a..f0e08301a1b 100644 --- a/doc/user/group/index.md +++ b/doc/user/group/index.md @@ -742,12 +742,11 @@ The group's new subgroups have push rules set for them based on either: FLAG: On self-managed GitLab, by default this feature is available. To hide the feature per group, ask an administrator to [disable the feature flag](../../administration/feature_flags.md) named `group_merge_request_approval_settings_feature_flag`. On GitLab.com, this feature is available. -Group approval rules provides an interface for managing -[project merge request approval rules](../project/merge_requests/approvals/index.md) at the -top-level group level. When rules are configured [at the instance level](../admin_area/merge_requests_approvals.md), -you can't edit locked rules. +Group approval rules manage [project merge request approval rules](../project/merge_requests/approvals/index.md) +at the top-level group level. These rules [cascade to all projects](../project/merge_requests/approvals/settings.md#settings-cascading) +that belong to the group. -To view the merge request approval rules UI for a group: +To view the merge request approval rules for a group: 1. Go to the top-level group's **Settings > General** page. 1. Expand the **Merge request approvals** section. diff --git a/doc/user/project/merge_requests/approvals/settings.md b/doc/user/project/merge_requests/approvals/settings.md index dab95212110..56e93741c1a 100644 --- a/doc/user/project/merge_requests/approvals/settings.md +++ b/doc/user/project/merge_requests/approvals/settings.md @@ -139,7 +139,7 @@ coverage. To learn more, see [Coverage check approval rule](../../../../ci/pipelines/settings.md#coverage-check-approval-rule). -## Merge request approval settings cascading +## Settings cascading > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) in GitLab 14.4. [Deployed behind the `group_merge_request_approval_settings_feature_flag` flag](../../../../administration/feature_flags.md), disabled by default. > - [Enabled by default](https://gitlab.com/gitlab-org/gitlab/-/issues/285410) in GitLab 14.5. @@ -149,11 +149,13 @@ On self-managed GitLab, by default this feature is available. To hide the featur You can also enforce merge request approval settings: -- At the [instance level](../../../admin_area/merge_requests_approvals.md), which apply to all groups on an instance and, therefore, all - projects. -- On a [top-level group](../../../group/index.md#group-approval-rules), which apply to all subgroups and projects. +- At the [instance level](../../../admin_area/merge_requests_approvals.md), which apply to all groups + on an instance and, therefore, all projects. +- On a [top-level group](../../../group/index.md#group-approval-rules), which apply to all subgroups + and projects. -If the settings are inherited by a group or project, they cannot be overridden by the group or project that inherited them. +If the settings are inherited by a group or project, they cannot be changed in the group or project +that inherited them. ## Related links diff --git a/scripts/pipeline_test_report_builder.rb b/scripts/pipeline_test_report_builder.rb index 56491d40a3e..2101decf59a 100755 --- a/scripts/pipeline_test_report_builder.rb +++ b/scripts/pipeline_test_report_builder.rb @@ -20,7 +20,7 @@ require_relative 'api/default_options' # Push into expected format for failed tests class PipelineTestReportBuilder def initialize(options) - @project = options.delete(:project) + @target_project = options.delete(:target_project) @mr_id = options.delete(:mr_id) || Host::DEFAULT_OPTIONS[:mr_id] @instance_base_url = options.delete(:instance_base_url) || Host::DEFAULT_OPTIONS[:instance_base_url] @output_file_path = options.delete(:output_file_path) @@ -40,38 +40,38 @@ class PipelineTestReportBuilder end end - private - - attr_reader :project, :mr_id, :instance_base_url, :output_file_path - - def project_api_base_url - "#{instance_base_url}/api/v4/projects/#{CGI.escape(project)}" - end - - def project_base_url - "#{instance_base_url}/#{project}" - end - def previous_pipeline # Top of the list will always be the current pipeline # Second from top will be the previous pipeline pipelines_for_mr.sort_by { |a| -Time.parse(a['created_at']).to_i }[1] end - def pipelines_for_mr - fetch("#{project_api_base_url}/merge_requests/#{mr_id}/pipelines") + private + + attr_reader :target_project, :mr_id, :instance_base_url, :output_file_path + + def pipeline_project_api_base_url(pipeline) + "#{instance_base_url}/api/v4/projects/#{pipeline['project_id']}" end - def failed_builds_for_pipeline(pipeline_id) - fetch("#{project_api_base_url}/pipelines/#{pipeline_id}/jobs?scope=failed&per_page=100") + def target_project_api_base_url + "#{instance_base_url}/api/v4/projects/#{CGI.escape(target_project)}" + end + + def pipelines_for_mr + fetch("#{target_project_api_base_url}/merge_requests/#{mr_id}/pipelines") + end + + def failed_builds_for_pipeline(pipeline) + fetch("#{pipeline_project_api_base_url(pipeline)}/pipelines/#{pipeline['id']}/jobs?scope=failed&per_page=100") end # Method uses the test suite endpoint to gather test results for a particular build. # Here we request individual builds, even though it is possible to supply multiple build IDs. # The reason for this; it is possible to lose the job context and name when requesting multiple builds. # Please see for more info: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/69053#note_709939709 - def test_report_for_build(pipeline_id, build_id) - fetch("#{project_base_url}/-/pipelines/#{pipeline_id}/tests/suite.json?build_ids[]=#{build_id}") + def test_report_for_build(pipeline, build_id) + fetch("#{pipeline['web_url']}/tests/suite.json?build_ids[]=#{build_id}") end def build_test_report_json_for_pipeline(pipeline) @@ -82,7 +82,7 @@ class PipelineTestReportBuilder puts "Discovered last failed pipeline (#{pipeline['id']}) for MR!#{mr_id}" - failed_builds_for_test_stage = failed_builds_for_pipeline(pipeline['id']).select do |failed_build| + failed_builds_for_test_stage = failed_builds_for_pipeline(pipeline).select do |failed_build| failed_build['stage'] == 'test' end @@ -92,7 +92,7 @@ class PipelineTestReportBuilder test_report['suites'] ||= [] failed_builds_for_test_stage.each do |failed_build| - test_report['suites'] << test_report_for_build(pipeline['id'], failed_build['id']) + test_report['suites'] << test_report_for_build(pipeline, failed_build['id']) end end @@ -127,8 +127,8 @@ if $0 == __FILE__ options = Host::DEFAULT_OPTIONS.dup OptionParser.new do |opts| - opts.on("-p", "--project PROJECT", String, "Project where to find the merge request(defaults to $CI_PROJECT_ID)") do |value| - options[:project] = value + opts.on("-t", "--target-project TARGET_PROJECT", String, "Project where to find the merge request") do |value| + options[:target_project] = value end opts.on("-m", "--mr-id MR_ID", String, "A merge request ID") do |value| diff --git a/scripts/rspec_helpers.sh b/scripts/rspec_helpers.sh index 7ac6b98938f..a3565db58fa 100644 --- a/scripts/rspec_helpers.sh +++ b/scripts/rspec_helpers.sh @@ -94,11 +94,14 @@ function retrieve_previous_failed_tests() { local rspec_pg_regex="${2}" local rspec_ee_pg_regex="${3}" local pipeline_report_path="test_results/previous/test_reports.json" - local project_path="gitlab-org/gitlab" + + # Used to query merge requests. This variable reflects where the merge request has been created + local target_project_path="${CI_MERGE_REQUEST_PROJECT_PATH}" + local instance_url="${CI_SERVER_URL}" echo 'Attempting to build pipeline test report...' - scripts/pipeline_test_report_builder.rb --instance-base-url "https://gitlab.com" --project "${project_path}" --mr-id "${CI_MERGE_REQUEST_IID}" --output-file-path "${pipeline_report_path}" + scripts/pipeline_test_report_builder.rb --instance-base-url "${instance_url}" --target-project "${target_project_path}" --mr-id "${CI_MERGE_REQUEST_IID}" --output-file-path "${pipeline_report_path}" echo 'Generating failed tests lists...' diff --git a/spec/factories/operations/feature_flags/strategy.rb b/spec/factories/operations/feature_flags/strategy.rb index bdb5d9f0f3c..8d04b6d25aa 100644 --- a/spec/factories/operations/feature_flags/strategy.rb +++ b/spec/factories/operations/feature_flags/strategy.rb @@ -5,5 +5,37 @@ FactoryBot.define do association :feature_flag, factory: :operations_feature_flag name { "default" } parameters { {} } + + trait :default do + name { "default" } + parameters { {} } + end + + trait :gitlab_userlist do + association :user_list, factory: :operations_feature_flag_user_list + name { "gitlabUserList" } + parameters { {} } + end + + trait :flexible_rollout do + name { "flexibleRollout" } + parameters do + { + groupId: 'default', + rollout: '10', + stickiness: 'default' + } + end + end + + trait :gradual_rollout do + name { "gradualRolloutUserId" } + parameters { { percentage: '10', groupId: 'default' } } + end + + trait :userwithid do + name { "userWithId" } + parameters { { userIds: 'user1' } } + end end end diff --git a/spec/frontend/environments/graphql/mock_data.js b/spec/frontend/environments/graphql/mock_data.js index 6cd8c0f4314..7b241fe44db 100644 --- a/spec/frontend/environments/graphql/mock_data.js +++ b/spec/frontend/environments/graphql/mock_data.js @@ -1,5 +1,34 @@ export const environmentsApp = { environments: [ + { + name: 'review', + size: 2, + latest: { + id: 42, + global_id: 'gid://gitlab/Environment/42', + name: 'review/goodbye', + state: 'available', + external_url: 'https://example.org', + environment_type: 'review', + name_without_type: 'goodbye', + last_deployment: null, + has_stop_action: false, + rollout_status: null, + environment_path: '/h5bp/html5-boilerplate/-/environments/42', + stop_path: '/h5bp/html5-boilerplate/-/environments/42/stop', + cancel_auto_stop_path: '/h5bp/html5-boilerplate/-/environments/42/cancel_auto_stop', + delete_path: '/api/v4/projects/8/environments/42', + folder_path: '/h5bp/html5-boilerplate/-/environments/folders/review', + created_at: '2021-10-04T19:27:20.639Z', + updated_at: '2021-10-04T19:27:20.639Z', + can_stop: true, + logs_path: '/h5bp/html5-boilerplate/-/logs?environment_name=review%2Fgoodbye', + logs_api_path: '/h5bp/html5-boilerplate/-/logs/k8s.json?environment_name=review%2Fgoodbye', + enable_advanced_logs_querying: false, + can_delete: false, + has_opened_alert: false, + }, + }, { name: 'production', size: 1, @@ -134,35 +163,6 @@ export const environmentsApp = { has_opened_alert: false, }, }, - { - name: 'review', - size: 2, - latest: { - id: 42, - global_id: 'gid://gitlab/Environment/42', - name: 'review/goodbye', - state: 'available', - external_url: 'https://example.org', - environment_type: 'review', - name_without_type: 'goodbye', - last_deployment: null, - has_stop_action: false, - rollout_status: null, - environment_path: '/h5bp/html5-boilerplate/-/environments/42', - stop_path: '/h5bp/html5-boilerplate/-/environments/42/stop', - cancel_auto_stop_path: '/h5bp/html5-boilerplate/-/environments/42/cancel_auto_stop', - delete_path: '/api/v4/projects/8/environments/42', - folder_path: '/h5bp/html5-boilerplate/-/environments/folders/review', - created_at: '2021-10-04T19:27:20.639Z', - updated_at: '2021-10-04T19:27:20.639Z', - can_stop: true, - logs_path: '/h5bp/html5-boilerplate/-/logs?environment_name=review%2Fgoodbye', - logs_api_path: '/h5bp/html5-boilerplate/-/logs/k8s.json?environment_name=review%2Fgoodbye', - enable_advanced_logs_querying: false, - can_delete: false, - has_opened_alert: false, - }, - }, { name: 'staging', size: 1, @@ -206,6 +206,36 @@ export const environmentsApp = { export const resolvedEnvironmentsApp = { availableCount: 4, environments: [ + { + name: 'review', + size: 2, + latest: { + id: 42, + globalId: 'gid://gitlab/Environment/42', + name: 'review/goodbye', + state: 'available', + externalUrl: 'https://example.org', + environmentType: 'review', + nameWithoutType: 'goodbye', + lastDeployment: null, + hasStopAction: false, + rolloutStatus: null, + environmentPath: '/h5bp/html5-boilerplate/-/environments/42', + stopPath: '/h5bp/html5-boilerplate/-/environments/42/stop', + cancelAutoStopPath: '/h5bp/html5-boilerplate/-/environments/42/cancel_auto_stop', + deletePath: '/api/v4/projects/8/environments/42', + folderPath: '/h5bp/html5-boilerplate/-/environments/folders/review', + createdAt: '2021-10-04T19:27:20.639Z', + updatedAt: '2021-10-04T19:27:20.639Z', + canStop: true, + logsPath: '/h5bp/html5-boilerplate/-/logs?environment_name=review%2Fgoodbye', + logsApiPath: '/h5bp/html5-boilerplate/-/logs/k8s.json?environment_name=review%2Fgoodbye', + enableAdvancedLogsQuerying: false, + canDelete: false, + hasOpenedAlert: false, + }, + __typename: 'NestedEnvironment', + }, { name: 'production', size: 1, @@ -340,36 +370,6 @@ export const resolvedEnvironmentsApp = { }, __typename: 'NestedEnvironment', }, - { - name: 'review', - size: 2, - latest: { - id: 42, - globalId: 'gid://gitlab/Environment/42', - name: 'review/goodbye', - state: 'available', - externalUrl: 'https://example.org', - environmentType: 'review', - nameWithoutType: 'goodbye', - lastDeployment: null, - hasStopAction: false, - rolloutStatus: null, - environmentPath: '/h5bp/html5-boilerplate/-/environments/42', - stopPath: '/h5bp/html5-boilerplate/-/environments/42/stop', - cancelAutoStopPath: '/h5bp/html5-boilerplate/-/environments/42/cancel_auto_stop', - deletePath: '/api/v4/projects/8/environments/42', - folderPath: '/h5bp/html5-boilerplate/-/environments/folders/review', - createdAt: '2021-10-04T19:27:20.639Z', - updatedAt: '2021-10-04T19:27:20.639Z', - canStop: true, - logsPath: '/h5bp/html5-boilerplate/-/logs?environment_name=review%2Fgoodbye', - logsApiPath: '/h5bp/html5-boilerplate/-/logs/k8s.json?environment_name=review%2Fgoodbye', - enableAdvancedLogsQuerying: false, - canDelete: false, - hasOpenedAlert: false, - }, - __typename: 'NestedEnvironment', - }, { name: 'staging', size: 1, diff --git a/spec/frontend/environments/new_environment_folder_spec.js b/spec/frontend/environments/new_environment_folder_spec.js new file mode 100644 index 00000000000..5696e187a86 --- /dev/null +++ b/spec/frontend/environments/new_environment_folder_spec.js @@ -0,0 +1,74 @@ +import VueApollo from 'vue-apollo'; +import Vue from 'vue'; +import { GlCollapse, GlIcon } from '@gitlab/ui'; +import createMockApollo from 'helpers/mock_apollo_helper'; +import { mountExtended } from 'helpers/vue_test_utils_helper'; +import EnvironmentsFolder from '~/environments/components/new_environment_folder.vue'; +import { s__ } from '~/locale'; +import { resolvedEnvironmentsApp, resolvedFolder } from './graphql/mock_data'; + +Vue.use(VueApollo); + +describe('~/environments/components/new_environments_folder.vue', () => { + let wrapper; + let environmentFolderMock; + let nestedEnvironment; + let folderName; + + const findLink = () => wrapper.findByRole('link', { name: s__('Environments|Show all') }); + + const createApolloProvider = () => { + const mockResolvers = { Query: { folder: environmentFolderMock } }; + + return createMockApollo([], mockResolvers); + }; + + const createWrapper = (propsData, apolloProvider) => + mountExtended(EnvironmentsFolder, { apolloProvider, propsData }); + + beforeEach(() => { + environmentFolderMock = jest.fn(); + [nestedEnvironment] = resolvedEnvironmentsApp.environments; + environmentFolderMock.mockReturnValue(resolvedFolder); + wrapper = createWrapper({ nestedEnvironment }, createApolloProvider()); + folderName = wrapper.findByText(nestedEnvironment.name); + }); + + afterEach(() => { + wrapper?.destroy(); + }); + + it('displays the name of the folder', () => { + expect(folderName.text()).toBe(nestedEnvironment.name); + }); + + describe('collapse', () => { + let icons; + let collapse; + + beforeEach(() => { + collapse = wrapper.findComponent(GlCollapse); + icons = wrapper.findAllComponents(GlIcon); + }); + + it('is collapsed by default', () => { + const link = findLink(); + + expect(collapse.attributes('visible')).toBeUndefined(); + expect(icons.wrappers.map((i) => i.props('name'))).toEqual(['angle-right', 'folder-o']); + expect(folderName.classes('gl-font-weight-bold')).toBe(false); + expect(link.exists()).toBe(false); + }); + + it('opens on click', async () => { + await folderName.trigger('click'); + + const link = findLink(); + + expect(collapse.attributes('visible')).toBe('true'); + expect(icons.wrappers.map((i) => i.props('name'))).toEqual(['angle-down', 'folder-open']); + expect(folderName.classes('gl-font-weight-bold')).toBe(true); + expect(link.attributes('href')).toBe(nestedEnvironment.latest.folderPath); + }); + }); +}); diff --git a/spec/frontend/environments/new_environments_app_spec.js b/spec/frontend/environments/new_environments_app_spec.js new file mode 100644 index 00000000000..0ad8e8f442c --- /dev/null +++ b/spec/frontend/environments/new_environments_app_spec.js @@ -0,0 +1,50 @@ +import Vue from 'vue'; +import VueApollo from 'vue-apollo'; +import { mount } from '@vue/test-utils'; +import createMockApollo from 'helpers/mock_apollo_helper'; +import waitForPromises from 'helpers/wait_for_promises'; +import EnvironmentsApp from '~/environments/components/new_environments_app.vue'; +import EnvironmentsFolder from '~/environments/components/new_environment_folder.vue'; +import { resolvedEnvironmentsApp, resolvedFolder } from './graphql/mock_data'; + +Vue.use(VueApollo); + +describe('~/environments/components/new_environments_app.vue', () => { + let wrapper; + let environmentAppMock; + let environmentFolderMock; + + const createApolloProvider = () => { + const mockResolvers = { + Query: { environmentApp: environmentAppMock, folder: environmentFolderMock }, + }; + + return createMockApollo([], mockResolvers); + }; + + const createWrapper = (apolloProvider) => mount(EnvironmentsApp, { apolloProvider }); + + beforeEach(() => { + environmentAppMock = jest.fn(); + environmentFolderMock = jest.fn(); + }); + + afterEach(() => { + wrapper?.destroy(); + }); + + it('should show all the folders that are fetched', async () => { + environmentAppMock.mockReturnValue(resolvedEnvironmentsApp); + environmentFolderMock.mockReturnValue(resolvedFolder); + const apolloProvider = createApolloProvider(); + wrapper = createWrapper(apolloProvider); + + await waitForPromises(); + await Vue.nextTick(); + + const text = wrapper.findAllComponents(EnvironmentsFolder).wrappers.map((w) => w.text()); + + expect(text).toContainEqual(expect.stringMatching('review')); + expect(text).not.toContainEqual(expect.stringMatching('production')); + }); +}); diff --git a/spec/frontend/ide/components/pipelines/__snapshots__/list_spec.js.snap b/spec/frontend/ide/components/pipelines/__snapshots__/list_spec.js.snap index 194a619c4aa..47e3a56e83d 100644 --- a/spec/frontend/ide/components/pipelines/__snapshots__/list_spec.js.snap +++ b/spec/frontend/ide/components/pipelines/__snapshots__/list_spec.js.snap @@ -8,7 +8,7 @@ exports[`IDE pipelines list when loaded renders empty state when no latestPipeli
diff --git a/spec/models/design_management/version_spec.rb b/spec/models/design_management/version_spec.rb index e004ad024bc..303bac61e1e 100644 --- a/spec/models/design_management/version_spec.rb +++ b/spec/models/design_management/version_spec.rb @@ -283,7 +283,7 @@ RSpec.describe DesignManagement::Version do it 'retrieves author from the Commit if author_id is nil and version has been persisted' do author = create(:user) version = create(:design_version, :committed, author: author) - author.destroy + author.destroy! version.reload commit = version.issue.project.design_repository.commit(version.sha) commit_user = create(:user, email: commit.author_email, name: commit.author_name) diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 872d97dc1eb..9d9862aa3d3 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -39,7 +39,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do it 'ensures environment tier when a new object is created' do environment = build(:environment, name: 'gprd', tier: nil) - expect { environment.save }.to change { environment.tier }.from(nil).to('production') + expect { environment.save! }.to change { environment.tier }.from(nil).to('production') end it 'ensures environment tier when an existing object is updated' do @@ -418,7 +418,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do context 'not in the same branch' do before do - deployment.update(sha: project.commit('feature').id) + deployment.update!(sha: project.commit('feature').id) end it 'returns false' do @@ -496,7 +496,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do context 'when no other actions' do context 'environment is available' do before do - environment.update(state: :available) + environment.update!(state: :available) end it do @@ -508,7 +508,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do context 'environment is already stopped' do before do - environment.update(state: :stopped) + environment.update!(state: :stopped) end it do @@ -1502,7 +1502,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do deployment = create(:deployment, :success, environment: environment, project: project) deployment.create_ref - expect { environment.destroy }.to change { project.commit(deployment.ref_path) }.to(nil) + expect { environment.destroy! }.to change { project.commit(deployment.ref_path) }.to(nil) end end @@ -1517,7 +1517,7 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do end it 'returns the environments count grouped by state with zero value' do - environment2.update(state: 'stopped') + environment2.update!(state: 'stopped') expect(project.environments.count_by_state).to eq({ stopped: 3, available: 0 }) end end diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index 41510b7aa1c..ee27eaf1d0b 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -32,7 +32,7 @@ RSpec.describe Event do describe 'after_create :set_last_repository_updated_at' do context 'with a push event' do it 'updates the project last_repository_updated_at' do - project.update(last_repository_updated_at: 1.year.ago) + project.update!(last_repository_updated_at: 1.year.ago) create_push_event(project, project.owner) @@ -44,7 +44,7 @@ RSpec.describe Event do context 'without a push event' do it 'does not update the project last_repository_updated_at' do - project.update(last_repository_updated_at: 1.year.ago) + project.update!(last_repository_updated_at: 1.year.ago) create(:closed_issue_event, project: project, author: project.owner) @@ -58,7 +58,7 @@ RSpec.describe Event do describe '#set_last_repository_updated_at' do it 'only updates once every Event::REPOSITORY_UPDATED_AT_INTERVAL minutes' do last_known_timestamp = (Event::REPOSITORY_UPDATED_AT_INTERVAL - 1.minute).ago - project.update(last_repository_updated_at: last_known_timestamp) + project.update!(last_repository_updated_at: last_known_timestamp) project.reload # a reload removes fractions of seconds expect do @@ -73,7 +73,7 @@ RSpec.describe Event do it 'passes event to UserInteractedProject.track' do expect(UserInteractedProject).to receive(:track).with(event) - event.save + event.save! end end end @@ -824,7 +824,7 @@ RSpec.describe Event do context 'when a project was updated less than 1 hour ago' do it 'does not update the project' do - project.update(last_activity_at: Time.current) + project.update!(last_activity_at: Time.current) expect(project).not_to receive(:update_column) .with(:last_activity_at, a_kind_of(Time)) @@ -835,7 +835,7 @@ RSpec.describe Event do context 'when a project was updated more than 1 hour ago' do it 'updates the project' do - project.update(last_activity_at: 1.year.ago) + project.update!(last_activity_at: 1.year.ago) create_push_event(project, project.owner) diff --git a/spec/models/fork_network_spec.rb b/spec/models/fork_network_spec.rb index c2ef1fdcb5f..f2ec0ccb4fd 100644 --- a/spec/models/fork_network_spec.rb +++ b/spec/models/fork_network_spec.rb @@ -8,7 +8,7 @@ RSpec.describe ForkNetwork do describe '#add_root_as_member' do it 'adds the root project as a member when creating a new root network' do project = create(:project) - fork_network = described_class.create(root_project: project) + fork_network = described_class.create!(root_project: project) expect(fork_network.projects).to include(project) end @@ -54,8 +54,8 @@ RSpec.describe ForkNetwork do first_fork = fork_project(first_project) second_fork = fork_project(second_project) - first_project.destroy - second_project.destroy + first_project.destroy! + second_project.destroy! expect(first_fork.fork_network).not_to be_nil expect(first_fork.fork_network.root_project).to be_nil diff --git a/spec/models/generic_commit_status_spec.rb b/spec/models/generic_commit_status_spec.rb index 6fe5a1407a9..9d70019734b 100644 --- a/spec/models/generic_commit_status_spec.rb +++ b/spec/models/generic_commit_status_spec.rb @@ -133,7 +133,7 @@ RSpec.describe GenericCommitStatus do before do generic_commit_status.context = nil generic_commit_status.stage = nil - generic_commit_status.save + generic_commit_status.save! end describe '#context' do diff --git a/spec/models/grafana_integration_spec.rb b/spec/models/grafana_integration_spec.rb index 79f102919ac..bb822187e0c 100644 --- a/spec/models/grafana_integration_spec.rb +++ b/spec/models/grafana_integration_spec.rb @@ -60,7 +60,7 @@ RSpec.describe GrafanaIntegration do context 'with grafana integration enabled' do it 'returns nil' do - grafana_integration.update(enabled: false) + grafana_integration.update!(enabled: false) expect(grafana_integration.client).to be(nil) end @@ -81,8 +81,8 @@ RSpec.describe GrafanaIntegration do end it 'prevents overriding token value with its encrypted or masked version', :aggregate_failures do - expect { grafana_integration.update(token: grafana_integration.encrypted_token) }.not_to change { grafana_integration.reload.send(:token) } - expect { grafana_integration.update(token: grafana_integration.masked_token) }.not_to change { grafana_integration.reload.send(:token) } + expect { grafana_integration.update!(token: grafana_integration.encrypted_token) }.not_to change { grafana_integration.reload.send(:token) } + expect { grafana_integration.update!(token: grafana_integration.masked_token) }.not_to change { grafana_integration.reload.send(:token) } end end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index f591c98071a..6b6e68ab937 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -160,7 +160,7 @@ RSpec.describe Group do context 'when sub group is deleted' do it 'does not delete parent notification settings' do expect do - sub_group.destroy + sub_group.destroy! end.to change { NotificationSetting.count }.by(-1) end end @@ -404,7 +404,7 @@ RSpec.describe Group do subject do recorded_queries.record do - group.update(parent: new_parent) + group.update!(parent: new_parent) end end @@ -496,7 +496,7 @@ RSpec.describe Group do let!(:group) { create(:group, parent: parent_group) } before do - parent_group.update(parent: new_grandparent) + parent_group.update!(parent: new_grandparent) end it 'updates traversal_ids for all descendants' do @@ -866,7 +866,7 @@ RSpec.describe Group do before do parent_group = create(:group) create(:group_member, :owner, group: parent_group) - group.update(parent: parent_group) + group.update!(parent: parent_group) end it { expect(group.last_owner?(@members[:owner])).to be_falsy } @@ -923,7 +923,7 @@ RSpec.describe Group do before do parent_group = create(:group) create(:group_member, :owner, group: parent_group) - group.update(parent: parent_group) + group.update!(parent: parent_group) end it { expect(group.member_last_blocked_owner?(member)).to be(false) } @@ -1971,7 +1971,7 @@ RSpec.describe Group do let(:environment) { 'foo%bar/test' } it 'matches literally for %' do - ci_variable.update(environment_scope: 'foo%bar/*') + ci_variable.update_attribute(:environment_scope, 'foo%bar/*') is_expected.to contain_exactly(ci_variable) end @@ -2112,7 +2112,7 @@ RSpec.describe Group do let(:ancestor_group) { create(:group) } before do - group.update(parent: ancestor_group) + group.update!(parent: ancestor_group) end it 'returns all ancestor group ids' do @@ -2629,7 +2629,7 @@ RSpec.describe Group do let_it_be(:project) { create(:project, group: group, service_desk_enabled: false) } before do - project.update(service_desk_enabled: false) + project.update!(service_desk_enabled: false) end it { is_expected.to eq(false) } diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb index c0efb2dff56..124c54a2028 100644 --- a/spec/models/identity_spec.rb +++ b/spec/models/identity_spec.rb @@ -118,19 +118,19 @@ RSpec.describe Identity do it 'if extern_uid changes' do expect(ldap_identity).not_to receive(:ensure_normalized_extern_uid) - ldap_identity.save + ldap_identity.save! end it 'if current_uid is nil' do expect(ldap_identity).to receive(:ensure_normalized_extern_uid) - ldap_identity.update(extern_uid: nil) + ldap_identity.update!(extern_uid: nil) expect(ldap_identity.extern_uid).to be_nil end it 'if extern_uid changed and not nil' do - ldap_identity.update(extern_uid: 'uid=john1,ou=PEOPLE,dc=example,dc=com') + ldap_identity.update!(extern_uid: 'uid=john1,ou=PEOPLE,dc=example,dc=com') expect(ldap_identity.extern_uid).to eq 'uid=john1,ou=people,dc=example,dc=com' end @@ -150,7 +150,7 @@ RSpec.describe Identity do expect(user.user_synced_attributes_metadata.provider).to eq 'ldapmain' - ldap_identity.destroy + ldap_identity.destroy! expect(user.reload.user_synced_attributes_metadata).to be_nil end @@ -162,7 +162,7 @@ RSpec.describe Identity do expect(user.user_synced_attributes_metadata.provider).to eq 'other' - ldap_identity.destroy + ldap_identity.destroy! expect(user.reload.user_synced_attributes_metadata.provider).to eq 'other' end diff --git a/spec/models/jira_import_state_spec.rb b/spec/models/jira_import_state_spec.rb index e982b7353ba..a272d001429 100644 --- a/spec/models/jira_import_state_spec.rb +++ b/spec/models/jira_import_state_spec.rb @@ -175,7 +175,7 @@ RSpec.describe JiraImportState do let(:jira_import) { build(:jira_import_state, project: project)} it 'does not run the callback', :aggregate_failures do - expect { jira_import.save }.to change { JiraImportState.count }.by(1) + expect { jira_import.save! }.to change { JiraImportState.count }.by(1) expect(jira_import.reload.error_message).to be_nil end end @@ -184,7 +184,7 @@ RSpec.describe JiraImportState do let(:jira_import) { build(:jira_import_state, project: project, error_message: 'error')} it 'does not run the callback', :aggregate_failures do - expect { jira_import.save }.to change { JiraImportState.count }.by(1) + expect { jira_import.save! }.to change { JiraImportState.count }.by(1) expect(jira_import.reload.error_message).to eq('error') end end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 4dae5b5201e..02cfb594348 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -228,7 +228,7 @@ RSpec.describe Namespace do expect(namespace.path).to eq('j') - namespace.update(name: 'something new') + namespace.update!(name: 'something new') expect(namespace).to be_valid expect(namespace.name).to eq('something new') @@ -240,7 +240,7 @@ RSpec.describe Namespace do it 'allows to update path to single char' do namespace = create(:project_namespace) - namespace.update(path: 'j') + namespace.update!(path: 'j') expect(namespace).to be_valid end @@ -740,7 +740,7 @@ RSpec.describe Namespace do end it "moves dir if path changed" do - namespace.update(path: namespace.full_path + '_new') + namespace.update!(path: namespace.full_path + '_new') expect(gitlab_shell.repository_exists?(project.repository_storage, "#{namespace.path}/#{project.path}.git")).to be_truthy end @@ -751,7 +751,7 @@ RSpec.describe Namespace do expect(namespace).to receive(:write_projects_repository_config).and_raise('foo') expect do - namespace.update(path: namespace.full_path + '_new') + namespace.update!(path: namespace.full_path + '_new') end.to raise_error('foo') end end @@ -768,7 +768,7 @@ RSpec.describe Namespace do end expect(Gitlab::ErrorTracking).to receive(:should_raise_for_dev?).and_return(false) # like prod - namespace.update(path: namespace.full_path + '_new') + namespace.update!(path: namespace.full_path + '_new') end end end @@ -998,7 +998,7 @@ RSpec.describe Namespace do it "repository directory remains unchanged if path changed" do before_disk_path = project.disk_path - namespace.update(path: namespace.full_path + '_new') + namespace.update!(path: namespace.full_path + '_new') expect(before_disk_path).to eq(project.disk_path) expect(gitlab_shell.repository_exists?(project.repository_storage, "#{project.disk_path}.git")).to be_truthy @@ -1013,7 +1013,7 @@ RSpec.describe Namespace do let!(:legacy_project_in_subgroup) { create(:project, :legacy_storage, :repository, namespace: subgroup, name: 'foo3') } it 'updates project full path in .git/config' do - parent.update(path: 'mygroup_new') + parent.update!(path: 'mygroup_new') expect(project_rugged(project_in_parent_group).config['gitlab.fullpath']).to eq "mygroup_new/#{project_in_parent_group.path}" expect(project_rugged(hashed_project_in_subgroup).config['gitlab.fullpath']).to eq "mygroup_new/mysubgroup/#{hashed_project_in_subgroup.path}" @@ -1025,7 +1025,7 @@ RSpec.describe Namespace do repository_hashed_project_in_subgroup = hashed_project_in_subgroup.project_repository repository_legacy_project_in_subgroup = legacy_project_in_subgroup.project_repository - parent.update(path: 'mygroup_moved') + parent.update!(path: 'mygroup_moved') expect(repository_project_in_parent_group.reload.disk_path).to eq "mygroup_moved/#{project_in_parent_group.path}" expect(repository_hashed_project_in_subgroup.reload.disk_path).to eq hashed_project_in_subgroup.disk_path @@ -1059,7 +1059,7 @@ RSpec.describe Namespace do it 'renames its dirs when deleted' do allow(GitlabShellWorker).to receive(:perform_in) - namespace.destroy + namespace.destroy! expect(File.exist?(deleted_path_in_dir)).to be(true) end @@ -1067,7 +1067,7 @@ RSpec.describe Namespace do it 'schedules the namespace for deletion' do expect(GitlabShellWorker).to receive(:perform_in).with(5.minutes, :rm_namespace, repository_storage, deleted_path) - namespace.destroy + namespace.destroy! end context 'in sub-groups' do @@ -1081,7 +1081,7 @@ RSpec.describe Namespace do it 'renames its dirs when deleted' do allow(GitlabShellWorker).to receive(:perform_in) - child.destroy + child.destroy! expect(File.exist?(deleted_path_in_dir)).to be(true) end @@ -1089,7 +1089,7 @@ RSpec.describe Namespace do it 'schedules the namespace for deletion' do expect(GitlabShellWorker).to receive(:perform_in).with(5.minutes, :rm_namespace, repository_storage, deleted_path) - child.destroy + child.destroy! end end end @@ -1102,7 +1102,7 @@ RSpec.describe Namespace do expect(File.exist?(path_in_dir)).to be(false) - namespace.destroy + namespace.destroy! expect(File.exist?(deleted_path_in_dir)).to be(false) end @@ -1628,7 +1628,7 @@ RSpec.describe Namespace do it 'returns the path before last save' do group = create(:group) - group.update(parent: nil) + group.update!(parent: nil) expect(group.full_path_before_last_save).to eq(group.path_before_last_save) end @@ -1639,7 +1639,7 @@ RSpec.describe Namespace do group = create(:group, parent: nil) parent = create(:group) - group.update(parent: parent) + group.update!(parent: parent) expect(group.full_path_before_last_save).to eq("#{group.path_before_last_save}") end @@ -1650,7 +1650,7 @@ RSpec.describe Namespace do parent = create(:group) group = create(:group, parent: parent) - group.update(parent: nil) + group.update!(parent: nil) expect(group.full_path_before_last_save).to eq("#{parent.full_path}/#{group.path}") end @@ -1662,7 +1662,7 @@ RSpec.describe Namespace do group = create(:group, parent: parent) new_parent = create(:group) - group.update(parent: new_parent) + group.update!(parent: new_parent) expect(group.full_path_before_last_save).to eq("#{parent.full_path}/#{group.path}") end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 0dd77967f25..9d9cca0678a 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -155,7 +155,7 @@ RSpec.describe Note do expect(note).to receive(:notify_after_destroy).and_call_original expect(note.noteable).to receive(:after_note_destroyed).with(note) - note.destroy + note.destroy! end it 'does not error if noteable is nil' do @@ -163,7 +163,7 @@ RSpec.describe Note do expect(note).to receive(:notify_after_destroy).and_call_original expect(note).to receive(:noteable).at_least(:once).and_return(nil) - expect { note.destroy }.not_to raise_error + expect { note.destroy! }.not_to raise_error end end end @@ -226,8 +226,8 @@ RSpec.describe Note do describe 'read' do before do - @p1.project_members.create(user: @u2, access_level: ProjectMember::GUEST) - @p2.project_members.create(user: @u3, access_level: ProjectMember::GUEST) + @p1.project_members.create!(user: @u2, access_level: ProjectMember::GUEST) + @p2.project_members.create!(user: @u3, access_level: ProjectMember::GUEST) end it { expect(Ability.allowed?(@u1, :read_note, @p1)).to be_falsey } @@ -237,8 +237,8 @@ RSpec.describe Note do describe 'write' do before do - @p1.project_members.create(user: @u2, access_level: ProjectMember::DEVELOPER) - @p2.project_members.create(user: @u3, access_level: ProjectMember::DEVELOPER) + @p1.project_members.create!(user: @u2, access_level: ProjectMember::DEVELOPER) + @p2.project_members.create!(user: @u3, access_level: ProjectMember::DEVELOPER) end it { expect(Ability.allowed?(@u1, :create_note, @p1)).to be_falsey } @@ -248,9 +248,9 @@ RSpec.describe Note do describe 'admin' do before do - @p1.project_members.create(user: @u1, access_level: ProjectMember::REPORTER) - @p1.project_members.create(user: @u2, access_level: ProjectMember::MAINTAINER) - @p2.project_members.create(user: @u3, access_level: ProjectMember::MAINTAINER) + @p1.project_members.create!(user: @u1, access_level: ProjectMember::REPORTER) + @p1.project_members.create!(user: @u2, access_level: ProjectMember::MAINTAINER) + @p2.project_members.create!(user: @u3, access_level: ProjectMember::MAINTAINER) end it { expect(Ability.allowed?(@u1, :admin_note, @p1)).to be_falsey } @@ -1468,7 +1468,7 @@ RSpec.describe Note do shared_examples 'assignee check' do context 'when the provided user is one of the assignees' do before do - note.noteable.update(assignees: [user, create(:user)]) + note.noteable.update!(assignees: [user, create(:user)]) end it 'returns true' do @@ -1480,7 +1480,7 @@ RSpec.describe Note do shared_examples 'author check' do context 'when the provided user is the author' do before do - note.noteable.update(author: user) + note.noteable.update!(author: user) end it 'returns true' do diff --git a/spec/models/notification_setting_spec.rb b/spec/models/notification_setting_spec.rb index 3f1684327e7..cc601fb30c2 100644 --- a/spec/models/notification_setting_spec.rb +++ b/spec/models/notification_setting_spec.rb @@ -36,7 +36,7 @@ RSpec.describe NotificationSetting do notification_setting.merge_merge_request = "t" notification_setting.close_merge_request = "nil" notification_setting.reopen_merge_request = "false" - notification_setting.save + notification_setting.save! end it "parses boolean before saving" do @@ -52,12 +52,12 @@ RSpec.describe NotificationSetting do context 'notification_email' do let_it_be(:user) { create(:user) } - subject { described_class.new(source_id: 1, source_type: 'Project', user_id: user.id) } + subject { build(:notification_setting, user_id: user.id) } it 'allows to change email to verified one' do email = create(:email, :confirmed, user: user) - subject.update(notification_email: email.email) + subject.notification_email = email.email expect(subject).to be_valid end @@ -65,13 +65,13 @@ RSpec.describe NotificationSetting do it 'does not allow to change email to not verified one' do email = create(:email, user: user) - subject.update(notification_email: email.email) + subject.notification_email = email.email expect(subject).to be_invalid end it 'allows to change email to empty one' do - subject.update(notification_email: '') + subject.notification_email = '' expect(subject).to be_valid end @@ -85,7 +85,7 @@ RSpec.describe NotificationSetting do 1.upto(4) do |i| setting = create(:notification_setting, user: user) - setting.project.update(pending_delete: true) if i.even? + setting.project.update!(pending_delete: true) if i.even? end end diff --git a/spec/models/operations/feature_flags/strategy_spec.rb b/spec/models/operations/feature_flags/strategy_spec.rb index 9289e3beab5..de1b9d2c855 100644 --- a/spec/models/operations/feature_flags/strategy_spec.rb +++ b/spec/models/operations/feature_flags/strategy_spec.rb @@ -20,8 +20,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do end with_them do it 'skips parameters validation' do - strategy = described_class.create(feature_flag: feature_flag, - name: invalid_name, parameters: { bad: 'params' }) + strategy = build(:operations_strategy, + feature_flag: feature_flag, + name: invalid_name, + parameters: { bad: 'params' }) + + expect(strategy).to be_invalid expect(strategy.errors[:name]).to eq(['strategy name is invalid']) expect(strategy.errors[:parameters]).to be_empty @@ -36,19 +40,24 @@ RSpec.describe Operations::FeatureFlags::Strategy do end with_them do it 'must have valid parameters for the strategy' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'gradualRolloutUserId', parameters: invalid_parameters) + strategy = build(:operations_strategy, + :gradual_rollout, + feature_flag: feature_flag, + parameters: invalid_parameters) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to eq(['parameters are invalid']) end end it 'allows the parameters in any order' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'gradualRolloutUserId', - parameters: { percentage: '10', groupId: 'mygroup' }) + strategy = build(:operations_strategy, + :gradual_rollout, + feature_flag: feature_flag, + parameters: { percentage: '10', groupId: 'mygroup' }) - expect(strategy.errors[:parameters]).to be_empty + expect(strategy).to be_valid end describe 'percentage' do @@ -59,9 +68,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do end with_them do it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'gradualRolloutUserId', - parameters: { groupId: 'mygroup', percentage: invalid_value }) + strategy = build(:operations_strategy, + :gradual_rollout, + feature_flag: feature_flag, + parameters: { groupId: 'mygroup', percentage: invalid_value }) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to eq(['percentage must be a string between 0 and 100 inclusive']) end @@ -72,11 +84,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do end with_them do it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'gradualRolloutUserId', - parameters: { groupId: 'mygroup', percentage: valid_value }) + strategy = build(:operations_strategy, + :gradual_rollout, + feature_flag: feature_flag, + parameters: { groupId: 'mygroup', percentage: valid_value }) - expect(strategy.errors[:parameters]).to eq([]) + expect(strategy).to be_valid end end end @@ -88,9 +101,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do end with_them do it 'must be a string value of up to 32 lowercase characters' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'gradualRolloutUserId', - parameters: { groupId: invalid_value, percentage: '40' }) + strategy = build(:operations_strategy, + :gradual_rollout, + feature_flag: feature_flag, + parameters: { groupId: invalid_value, percentage: '40' }) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to eq(['groupId parameter is invalid']) end @@ -101,11 +117,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do end with_them do it 'must be a string value of up to 32 lowercase characters' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'gradualRolloutUserId', - parameters: { groupId: valid_value, percentage: '40' }) + strategy = build(:operations_strategy, + :gradual_rollout, + feature_flag: feature_flag, + parameters: { groupId: valid_value, percentage: '40' }) - expect(strategy.errors[:parameters]).to eq([]) + expect(strategy).to be_valid end end end @@ -123,9 +140,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do ]) with_them do it 'must have valid parameters for the strategy' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'flexibleRollout', - parameters: invalid_parameters) + strategy = build(:operations_strategy, + :flexible_rollout, + feature_flag: feature_flag, + parameters: invalid_parameters) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to eq(['parameters are invalid']) end @@ -137,11 +157,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do [:groupId, 'mygroup'] ].permutation(3).each do |parameters| it "allows the parameters in the order #{parameters.map { |p| p.first }.join(', ')}" do - strategy = described_class.create(feature_flag: feature_flag, - name: 'flexibleRollout', - parameters: Hash[parameters]) + strategy = build(:operations_strategy, + :flexible_rollout, + feature_flag: feature_flag, + parameters: Hash[parameters]) - expect(strategy.errors[:parameters]).to be_empty + expect(strategy).to be_valid end end @@ -152,9 +173,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do with_them do it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do parameters = { stickiness: 'default', groupId: 'mygroup', rollout: invalid_value } - strategy = described_class.create(feature_flag: feature_flag, - name: 'flexibleRollout', - parameters: parameters) + strategy = build(:operations_strategy, + :flexible_rollout, + feature_flag: feature_flag, + parameters: parameters) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to eq([ 'rollout must be a string between 0 and 100 inclusive' @@ -166,11 +190,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do with_them do it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do parameters = { stickiness: 'default', groupId: 'mygroup', rollout: valid_value } - strategy = described_class.create(feature_flag: feature_flag, - name: 'flexibleRollout', - parameters: parameters) + strategy = build(:operations_strategy, + :flexible_rollout, + feature_flag: feature_flag, + parameters: parameters) - expect(strategy.errors[:parameters]).to eq([]) + expect(strategy).to be_valid end end end @@ -181,9 +206,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do with_them do it 'must be a string value of up to 32 lowercase characters' do parameters = { stickiness: 'default', groupId: invalid_value, rollout: '40' } - strategy = described_class.create(feature_flag: feature_flag, - name: 'flexibleRollout', - parameters: parameters) + strategy = build(:operations_strategy, + :flexible_rollout, + feature_flag: feature_flag, + parameters: parameters) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to eq(['groupId parameter is invalid']) end @@ -193,11 +221,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do with_them do it 'must be a string value of up to 32 lowercase characters' do parameters = { stickiness: 'default', groupId: valid_value, rollout: '40' } - strategy = described_class.create(feature_flag: feature_flag, - name: 'flexibleRollout', - parameters: parameters) + strategy = build(:operations_strategy, + :flexible_rollout, + feature_flag: feature_flag, + parameters: parameters) - expect(strategy.errors[:parameters]).to eq([]) + expect(strategy).to be_valid end end end @@ -207,9 +236,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do with_them do it 'must be a string representing a supported stickiness setting' do parameters = { stickiness: invalid_value, groupId: 'mygroup', rollout: '40' } - strategy = described_class.create(feature_flag: feature_flag, - name: 'flexibleRollout', - parameters: parameters) + strategy = build(:operations_strategy, + :flexible_rollout, + feature_flag: feature_flag, + parameters: parameters) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to eq([ 'stickiness parameter must be default, userId, sessionId, or random' @@ -221,11 +253,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do with_them do it 'must be a string representing a supported stickiness setting' do parameters = { stickiness: valid_value, groupId: 'mygroup', rollout: '40' } - strategy = described_class.create(feature_flag: feature_flag, - name: 'flexibleRollout', - parameters: parameters) + strategy = build(:operations_strategy, + :flexible_rollout, + feature_flag: feature_flag, + parameters: parameters) - expect(strategy.errors[:parameters]).to eq([]) + expect(strategy).to be_valid end end end @@ -237,8 +270,11 @@ RSpec.describe Operations::FeatureFlags::Strategy do end with_them do it 'must have valid parameters for the strategy' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'userWithId', parameters: invalid_parameters) + strategy = build(:operations_strategy, + feature_flag: feature_flag, + name: 'userWithId', parameters: invalid_parameters) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to eq(['parameters are invalid']) end @@ -253,10 +289,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do end with_them do it 'is valid with a string of comma separated values' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'userWithId', parameters: { userIds: valid_value }) + strategy = build(:operations_strategy, + feature_flag: feature_flag, + name: 'userWithId', + parameters: { userIds: valid_value }) - expect(strategy.errors[:parameters]).to be_empty + expect(strategy).to be_valid end end @@ -267,8 +305,12 @@ RSpec.describe Operations::FeatureFlags::Strategy do end with_them do it 'is invalid' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'userWithId', parameters: { userIds: invalid_value }) + strategy = build(:operations_strategy, + feature_flag: feature_flag, + name: 'userWithId', + parameters: { userIds: invalid_value }) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to include( 'userIds must be a string of unique comma separated values each 256 characters or less' @@ -284,43 +326,48 @@ RSpec.describe Operations::FeatureFlags::Strategy do end with_them do it 'must be empty' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'default', - parameters: invalid_value) + strategy = build(:operations_strategy, :default, feature_flag: feature_flag, parameters: invalid_value) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to eq(['parameters are invalid']) end end it 'must be empty' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'default', - parameters: {}) + strategy = build(:operations_strategy, :default, feature_flag: feature_flag) - expect(strategy.errors[:parameters]).to be_empty + expect(strategy).to be_valid end end context 'when the strategy name is gitlabUserList' do + let_it_be(:user_list) { create(:operations_feature_flag_user_list, project: project) } + where(:invalid_value) do [{ groupId: "default", percentage: "7" }, "", "nothing", 7, nil, [], 2.5, { userIds: 'user1' }] end with_them do - it 'must be empty' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'gitlabUserList', - parameters: invalid_value) + it 'is invalid' do + strategy = build(:operations_strategy, + :gitlab_userlist, + user_list: user_list, + feature_flag: feature_flag, + parameters: invalid_value) + + expect(strategy).to be_invalid expect(strategy.errors[:parameters]).to eq(['parameters are invalid']) end end - it 'must be empty' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'gitlabUserList', - parameters: {}) + it 'is valid' do + strategy = build(:operations_strategy, + :gitlab_userlist, + user_list: user_list, + feature_flag: feature_flag) - expect(strategy.errors[:parameters]).to be_empty + expect(strategy).to be_valid end end end @@ -329,18 +376,15 @@ RSpec.describe Operations::FeatureFlags::Strategy do context 'when name is gitlabUserList' do it 'is valid when associated with a user list' do user_list = create(:operations_feature_flag_user_list, project: project) - strategy = described_class.create(feature_flag: feature_flag, - name: 'gitlabUserList', - user_list: user_list, - parameters: {}) + strategy = build(:operations_strategy, :gitlab_userlist, feature_flag: feature_flag, user_list: user_list) - expect(strategy.errors[:user_list]).to be_empty + expect(strategy).to be_valid end it 'is invalid without a user list' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'gitlabUserList', - parameters: {}) + strategy = build(:operations_strategy, :gitlab_userlist, feature_flag: feature_flag, user_list: nil) + + expect(strategy).to be_invalid expect(strategy.errors[:user_list]).to eq(["can't be blank"]) end @@ -348,10 +392,9 @@ RSpec.describe Operations::FeatureFlags::Strategy do it 'is invalid when associated with a user list from another project' do other_project = create(:project) user_list = create(:operations_feature_flag_user_list, project: other_project) - strategy = described_class.create(feature_flag: feature_flag, - name: 'gitlabUserList', - user_list: user_list, - parameters: {}) + strategy = build(:operations_strategy, :gitlab_userlist, feature_flag: feature_flag, user_list: user_list) + + expect(strategy).to be_invalid expect(strategy.errors[:user_list]).to eq(['must belong to the same project']) end @@ -360,84 +403,68 @@ RSpec.describe Operations::FeatureFlags::Strategy do context 'when name is default' do it 'is invalid when associated with a user list' do user_list = create(:operations_feature_flag_user_list, project: project) - strategy = described_class.create(feature_flag: feature_flag, - name: 'default', - user_list: user_list, - parameters: {}) + strategy = build(:operations_strategy, :default, feature_flag: feature_flag, user_list: user_list) + + expect(strategy).to be_invalid expect(strategy.errors[:user_list]).to eq(['must be blank']) end it 'is valid without a user list' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'default', - parameters: {}) + strategy = build(:operations_strategy, :default, feature_flag: feature_flag) - expect(strategy.errors[:user_list]).to be_empty + expect(strategy).to be_valid end end context 'when name is userWithId' do it 'is invalid when associated with a user list' do user_list = create(:operations_feature_flag_user_list, project: project) - strategy = described_class.create(feature_flag: feature_flag, - name: 'userWithId', - user_list: user_list, - parameters: { userIds: 'user1' }) + strategy = build(:operations_strategy, :userwithid, feature_flag: feature_flag, user_list: user_list) + + expect(strategy).to be_invalid expect(strategy.errors[:user_list]).to eq(['must be blank']) end it 'is valid without a user list' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'userWithId', - parameters: { userIds: 'user1' }) + strategy = build(:operations_strategy, :userwithid, feature_flag: feature_flag) - expect(strategy.errors[:user_list]).to be_empty + expect(strategy).to be_valid end end context 'when name is gradualRolloutUserId' do it 'is invalid when associated with a user list' do user_list = create(:operations_feature_flag_user_list, project: project) - strategy = described_class.create(feature_flag: feature_flag, - name: 'gradualRolloutUserId', - user_list: user_list, - parameters: { groupId: 'default', percentage: '10' }) + strategy = build(:operations_strategy, :gradual_rollout, feature_flag: feature_flag, user_list: user_list) + + expect(strategy).to be_invalid expect(strategy.errors[:user_list]).to eq(['must be blank']) end it 'is valid without a user list' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'gradualRolloutUserId', - parameters: { groupId: 'default', percentage: '10' }) + strategy = build(:operations_strategy, :gradual_rollout, feature_flag: feature_flag) - expect(strategy.errors[:user_list]).to be_empty + expect(strategy).to be_valid end end context 'when name is flexibleRollout' do it 'is invalid when associated with a user list' do user_list = create(:operations_feature_flag_user_list, project: project) - strategy = described_class.create(feature_flag: feature_flag, - name: 'flexibleRollout', - user_list: user_list, - parameters: { groupId: 'default', - rollout: '10', - stickiness: 'default' }) + strategy = build(:operations_strategy, :flexible_rollout, feature_flag: feature_flag, user_list: user_list) + + expect(strategy).to be_invalid expect(strategy.errors[:user_list]).to eq(['must be blank']) end it 'is valid without a user list' do - strategy = described_class.create(feature_flag: feature_flag, - name: 'flexibleRollout', - parameters: { groupId: 'default', - rollout: '10', - stickiness: 'default' }) + strategy = build(:operations_strategy, :flexible_rollout, feature_flag: feature_flag) - expect(strategy.errors[:user_list]).to be_empty + expect(strategy).to be_valid end end end diff --git a/spec/models/operations/feature_flags/user_list_spec.rb b/spec/models/operations/feature_flags/user_list_spec.rb index 3a48d3389a3..b2dbebb2c0d 100644 --- a/spec/models/operations/feature_flags/user_list_spec.rb +++ b/spec/models/operations/feature_flags/user_list_spec.rb @@ -20,9 +20,9 @@ RSpec.describe Operations::FeatureFlags::UserList do end with_them do it 'is valid with a string of comma separated values' do - user_list = described_class.create(user_xids: valid_value) + user_list = build(:operations_feature_flag_user_list, user_xids: valid_value) - expect(user_list.errors[:user_xids]).to be_empty + expect(user_list).to be_valid end end @@ -31,9 +31,10 @@ RSpec.describe Operations::FeatureFlags::UserList do end with_them do it 'automatically casts values of other types' do - user_list = described_class.create(user_xids: typecast_value) + user_list = build(:operations_feature_flag_user_list, user_xids: typecast_value) + + expect(user_list).to be_valid - expect(user_list.errors[:user_xids]).to be_empty expect(user_list.user_xids).to eq(typecast_value.to_s) end end @@ -45,7 +46,9 @@ RSpec.describe Operations::FeatureFlags::UserList do end with_them do it 'is invalid' do - user_list = described_class.create(user_xids: invalid_value) + user_list = build(:operations_feature_flag_user_list, user_xids: invalid_value) + + expect(user_list).to be_invalid expect(user_list.errors[:user_xids]).to include( 'user_xids must be a string of unique comma separated values each 256 characters or less' @@ -70,20 +73,20 @@ RSpec.describe Operations::FeatureFlags::UserList do describe '#destroy' do it 'deletes the model if it is not associated with any feature flag strategies' do project = create(:project) - user_list = described_class.create(project: project, name: 'My User List', user_xids: 'user1,user2') + user_list = described_class.create!(project: project, name: 'My User List', user_xids: 'user1,user2') - user_list.destroy + user_list.destroy! expect(described_class.count).to eq(0) end it 'does not delete the model if it is associated with a feature flag strategy' do project = create(:project) - user_list = described_class.create(project: project, name: 'My User List', user_xids: 'user1,user2') + user_list = described_class.create!(project: project, name: 'My User List', user_xids: 'user1,user2') feature_flag = create(:operations_feature_flag, :new_version_flag, project: project) strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'gitlabUserList', user_list: user_list) - user_list.destroy + user_list.destroy # rubocop:disable Rails/SaveBang expect(described_class.count).to eq(1) expect(::Operations::FeatureFlags::StrategyUserList.count).to eq(1) diff --git a/spec/models/pages_domain_spec.rb b/spec/models/pages_domain_spec.rb index 2b6ed9a9927..d476e18a72c 100644 --- a/spec/models/pages_domain_spec.rb +++ b/spec/models/pages_domain_spec.rb @@ -104,7 +104,7 @@ RSpec.describe PagesDomain do let(:domain) { build(:pages_domain) } it 'saves validity time' do - domain.save + domain.save! expect(domain.certificate_valid_not_before).to be_like_time(Time.zone.parse("2020-03-16 14:20:34 UTC")) expect(domain.certificate_valid_not_after).to be_like_time(Time.zone.parse("2220-01-28 14:20:34 UTC")) @@ -161,7 +161,7 @@ RSpec.describe PagesDomain do context 'when certificate is already saved' do it "doesn't add error to certificate" do - domain.save(validate: false) + domain.save!(validate: false) domain.valid? diff --git a/spec/models/protectable_dropdown_spec.rb b/spec/models/protectable_dropdown_spec.rb index 918c3078405..ab3f455fe63 100644 --- a/spec/models/protectable_dropdown_spec.rb +++ b/spec/models/protectable_dropdown_spec.rb @@ -16,7 +16,7 @@ RSpec.describe ProtectableDropdown do describe '#protectable_ref_names' do context 'when project repository is not empty' do before do - project.protected_branches.create(name: 'master') + create(:protected_branch, project: project, name: 'master') end it { expect(subject.protectable_ref_names).to include('feature') } diff --git a/spec/models/redirect_route_spec.rb b/spec/models/redirect_route_spec.rb index c6e35923b89..2de662fd4b4 100644 --- a/spec/models/redirect_route_spec.rb +++ b/spec/models/redirect_route_spec.rb @@ -4,7 +4,7 @@ require 'spec_helper' RSpec.describe RedirectRoute do let(:group) { create(:group) } - let!(:redirect_route) { group.redirect_routes.create(path: 'gitlabb') } + let!(:redirect_route) { group.redirect_routes.create!(path: 'gitlabb') } describe 'relationships' do it { is_expected.to belong_to(:source) } @@ -17,10 +17,10 @@ RSpec.describe RedirectRoute do end describe '.matching_path_and_descendants' do - let!(:redirect2) { group.redirect_routes.create(path: 'gitlabb/test') } - let!(:redirect3) { group.redirect_routes.create(path: 'gitlabb/test/foo') } - let!(:redirect4) { group.redirect_routes.create(path: 'gitlabb/test/foo/bar') } - let!(:redirect5) { group.redirect_routes.create(path: 'gitlabb/test/baz') } + let!(:redirect2) { group.redirect_routes.create!(path: 'gitlabb/test') } + let!(:redirect3) { group.redirect_routes.create!(path: 'gitlabb/test/foo') } + let!(:redirect4) { group.redirect_routes.create!(path: 'gitlabb/test/foo/bar') } + let!(:redirect5) { group.redirect_routes.create!(path: 'gitlabb/test/baz') } context 'when the redirect route matches with same casing' do it 'returns correct routes' do diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb index b88813b3328..125fec61d72 100644 --- a/spec/models/release_spec.rb +++ b/spec/models/release_spec.rb @@ -26,10 +26,10 @@ RSpec.describe Release do context 'when a release exists in the database without a name' do it 'does not require name' do existing_release_without_name = build(:release, project: project, author: user, name: nil) - existing_release_without_name.save(validate: false) + existing_release_without_name.save!(validate: false) existing_release_without_name.description = "change" - existing_release_without_name.save + existing_release_without_name.save! existing_release_without_name.reload expect(existing_release_without_name).to be_valid @@ -88,7 +88,7 @@ RSpec.describe Release do describe '.create' do it "fills released_at using created_at if it's not set" do - release = described_class.create(project: project, author: user) + release = create(:release, project: project, author: user, released_at: nil) expect(release.released_at).to eq(release.created_at) end @@ -96,14 +96,14 @@ RSpec.describe Release do it "does not change released_at if it's set explicitly" do released_at = Time.zone.parse('2018-10-20T18:00:00Z') - release = described_class.create(project: project, author: user, released_at: released_at) + release = create(:release, project: project, author: user, released_at: released_at) expect(release.released_at).to eq(released_at) end end describe '#update' do - subject { release.update(params) } + subject { release.update!(params) } context 'when links do not exist' do context 'when params are specified for creation' do @@ -182,7 +182,7 @@ RSpec.describe Release do it 'also deletes the associated evidence' do release_with_evidence - expect { release_with_evidence.destroy }.to change(Releases::Evidence, :count).by(-1) + expect { release_with_evidence.destroy! }.to change(Releases::Evidence, :count).by(-1) end end end @@ -190,7 +190,7 @@ RSpec.describe Release do describe '#name' do context 'name is nil' do before do - release.update(name: nil) + release.update!(name: nil) end it 'returns tag' do diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb index 382359ccb17..9f1d1c84da3 100644 --- a/spec/models/remote_mirror_spec.rb +++ b/spec/models/remote_mirror_spec.rb @@ -289,7 +289,7 @@ RSpec.describe RemoteMirror, :mailer do context 'with remote mirroring disabled' do it 'returns nil' do - remote_mirror.update(enabled: false) + remote_mirror.update!(enabled: false) expect(remote_mirror.sync).to be_nil end @@ -354,7 +354,7 @@ RSpec.describe RemoteMirror, :mailer do let(:remote_mirror) { create(:project, :repository, :remote_mirror).remote_mirrors.first } it 'resets all the columns when URL changes' do - remote_mirror.update(last_error: Time.current, + remote_mirror.update!(last_error: Time.current, last_update_at: Time.current, last_successful_update_at: Time.current, update_status: 'started', @@ -378,7 +378,7 @@ RSpec.describe RemoteMirror, :mailer do end before do - remote_mirror.update(last_update_started_at: Time.current) + remote_mirror.update!(last_update_started_at: Time.current) end context 'when remote mirror does not have status failed' do @@ -393,7 +393,7 @@ RSpec.describe RemoteMirror, :mailer do context 'when remote mirror has status failed' do it 'returns false when last update started after the timestamp' do - remote_mirror.update(update_status: 'failed') + remote_mirror.update!(update_status: 'failed') expect(remote_mirror.updated_since?(timestamp)).to be false end @@ -409,7 +409,7 @@ RSpec.describe RemoteMirror, :mailer do updated_at: 25.hours.ago) project = mirror.project project.pending_delete = true - project.save + project.save! mirror.reload expect(mirror.sync).to be_nil diff --git a/spec/models/route_spec.rb b/spec/models/route_spec.rb index eb81db95cd3..b2fa9c24535 100644 --- a/spec/models/route_spec.rb +++ b/spec/models/route_spec.rb @@ -31,18 +31,18 @@ RSpec.describe Route do context 'after update' do it 'calls #create_redirect_for_old_path' do expect(route).to receive(:create_redirect_for_old_path) - route.update(path: 'foo') + route.update!(path: 'foo') end it 'calls #delete_conflicting_redirects' do expect(route).to receive(:delete_conflicting_redirects) - route.update(path: 'foo') + route.update!(path: 'foo') end end context 'after create' do it 'calls #delete_conflicting_redirects' do - route.destroy + route.destroy! new_route = described_class.new(source: group, path: group.path) expect(new_route).to receive(:delete_conflicting_redirects) new_route.save! @@ -81,7 +81,7 @@ RSpec.describe Route do context 'path update' do context 'when route name is set' do before do - route.update(path: 'bar') + route.update!(path: 'bar') end it 'updates children routes with new path' do @@ -111,7 +111,7 @@ RSpec.describe Route do let!(:conflicting_redirect3) { route.create_redirect('gitlab-org') } it 'deletes the conflicting redirects' do - route.update(path: 'bar') + route.update!(path: 'bar') expect(RedirectRoute.exists?(path: 'bar/test')).to be_falsey expect(RedirectRoute.exists?(path: 'bar/test/foo')).to be_falsey @@ -122,7 +122,7 @@ RSpec.describe Route do context 'name update' do it 'updates children routes with new path' do - route.update(name: 'bar') + route.update!(name: 'bar') expect(described_class.exists?(name: 'bar')).to be_truthy expect(described_class.exists?(name: 'bar / test')).to be_truthy @@ -134,7 +134,7 @@ RSpec.describe Route do # Note: using `update_columns` to skip all validation and callbacks route.update_columns(name: nil) - expect { route.update(name: 'bar') } + expect { route.update!(name: 'bar') } .to change { route.name }.from(nil).to('bar') end end diff --git a/spec/models/sentry_issue_spec.rb b/spec/models/sentry_issue_spec.rb index c24350d7067..09b23b6fd0d 100644 --- a/spec/models/sentry_issue_spec.rb +++ b/spec/models/sentry_issue_spec.rb @@ -53,7 +53,7 @@ RSpec.describe SentryIssue do create(:sentry_issue) project = sentry_issue.issue.project sentry_issue_3 = build(:sentry_issue, issue: create(:issue, project: project), sentry_issue_identifier: sentry_issue.sentry_issue_identifier) - sentry_issue_3.save(validate: false) + sentry_issue_3.save!(validate: false) result = described_class.for_project_and_identifier(project, sentry_issue.sentry_issue_identifier) diff --git a/spec/models/snippet_spec.rb b/spec/models/snippet_spec.rb index 4e20a83f18e..e24dd910c39 100644 --- a/spec/models/snippet_spec.rb +++ b/spec/models/snippet_spec.rb @@ -98,7 +98,7 @@ RSpec.describe Snippet do snippet = build(:snippet) expect(snippet.statistics).to be_nil - snippet.save + snippet.save! expect(snippet.statistics).to be_persisted end @@ -289,7 +289,7 @@ RSpec.describe Snippet do let(:access_level) { ProjectFeature::ENABLED } before do - project.project_feature.update(snippets_access_level: access_level) + project.project_feature.update!(snippets_access_level: access_level) end it 'includes snippets for projects with snippets enabled' do @@ -623,7 +623,7 @@ RSpec.describe Snippet do context 'when snippet_repository does not exist' do it 'creates a snippet_repository' do - snippet.snippet_repository.destroy + snippet.snippet_repository.destroy! snippet.reload expect do diff --git a/spec/models/upload_spec.rb b/spec/models/upload_spec.rb index 7e2c97c8425..cdf73b203af 100644 --- a/spec/models/upload_spec.rb +++ b/spec/models/upload_spec.rb @@ -19,7 +19,7 @@ RSpec.describe Upload do it 'schedules checksum calculation' do stub_const('UploadChecksumWorker', spy) - upload = described_class.create( + upload = described_class.create!( path: __FILE__, size: described_class::CHECKSUM_THRESHOLD + 1.kilobyte, model: build_stubbed(:user), @@ -42,7 +42,7 @@ RSpec.describe Upload do store: ObjectStorage::Store::LOCAL ) - expect { upload.save } + expect { upload.save! } .to change { upload.checksum }.from(nil) .to(a_string_matching(/\A\h{64}\z/)) end @@ -55,7 +55,7 @@ RSpec.describe Upload do it 'calls delete_file!' do is_expected.to receive(:delete_file!) - subject.destroy + subject.destroy! end end end diff --git a/spec/scripts/pipeline_test_report_builder_spec.rb b/spec/scripts/pipeline_test_report_builder_spec.rb index 8a5388f4db8..8553ada044e 100644 --- a/spec/scripts/pipeline_test_report_builder_spec.rb +++ b/spec/scripts/pipeline_test_report_builder_spec.rb @@ -9,30 +9,39 @@ RSpec.describe PipelineTestReportBuilder do subject do described_class.new( - project: 'gitlab-org/gitlab', + target_project: 'gitlab-org/gitlab', mr_id: '999', instance_base_url: 'https://gitlab.com', output_file_path: output_file_path ) end - let(:mr_pipelines) do - [ - { - 'status' => 'running', - 'created_at' => DateTime.now.to_s - }, - { - 'status' => 'failed', - 'created_at' => (DateTime.now - 5).to_s - } - ] + let(:failed_pipeline_url) { 'pipeline2_url' } + + let(:failed_pipeline) do + { + 'status' => 'failed', + 'created_at' => (DateTime.now - 5).to_s, + 'web_url' => failed_pipeline_url + } end + let(:current_pipeline) do + { + 'status' => 'running', + 'created_at' => DateTime.now.to_s, + 'web_url' => 'pipeline1_url' + } + end + + let(:mr_pipelines) { [current_pipeline, failed_pipeline] } + + let(:failed_build_id) { 9999 } + let(:failed_builds_for_pipeline) do [ { - 'id' => 9999, + 'id' => failed_build_id, 'stage' => 'test' } ] @@ -62,75 +71,114 @@ RSpec.describe PipelineTestReportBuilder do before do allow(subject).to receive(:pipelines_for_mr).and_return(mr_pipelines) allow(subject).to receive(:failed_builds_for_pipeline).and_return(failed_builds_for_pipeline) - allow(subject).to receive(:test_report_for_build).and_return(test_report_for_build) + end + + describe '#previous_pipeline' do + let(:fork_pipeline_url) { 'fork_pipeline_url' } + let(:fork_pipeline) do + { + 'status' => 'failed', + 'created_at' => (DateTime.now - 5).to_s, + 'web_url' => fork_pipeline_url + } + end + + before do + allow(subject).to receive(:test_report_for_build).and_return(test_report_for_build) + end + + context 'pipeline in a fork project' do + let(:mr_pipelines) { [current_pipeline, fork_pipeline] } + + it 'returns fork pipeline' do + expect(subject.previous_pipeline).to eq(fork_pipeline) + end + end + + context 'pipeline in target project' do + it 'returns failed pipeline' do + expect(subject.previous_pipeline).to eq(failed_pipeline) + end + end end describe '#test_report_for_latest_pipeline' do - context 'no previous pipeline' do - let(:mr_pipelines) { [] } - - it 'returns empty hash' do - expect(subject.test_report_for_latest_pipeline).to eq("{}") - end + it 'fetches builds from pipeline related to MR' do + expect(subject).to receive(:fetch).with("#{failed_pipeline_url}/tests/suite.json?build_ids[]=#{failed_build_id}").and_return(failed_builds_for_pipeline) + subject.test_report_for_latest_pipeline end - context 'first pipeline scenario' do - let(:mr_pipelines) do - [ - { - 'status' => 'running', - 'created_at' => DateTime.now.to_s - } - ] + context 'canonical pipeline' do + before do + allow(subject).to receive(:test_report_for_build).and_return(test_report_for_build) end - it 'returns empty hash' do - expect(subject.test_report_for_latest_pipeline).to eq("{}") - end - end + context 'no previous pipeline' do + let(:mr_pipelines) { [] } - context 'no previous failed pipeline' do - let(:mr_pipelines) do - [ - { - 'status' => 'running', - 'created_at' => DateTime.now.to_s - }, - { - 'status' => 'success', - 'created_at' => (DateTime.now - 5).to_s - } - ] + it 'returns empty hash' do + expect(subject.test_report_for_latest_pipeline).to eq("{}") + end end - it 'returns empty hash' do - expect(subject.test_report_for_latest_pipeline).to eq("{}") - end - end + context 'first pipeline scenario' do + let(:mr_pipelines) do + [ + { + 'status' => 'running', + 'created_at' => DateTime.now.to_s + } + ] + end - context 'no failed test builds' do - let(:failed_builds_for_pipeline) do - [ - { - 'id' => 9999, - 'stage' => 'prepare' - } - ] + it 'returns empty hash' do + expect(subject.test_report_for_latest_pipeline).to eq("{}") + end end - it 'returns empty hash' do - expect(subject.test_report_for_latest_pipeline).to eq("{}") + context 'no previous failed pipeline' do + let(:mr_pipelines) do + [ + { + 'status' => 'running', + 'created_at' => DateTime.now.to_s + }, + { + 'status' => 'success', + 'created_at' => (DateTime.now - 5).to_s + } + ] + end + + it 'returns empty hash' do + expect(subject.test_report_for_latest_pipeline).to eq("{}") + end end - end - context 'failed pipeline and failed test builds' do - it 'returns populated test list for suites' do - actual = subject.test_report_for_latest_pipeline - expected = { - 'suites' => [test_report_for_build] - }.to_json + context 'no failed test builds' do + let(:failed_builds_for_pipeline) do + [ + { + 'id' => 9999, + 'stage' => 'prepare' + } + ] + end - expect(actual).to eq(expected) + it 'returns empty hash' do + expect(subject.test_report_for_latest_pipeline).to eq("{}") + end + end + + context 'failed pipeline and failed test builds' do + it 'returns populated test list for suites' do + actual = subject.test_report_for_latest_pipeline + expected = { + 'suites' => [test_report_for_build] + }.to_json + + expect(actual).to eq(expected) + end end end end