From 6121ad5af38294f12db08f13aec122c3dbef583a Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 12 Dec 2023 18:07:46 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- spec/frontend/emoji/components/emoji_group_spec.js | 15 ++- .../settings/general/components/change_url_spec.js | 94 +++++++++++------ .../components/organization_settings_spec.js | 108 +++++++++++++------ .../resolvers/group_milestones_resolver_spec.rb | 73 +------------ spec/requests/api/graphql/milestone_spec.rb | 14 +++ spec/requests/api/group_milestones_spec.rb | 114 ++++++++++++--------- spec/requests/api/ml_model_packages_spec.rb | 42 ++++---- .../find_or_create_model_version_service_spec.rb | 18 +++- .../ml_model/create_package_file_service_spec.rb | 51 +++------ .../api/graphql_rest/milestones_shared_examples.rb | 83 +++++++++++++++ .../api/ml_model_packages_shared_examples.rb | 42 +++++--- spec/tooling/danger/project_helper_spec.rb | 5 +- 12 files changed, 381 insertions(+), 278 deletions(-) create mode 100644 spec/support/shared_examples/requests/api/graphql_rest/milestones_shared_examples.rb (limited to 'spec') diff --git a/spec/frontend/emoji/components/emoji_group_spec.js b/spec/frontend/emoji/components/emoji_group_spec.js index 75397ce25ff..a2a46bedd7b 100644 --- a/spec/frontend/emoji/components/emoji_group_spec.js +++ b/spec/frontend/emoji/components/emoji_group_spec.js @@ -1,5 +1,6 @@ import { shallowMount } from '@vue/test-utils'; import Vue from 'vue'; +import { GlButton } from '@gitlab/ui'; import { extendedWrapper } from 'helpers/vue_test_utils_helper'; import EmojiGroup from '~/emoji/components/emoji_group.vue'; @@ -10,6 +11,9 @@ function factory(propsData = {}) { wrapper = extendedWrapper( shallowMount(EmojiGroup, { propsData, + stubs: { + GlButton, + }, }), ); } @@ -19,7 +23,6 @@ describe('Emoji group component', () => { factory({ emojis: [], renderGroup: false, - clickEmoji: jest.fn(), }); expect(wrapper.findByTestId('emoji-button').exists()).toBe(false); @@ -29,24 +32,20 @@ describe('Emoji group component', () => { factory({ emojis: ['thumbsup', 'thumbsdown'], renderGroup: true, - clickEmoji: jest.fn(), }); expect(wrapper.findAllByTestId('emoji-button').exists()).toBe(true); expect(wrapper.findAllByTestId('emoji-button').length).toBe(2); }); - it('calls clickEmoji', () => { - const clickEmoji = jest.fn(); - + it('emits emoji-click', () => { factory({ emojis: ['thumbsup', 'thumbsdown'], renderGroup: true, - clickEmoji, }); - wrapper.findByTestId('emoji-button').trigger('click'); + wrapper.findComponent(GlButton).vm.$emit('click'); - expect(clickEmoji).toHaveBeenCalledWith('thumbsup'); + expect(wrapper.emitted('emoji-click')).toStrictEqual([['thumbsup']]); }); }); diff --git a/spec/frontend/organizations/settings/general/components/change_url_spec.js b/spec/frontend/organizations/settings/general/components/change_url_spec.js index 65289a83b7c..a4e3db0557c 100644 --- a/spec/frontend/organizations/settings/general/components/change_url_spec.js +++ b/spec/frontend/organizations/settings/general/components/change_url_spec.js @@ -4,10 +4,14 @@ import Vue, { nextTick } from 'vue'; import { mountExtended } from 'helpers/vue_test_utils_helper'; import ChangeUrl from '~/organizations/settings/general/components/change_url.vue'; -import resolvers from '~/organizations/shared/graphql/resolvers'; -import { updateOrganizationResponse } from '~/organizations/mock_data'; +import organizationUpdateMutation from '~/organizations/settings/general/graphql/mutations/organization_update.mutation.graphql'; +import { + organizationUpdateResponse, + organizationUpdateResponseWithErrors, +} from '~/organizations/mock_data'; import { createAlert } from '~/alert'; import { visitUrlWithAlerts } from '~/lib/utils/url_utility'; +import FormErrorsAlert from '~/vue_shared/components/form/errors_alert.vue'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; @@ -16,7 +20,6 @@ jest.mock('~/lib/utils/url_utility', () => ({ ...jest.requireActual('~/lib/utils/url_utility'), visitUrlWithAlerts: jest.fn(), })); -jest.useFakeTimers(); Vue.use(VueApollo); @@ -34,8 +37,12 @@ describe('ChangeUrl', () => { rootUrl: 'http://127.0.0.1:3000/', }; - const createComponent = ({ mockResolvers = resolvers } = {}) => { - mockApollo = createMockApollo([], mockResolvers); + const successfulResponseHandler = jest.fn().mockResolvedValue(organizationUpdateResponse); + + const createComponent = ({ + handlers = [[organizationUpdateMutation, successfulResponseHandler]], + } = {}) => { + mockApollo = createMockApollo(handlers); wrapper = mountExtended(ChangeUrl, { attachTo: document.body, @@ -94,13 +101,11 @@ describe('ChangeUrl', () => { describe('when API is loading', () => { beforeEach(async () => { - const mockResolvers = { - Mutation: { - updateOrganization: jest.fn().mockReturnValueOnce(new Promise(() => {})), - }, - }; - - createComponent({ mockResolvers }); + createComponent({ + handlers: [ + [organizationUpdateMutation, jest.fn().mockReturnValueOnce(new Promise(() => {}))], + ], + }); await findOrganizationUrlField().setValue('foo-bar-baz'); await submitForm(); @@ -116,13 +121,18 @@ describe('ChangeUrl', () => { createComponent(); await findOrganizationUrlField().setValue('foo-bar-baz'); await submitForm(); - jest.runAllTimers(); await waitForPromises(); }); - it('redirects user to new organization settings page and shows success alert', () => { + it('calls mutation with correct variables and redirects user to new organization settings page with success alert', () => { + expect(successfulResponseHandler).toHaveBeenCalledWith({ + input: { + id: 'gid://gitlab/Organizations::Organization/1', + path: 'foo-bar-baz', + }, + }); expect(visitUrlWithAlerts).toHaveBeenCalledWith( - `${updateOrganizationResponse.organization.webUrl}/settings/general`, + `${organizationUpdateResponse.data.organizationUpdate.organization.webUrl}/settings/general`, [ { id: 'organization-url-successfully-changed', @@ -135,27 +145,45 @@ describe('ChangeUrl', () => { }); describe('when API request is not successful', () => { - const error = new Error(); - - beforeEach(async () => { - const mockResolvers = { - Mutation: { - updateOrganization: jest.fn().mockRejectedValueOnce(error), - }, - }; + describe('when there is a network error', () => { + const error = new Error(); + + beforeEach(async () => { + createComponent({ + handlers: [[organizationUpdateMutation, jest.fn().mockRejectedValue(error)]], + }); + await findOrganizationUrlField().setValue('foo-bar-baz'); + await submitForm(); + await waitForPromises(); + }); - createComponent({ mockResolvers }); - await findOrganizationUrlField().setValue('foo-bar-baz'); - await submitForm(); - jest.runAllTimers(); - await waitForPromises(); + it('displays error alert', () => { + expect(createAlert).toHaveBeenCalledWith({ + message: 'An error occurred changing your organization URL. Please try again.', + error, + captureError: true, + }); + }); }); - it('displays error alert', () => { - expect(createAlert).toHaveBeenCalledWith({ - message: 'An error occurred changing your organization URL. Please try again.', - error, - captureError: true, + describe('when there are GraphQL errors', () => { + beforeEach(async () => { + createComponent({ + handlers: [ + [ + organizationUpdateMutation, + jest.fn().mockResolvedValue(organizationUpdateResponseWithErrors), + ], + ], + }); + await submitForm(); + await waitForPromises(); + }); + + it('displays form errors alert', () => { + expect(wrapper.findComponent(FormErrorsAlert).props('errors')).toEqual( + organizationUpdateResponseWithErrors.data.organizationUpdate.errors, + ); }); }); }); diff --git a/spec/frontend/organizations/settings/general/components/organization_settings_spec.js b/spec/frontend/organizations/settings/general/components/organization_settings_spec.js index 7645b41e3bd..d1c637331a8 100644 --- a/spec/frontend/organizations/settings/general/components/organization_settings_spec.js +++ b/spec/frontend/organizations/settings/general/components/organization_settings_spec.js @@ -6,14 +6,26 @@ import OrganizationSettings from '~/organizations/settings/general/components/or import SettingsBlock from '~/vue_shared/components/settings/settings_block.vue'; import NewEditForm from '~/organizations/shared/components/new_edit_form.vue'; import { FORM_FIELD_NAME, FORM_FIELD_ID } from '~/organizations/shared/constants'; -import resolvers from '~/organizations/shared/graphql/resolvers'; -import { createAlert, VARIANT_INFO } from '~/alert'; +import organizationUpdateMutation from '~/organizations/settings/general/graphql/mutations/organization_update.mutation.graphql'; +import { + organizationUpdateResponse, + organizationUpdateResponseWithErrors, +} from '~/organizations/mock_data'; +import { createAlert } from '~/alert'; +import { visitUrlWithAlerts } from '~/lib/utils/url_utility'; +import FormErrorsAlert from '~/vue_shared/components/form/errors_alert.vue'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; +import { useMockLocationHelper } from 'helpers/mock_window_location_helper'; Vue.use(VueApollo); -jest.useFakeTimers(); jest.mock('~/alert'); +jest.mock('~/lib/utils/url_utility', () => ({ + ...jest.requireActual('~/lib/utils/url_utility'), + visitUrlWithAlerts: jest.fn(), +})); + +useMockLocationHelper(); describe('OrganizationSettings', () => { let wrapper; @@ -26,8 +38,12 @@ describe('OrganizationSettings', () => { }, }; - const createComponent = ({ mockResolvers = resolvers } = {}) => { - mockApollo = createMockApollo([], mockResolvers); + const successfulResponseHandler = jest.fn().mockResolvedValue(organizationUpdateResponse); + + const createComponent = ({ + handlers = [[organizationUpdateMutation, successfulResponseHandler]], + } = {}) => { + mockApollo = createMockApollo(handlers); wrapper = shallowMountExtended(OrganizationSettings, { provide: defaultProvide, @@ -66,13 +82,11 @@ describe('OrganizationSettings', () => { describe('when form is submitted', () => { describe('when API is loading', () => { beforeEach(async () => { - const mockResolvers = { - Mutation: { - updateOrganization: jest.fn().mockReturnValueOnce(new Promise(() => {})), - }, - }; - - createComponent({ mockResolvers }); + createComponent({ + handlers: [ + [organizationUpdateMutation, jest.fn().mockReturnValueOnce(new Promise(() => {}))], + ], + }); await submitForm(); }); @@ -86,39 +100,65 @@ describe('OrganizationSettings', () => { beforeEach(async () => { createComponent(); await submitForm(); - jest.runAllTimers(); await waitForPromises(); }); - it('displays info alert', () => { - expect(createAlert).toHaveBeenCalledWith({ - message: 'Organization was successfully updated.', - variant: VARIANT_INFO, + it('calls mutation with correct variables and displays info alert', () => { + expect(successfulResponseHandler).toHaveBeenCalledWith({ + input: { + id: 'gid://gitlab/Organizations::Organization/1', + name: 'Foo bar', + }, }); + expect(visitUrlWithAlerts).toHaveBeenCalledWith(window.location.href, [ + { + id: 'organization-successfully-updated', + message: 'Organization was successfully updated.', + variant: 'info', + }, + ]); }); }); describe('when API request is not successful', () => { - const error = new Error(); - - beforeEach(async () => { - const mockResolvers = { - Mutation: { - updateOrganization: jest.fn().mockRejectedValueOnce(error), - }, - }; + describe('when there is a network error', () => { + const error = new Error(); + + beforeEach(async () => { + createComponent({ + handlers: [[organizationUpdateMutation, jest.fn().mockRejectedValue(error)]], + }); + await submitForm(); + await waitForPromises(); + }); - createComponent({ mockResolvers }); - await submitForm(); - jest.runAllTimers(); - await waitForPromises(); + it('displays error alert', () => { + expect(createAlert).toHaveBeenCalledWith({ + message: 'An error occurred updating your organization. Please try again.', + error, + captureError: true, + }); + }); }); - it('displays error alert', () => { - expect(createAlert).toHaveBeenCalledWith({ - message: 'An error occurred updating your organization. Please try again.', - error, - captureError: true, + describe('when there are GraphQL errors', () => { + beforeEach(async () => { + createComponent({ + handlers: [ + [ + organizationUpdateMutation, + jest.fn().mockResolvedValue(organizationUpdateResponseWithErrors), + ], + ], + }); + await submitForm(); + await waitForPromises(); + }); + + it('displays form errors alert', () => { + expect(wrapper.findComponent(FormErrorsAlert).props('errors')).toEqual( + organizationUpdateResponseWithErrors.data.organizationUpdate.errors, + ); }); }); }); diff --git a/spec/graphql/resolvers/group_milestones_resolver_spec.rb b/spec/graphql/resolvers/group_milestones_resolver_spec.rb index b9b8ef1870b..e9caf91ecb7 100644 --- a/spec/graphql/resolvers/group_milestones_resolver_spec.rb +++ b/spec/graphql/resolvers/group_milestones_resolver_spec.rb @@ -102,76 +102,7 @@ RSpec.describe Resolvers::GroupMilestonesResolver, feature_category: :team_plann end end - context 'when including descendant milestones in a public group' do - let_it_be(:group) { create(:group, :public) } - - let(:args) { { include_descendants: true } } - - it 'finds milestones only in accessible projects and groups' do - accessible_group = create(:group, :private, parent: group) - accessible_project = create(:project, group: accessible_group) - accessible_group.add_developer(current_user) - inaccessible_group = create(:group, :private, parent: group) - inaccessible_project = create(:project, :private, group: group) - milestone1 = create(:milestone, group: group) - milestone2 = create(:milestone, group: accessible_group) - milestone3 = create(:milestone, project: accessible_project) - create(:milestone, group: inaccessible_group) - create(:milestone, project: inaccessible_project) - - expect(resolve_group_milestones(args: args)).to match_array([milestone1, milestone2, milestone3]) - end - end - - describe 'include_descendants and include_ancestors' do - let_it_be(:parent_group) { create(:group, :public) } - let_it_be(:group) { create(:group, :public, parent: parent_group) } - let_it_be(:accessible_group) { create(:group, :private, parent: group) } - let_it_be(:accessible_project) { create(:project, group: accessible_group) } - let_it_be(:inaccessible_group) { create(:group, :private, parent: group) } - let_it_be(:inaccessible_project) { create(:project, :private, group: group) } - let_it_be(:milestone1) { create(:milestone, group: group) } - let_it_be(:milestone2) { create(:milestone, group: accessible_group) } - let_it_be(:milestone3) { create(:milestone, project: accessible_project) } - let_it_be(:milestone4) { create(:milestone, group: inaccessible_group) } - let_it_be(:milestone5) { create(:milestone, project: inaccessible_project) } - let_it_be(:milestone6) { create(:milestone, group: parent_group) } - - before do - accessible_group.add_developer(current_user) - end - - context 'when including neither ancestor or descendant milestones in a public group' do - let(:args) { {} } - - it 'finds milestones only in accessible projects and groups' do - expect(resolve_group_milestones(args: args)).to match_array([milestone1]) - end - end - - context 'when including descendant milestones in a public group' do - let(:args) { { include_descendants: true } } - - it 'finds milestones only in accessible projects and groups' do - expect(resolve_group_milestones(args: args)).to match_array([milestone1, milestone2, milestone3]) - end - end - - context 'when including ancestor milestones in a public group' do - let(:args) { { include_ancestors: true } } - - it 'finds milestones only in accessible projects and groups' do - expect(resolve_group_milestones(args: args)).to match_array([milestone1, milestone6]) - end - end - - context 'when including both ancestor or descendant milestones in a public group' do - let(:args) { { include_descendants: true, include_ancestors: true } } - - it 'finds milestones only in accessible projects and groups' do - expect(resolve_group_milestones(args: args)).to match_array([milestone1, milestone2, milestone3, milestone6]) - end - end - end + # testing for include_descendants and include_ancestors moved into + # `spec/requests/api/graphql/milestone_spec.rb` end end diff --git a/spec/requests/api/graphql/milestone_spec.rb b/spec/requests/api/graphql/milestone_spec.rb index 2cea9fd0408..0dc2eabc3e1 100644 --- a/spec/requests/api/graphql/milestone_spec.rb +++ b/spec/requests/api/graphql/milestone_spec.rb @@ -151,4 +151,18 @@ RSpec.describe 'Querying a Milestone', feature_category: :team_planning do end end end + + context 'for common GraphQL/REST' do + it_behaves_like 'group milestones including ancestors and descendants' + + def query_group_milestone_ids(params) + query = graphql_query_for('group', { 'fullPath' => group.full_path }, + query_graphql_field('milestones', params, query_graphql_path([:nodes], :id)) + ) + + post_graphql(query, current_user: current_user) + + graphql_data_at(:group, :milestones, :nodes).pluck('id').map { |gid| GlobalID.parse(gid).model_id.to_i } + end + end end diff --git a/spec/requests/api/group_milestones_spec.rb b/spec/requests/api/group_milestones_spec.rb index 2b17ce1d40f..82a4311f7d0 100644 --- a/spec/requests/api/group_milestones_spec.rb +++ b/spec/requests/api/group_milestones_spec.rb @@ -30,91 +30,103 @@ RSpec.describe API::GroupMilestones, feature_category: :team_planning do it_behaves_like 'group and project milestones', "/groups/:id/milestones" describe 'GET /groups/:id/milestones' do - let_it_be(:ancestor_group) { create(:group, :private) } - let_it_be(:ancestor_group_milestone) { create(:milestone, group: ancestor_group, updated_at: 2.days.ago) } + context 'for REST only' do + let_it_be(:ancestor_group) { create(:group, :private) } + let_it_be(:ancestor_group_milestone) { create(:milestone, group: ancestor_group, updated_at: 2.days.ago) } - before_all do - group.update!(parent: ancestor_group) - end + before_all do + group.update!(parent: ancestor_group) + end - context 'when include_ancestors is true' do - let(:params) { { include_ancestors: true } } + context 'when include_ancestors is true' do + let(:params) { { include_ancestors: true } } - context 'when user has access to ancestor groups' do - let(:milestones) { [ancestor_group_milestone, milestone, closed_milestone] } + context 'when user has access to ancestor groups' do + let(:milestones) { [ancestor_group_milestone, milestone, closed_milestone] } - before do - ancestor_group.add_guest(user) - group.add_guest(user) - end + before do + ancestor_group.add_guest(user) + group.add_guest(user) + end - it_behaves_like 'listing all milestones' + it_behaves_like 'listing all milestones' - context 'when deprecated include_parent_milestones is true' do - let(:params) { { include_parent_milestones: true } } + context 'when deprecated include_parent_milestones is true' do + let(:params) { { include_parent_milestones: true } } - it_behaves_like 'listing all milestones' - end + it_behaves_like 'listing all milestones' + end - context 'when both include_parent_milestones and include_ancestors are specified' do - let(:params) { { include_ancestors: true, include_parent_milestones: true } } + context 'when both include_parent_milestones and include_ancestors are specified' do + let(:params) { { include_ancestors: true, include_parent_milestones: true } } - it 'returns 400' do - get api(route, user), params: params + it 'returns 400' do + get api(route, user), params: params - expect(response).to have_gitlab_http_status(:bad_request) + expect(response).to have_gitlab_http_status(:bad_request) + end end - end - context 'when iids param is present' do - let(:params) { { include_ancestors: true, iids: [milestone.iid] } } + context 'when iids param is present' do + let(:params) { { include_ancestors: true, iids: [milestone.iid] } } - it_behaves_like 'listing all milestones' - end + it_behaves_like 'listing all milestones' + end - context 'when updated_before param is present' do - let(:params) { { updated_before: 1.day.ago.iso8601, include_ancestors: true } } + context 'when updated_before param is present' do + let(:params) { { updated_before: 1.day.ago.iso8601, include_ancestors: true } } - it_behaves_like 'listing all milestones' do - let(:milestones) { [ancestor_group_milestone, milestone] } + it_behaves_like 'listing all milestones' do + let(:milestones) { [ancestor_group_milestone, milestone] } + end + end + + context 'when updated_after param is present' do + let(:params) { { updated_after: 1.day.ago.iso8601, include_ancestors: true } } + + it_behaves_like 'listing all milestones' do + let(:milestones) { [closed_milestone] } + end end end - context 'when updated_after param is present' do - let(:params) { { updated_after: 1.day.ago.iso8601, include_ancestors: true } } + context 'when user has no access to ancestor groups' do + let(:user) { create(:user) } + + before do + group.add_guest(user) + end it_behaves_like 'listing all milestones' do - let(:milestones) { [closed_milestone] } + let(:milestones) { [milestone, closed_milestone] } end end end - context 'when user has no access to ancestor groups' do - let(:user) { create(:user) } - - before do - group.add_guest(user) - end + context 'when updated_before param is present' do + let(:params) { { updated_before: 1.day.ago.iso8601 } } it_behaves_like 'listing all milestones' do - let(:milestones) { [milestone, closed_milestone] } + let(:milestones) { [milestone] } end end - end - context 'when updated_before param is present' do - let(:params) { { updated_before: 1.day.ago.iso8601 } } + context 'when updated_after param is present' do + let(:params) { { updated_after: 1.day.ago.iso8601 } } - it_behaves_like 'listing all milestones' do - let(:milestones) { [milestone] } + it_behaves_like 'listing all milestones' do + let(:milestones) { [closed_milestone] } + end end end - context 'when updated_after param is present' do - let(:params) { { updated_after: 1.day.ago.iso8601 } } + context 'for common GraphQL/REST' do + it_behaves_like 'group milestones including ancestors and descendants' + + def query_group_milestone_ids(params) + get api(route, current_user), params: params - it_behaves_like 'listing all milestones' do - let(:milestones) { [closed_milestone] } + json_response.pluck('id') end end end diff --git a/spec/requests/api/ml_model_packages_spec.rb b/spec/requests/api/ml_model_packages_spec.rb index 3166298b430..894127cac78 100644 --- a/spec/requests/api/ml_model_packages_spec.rb +++ b/spec/requests/api/ml_model_packages_spec.rb @@ -16,6 +16,8 @@ RSpec.describe ::API::MlModelPackages, feature_category: :mlops do let_it_be(:deploy_token) { create(:deploy_token, read_package_registry: true, write_package_registry: true) } let_it_be(:project_deploy_token) { create(:project_deploy_token, deploy_token: deploy_token, project: project) } let_it_be(:another_project, reload: true) { create(:project) } + let_it_be(:model) { create(:ml_models, user: project.owner, project: project) } + let_it_be(:model_version) { create(:ml_model_versions, :with_package, model: model, version: '0.1.0') } let_it_be(:tokens) do { @@ -70,10 +72,6 @@ RSpec.describe ::API::MlModelPackages, feature_category: :mlops do :private | :guest | false | :job_token | true | :not_found :private | :developer | false | :job_token | false | :unauthorized :private | :guest | false | :job_token | false | :unauthorized - :public | :developer | true | :deploy_token | true | :success - :public | :developer | true | :deploy_token | false | :unauthorized - :private | :developer | true | :deploy_token | true | :success - :private | :developer | true | :deploy_token | false | :unauthorized end # :visibility, :user_role, :member, :token_type, :valid_token, :expected_status @@ -112,10 +110,6 @@ RSpec.describe ::API::MlModelPackages, feature_category: :mlops do :private | :guest | false | :job_token | true | :not_found :private | :developer | false | :job_token | false | :unauthorized :private | :guest | false | :job_token | false | :unauthorized - :public | :developer | true | :deploy_token | true | :success - :public | :developer | true | :deploy_token | false | :unauthorized - :private | :developer | true | :deploy_token | true | :success - :private | :developer | true | :deploy_token | false | :unauthorized end # rubocop:enable Metrics/AbcSize end @@ -128,14 +122,15 @@ RSpec.describe ::API::MlModelPackages, feature_category: :mlops do include_context 'ml model authorize permissions table' let(:token) { tokens[:personal_access_token] } - let(:user_headers) { { 'HTTP_AUTHORIZATION' => token } } + let(:user_headers) { { 'Authorization' => "Bearer #{token}" } } let(:headers) { user_headers.merge(workhorse_headers) } let(:request) { authorize_upload_file(headers) } - let(:model_name) { 'my_package' } + let(:model_name) { model_version.name } + let(:version) { model_version.version } let(:file_name) { 'myfile.tar.gz' } subject(:api_response) do - url = "/projects/#{project.id}/packages/ml_models/#{model_name}/0.0.1/#{file_name}/authorize" + url = "/projects/#{project.id}/packages/ml_models/#{model_name}/#{version}/#{file_name}/authorize" put api(url), headers: headers @@ -149,7 +144,7 @@ RSpec.describe ::API::MlModelPackages, feature_category: :mlops do with_them do let(:token) { valid_token ? tokens[token_type] : 'invalid-token123' } - let(:user_headers) { user_role == :anonymous ? {} : { 'HTTP_AUTHORIZATION' => token } } + let(:user_headers) { user_role == :anonymous ? {} : { 'Authorization' => "Bearer #{token}" } } before do project.update_column(:visibility_level, Gitlab::VisibilityLevel.level_value(visibility.to_s)) @@ -183,15 +178,16 @@ RSpec.describe ::API::MlModelPackages, feature_category: :mlops do let_it_be(:file_name) { 'model.md5' } let(:token) { tokens[:personal_access_token] } - let(:user_headers) { { 'HTTP_AUTHORIZATION' => token } } + let(:user_headers) { { 'Authorization' => "Bearer #{token}" } } let(:headers) { user_headers.merge(workhorse_headers) } let(:params) { { file: temp_file(file_name) } } let(:file_key) { :file } let(:send_rewritten_field) { true } - let(:model_name) { 'my_package' } + let(:model_name) { model_version.name } + let(:version) { model_version.version } subject(:api_response) do - url = "/projects/#{project.id}/packages/ml_models/#{model_name}/0.0.1/#{file_name}" + url = "/projects/#{project.id}/packages/ml_models/#{model_name}/#{version}/#{file_name}" workhorse_finalize( api(url), @@ -219,7 +215,7 @@ RSpec.describe ::API::MlModelPackages, feature_category: :mlops do with_them do let(:token) { valid_token ? tokens[token_type] : 'invalid-token123' } - let(:user_headers) { user_role == :anonymous ? {} : { 'HTTP_AUTHORIZATION' => token } } + let(:user_headers) { user_role == :anonymous ? {} : { 'Authorization' => "Bearer #{token}" } } before do project.update_column(:visibility_level, Gitlab::VisibilityLevel.level_value(visibility.to_s)) @@ -233,25 +229,27 @@ RSpec.describe ::API::MlModelPackages, feature_category: :mlops do end it_behaves_like 'Endpoint not found if read_model_registry not available' + it_behaves_like 'Endpoint not found if write_model_registry not available' + it_behaves_like 'Not found when model version does not exist' end end describe 'GET /api/v4/projects/:project_id/packages/ml_models/:model_name/:model_version/:file_name' do include_context 'ml model authorize permissions table' - let_it_be(:package) { create(:ml_model_package, project: project, name: 'model', version: '0.0.1') } + let_it_be(:package) { model_version.package } let_it_be(:package_file) { create(:package_file, :generic, package: package, file_name: 'model.md5') } - let(:model_name) { package.name } - let(:model_version) { package.version } + let(:model_name) { model_version.name } + let(:version) { model_version.version } let(:file_name) { package_file.file_name } let(:token) { tokens[:personal_access_token] } - let(:user_headers) { { 'HTTP_AUTHORIZATION' => token } } + let(:user_headers) { { 'Authorization' => "Bearer #{token}" } } let(:headers) { user_headers.merge(workhorse_headers) } subject(:api_response) do - url = "/projects/#{project.id}/packages/ml_models/#{model_name}/#{model_version}/#{file_name}" + url = "/projects/#{project.id}/packages/ml_models/#{model_name}/#{version}/#{file_name}" get api(url), headers: headers @@ -265,7 +263,7 @@ RSpec.describe ::API::MlModelPackages, feature_category: :mlops do with_them do let(:token) { valid_token ? tokens[token_type] : 'invalid-token123' } - let(:user_headers) { user_role == :anonymous ? {} : { 'HTTP_AUTHORIZATION' => token } } + let(:user_headers) { user_role == :anonymous ? {} : { 'Authorization' => "Bearer #{token}" } } before do project.update_column(:visibility_level, Gitlab::VisibilityLevel.level_value(visibility.to_s)) diff --git a/spec/services/ml/find_or_create_model_version_service_spec.rb b/spec/services/ml/find_or_create_model_version_service_spec.rb index e5ca7c3a450..88647f23ad9 100644 --- a/spec/services/ml/find_or_create_model_version_service_spec.rb +++ b/spec/services/ml/find_or_create_model_version_service_spec.rb @@ -35,21 +35,29 @@ RSpec.describe ::Ml::FindOrCreateModelVersionService, feature_category: :mlops d end end - context 'when model version does not exist' do + context 'when model does not exist' do let(:project) { existing_version.project } let(:name) { 'a_new_model' } let(:version) { '2.0.0' } + + it 'does not create a new model version', :aggregate_failures do + expect { model_version }.to change { Ml::ModelVersion.count }.by(0) + end + end + + context 'when model exists and model version does not' do + let(:project) { existing_version.project } + let(:name) { existing_version.name } + let(:version) { '2.0.0' } let(:description) { 'A model version' } let(:package) { create(:ml_model_package, project: project, name: name, version: version) } it 'creates a new model version', :aggregate_failures do - expect { model_version }.to change { Ml::ModelVersion.count }.by(1).and change { Ml::Candidate.count }.by(1) + expect { model_version }.to change { Ml::ModelVersion.count }.by(1) - expect(model_version.name).to eq(name) expect(model_version.version).to eq(version) - expect(model_version.package).to eq(package) - expect(model_version.candidate.model_version_id).to eq(model_version.id) + expect(model_version.model).to eq(existing_version.model) expect(model_version.description).to eq(description) end end diff --git a/spec/services/packages/ml_model/create_package_file_service_spec.rb b/spec/services/packages/ml_model/create_package_file_service_spec.rb index 30a6bedd07b..505c8038976 100644 --- a/spec/services/packages/ml_model/create_package_file_service_spec.rb +++ b/spec/services/packages/ml_model/create_package_file_service_spec.rb @@ -8,6 +8,8 @@ RSpec.describe Packages::MlModel::CreatePackageFileService, feature_category: :m let_it_be(:user) { create(:user) } let_it_be(:pipeline) { create(:ci_pipeline, user: user, project: project) } let_it_be(:file_name) { 'myfile.tar.gz.1' } + let_it_be(:model) { create(:ml_models, user: user, project: project) } + let_it_be(:model_version) { create(:ml_model_versions, :with_package, model: model, version: '0.1.0') } let(:build) { instance_double(Ci::Build, pipeline: pipeline) } @@ -26,47 +28,24 @@ RSpec.describe Packages::MlModel::CreatePackageFileService, feature_category: :m FileUtils.rm_f(temp_file) end - context 'without existing package' do + context 'when model version is nil' do let(:params) do { - package_name: 'new_model', - package_version: '1.0.0', + model_version: nil, file: file, file_name: file_name } end - it 'creates package file', :aggregate_failures do - expect { execute_service } - .to change { Packages::MlModel::Package.count }.by(1) - .and change { Packages::PackageFile.count }.by(1) - .and change { Packages::PackageFileBuildInfo.count }.by(0) - .and change { Ml::ModelVersion.count }.by(1) - - new_model = Packages::MlModel::Package.last - package_file = new_model.package_files.last - new_model_version = Ml::ModelVersion.last - - expect(new_model.name).to eq('new_model') - expect(new_model.version).to eq('1.0.0') - expect(new_model.status).to eq('default') - expect(package_file.package).to eq(new_model) - expect(package_file.file_name).to eq(file_name) - expect(package_file.size).to eq(file.size) - expect(package_file.file_sha256).to eq(sha256) - expect(new_model_version.name).to eq('new_model') - expect(new_model_version.version).to eq('1.0.0') - expect(new_model_version.package).to eq(new_model) + it 'does not create package file', :aggregate_failures do + expect(execute_service).to be(nil) end end - context 'with existing package' do - let_it_be(:model) { create(:ml_model_package, creator: user, project: project, version: '0.1.0') } - + context 'with existing model version' do let(:params) do { - package_name: model.name, - package_version: model.version, + model_version: model_version, file: file, file_name: file_name, status: :hidden, @@ -76,18 +55,16 @@ RSpec.describe Packages::MlModel::CreatePackageFileService, feature_category: :m it 'adds the package file and updates status and ci_build', :aggregate_failures do expect { execute_service } - .to change { project.packages.ml_model.count }.by(0) - .and change { model.package_files.count }.by(1) + .to change { model_version.package.package_files.count }.by(1) .and change { Packages::PackageFileBuildInfo.count }.by(1) - model.reload - - package_file = model.package_files.last + package = model_version.reload.package + package_file = package.package_files.last - expect(model.build_infos.first.pipeline).to eq(build.pipeline) - expect(model.status).to eq('hidden') + expect(package.build_infos.first.pipeline).to eq(build.pipeline) + expect(package.status).to eq('hidden') - expect(package_file.package).to eq(model) + expect(package_file.package).to eq(package) expect(package_file.file_name).to eq(file_name) expect(package_file.size).to eq(file.size) expect(package_file.file_sha256).to eq(sha256) diff --git a/spec/support/shared_examples/requests/api/graphql_rest/milestones_shared_examples.rb b/spec/support/shared_examples/requests/api/graphql_rest/milestones_shared_examples.rb new file mode 100644 index 00000000000..8e147f43091 --- /dev/null +++ b/spec/support/shared_examples/requests/api/graphql_rest/milestones_shared_examples.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +# Examples for both GraphQL and REST APIs +RSpec.shared_examples 'group milestones including ancestors and descendants' do + context 'for group milestones' do + let_it_be(:current_user) { create(:user) } + + context 'when including descendant milestones in a public group' do + let_it_be(:group) { create(:group, :public) } + + let(:params) { { include_descendants: true } } + + it 'finds milestones only in accessible projects and groups' do + accessible_group = create(:group, :private, parent: group) + accessible_project = create(:project, group: accessible_group) + accessible_group.add_developer(current_user) + inaccessible_group = create(:group, :private, parent: group) + inaccessible_project = create(:project, :private, group: group) + milestone1 = create(:milestone, group: group) + milestone2 = create(:milestone, group: accessible_group) + milestone3 = create(:milestone, project: accessible_project) + create(:milestone, group: inaccessible_group) + create(:milestone, project: inaccessible_project) + + milestone_ids = query_group_milestone_ids(params) + + expect(milestone_ids).to match_array([milestone1, milestone2, milestone3].pluck(:id)) + end + end + + describe 'include_descendants and include_ancestors' do + let_it_be(:parent_group) { create(:group, :public) } + let_it_be(:group) { create(:group, :public, parent: parent_group) } + let_it_be(:accessible_group) { create(:group, :private, parent: group) } + let_it_be(:accessible_project) { create(:project, group: accessible_group) } + let_it_be(:inaccessible_group) { create(:group, :private, parent: group) } + let_it_be(:inaccessible_project) { create(:project, :private, group: group) } + let_it_be(:milestone1) { create(:milestone, group: group) } + let_it_be(:milestone2) { create(:milestone, group: accessible_group) } + let_it_be(:milestone3) { create(:milestone, project: accessible_project) } + let_it_be(:milestone4) { create(:milestone, group: inaccessible_group) } + let_it_be(:milestone5) { create(:milestone, project: inaccessible_project) } + let_it_be(:milestone6) { create(:milestone, group: parent_group) } + + before_all do + accessible_group.add_developer(current_user) + end + + context 'when including neither ancestor nor descendant milestones in a public group' do + let(:params) { {} } + + it 'finds milestones only in accessible projects and groups' do + expect(query_group_milestone_ids(params)).to match_array([milestone1.id]) + end + end + + context 'when including descendant milestones in a public group' do + let(:params) { { include_descendants: true } } + + it 'finds milestones only in accessible projects and groups' do + expect(query_group_milestone_ids(params)).to match_array([milestone1, milestone2, milestone3].pluck(:id)) + end + end + + context 'when including ancestor milestones in a public group' do + let(:params) { { include_ancestors: true } } + + it 'finds milestones only in accessible projects and groups' do + expect(query_group_milestone_ids(params)).to match_array([milestone1, milestone6].pluck(:id)) + end + end + + context 'when including both ancestor and descendant milestones in a public group' do + let(:params) { { include_descendants: true, include_ancestors: true } } + + it 'finds milestones only in accessible projects and groups' do + expect(query_group_milestone_ids(params)) + .to match_array([milestone1, milestone2, milestone3, milestone6].pluck(:id)) + end + end + end + end +end diff --git a/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb b/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb index 47cbd268a65..30a1398bf94 100644 --- a/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/ml_model_packages_shared_examples.rb @@ -15,11 +15,31 @@ RSpec.shared_examples 'Endpoint not found if read_model_registry not available' end end -RSpec.shared_examples 'creates model experiments package files' do +RSpec.shared_examples 'Endpoint not found if write_model_registry not available' do + context 'when write_model_registry is disabled for current project' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?) + .with(user, :write_model_registry, project) + .and_return(false) + end + + it { is_expected.to have_gitlab_http_status(:not_found) } + end +end + +RSpec.shared_examples 'Not found when model version does not exist' do + context 'when model version does not exist' do + let(:version) { "#{non_existing_record_id}.0.0" } + + it { is_expected.to have_gitlab_http_status(:not_found) } + end +end + +RSpec.shared_examples 'creates package files for model versions' do it 'creates package files', :aggregate_failures do expect { api_response } - .to change { project.packages.count }.by(1) - .and change { Packages::PackageFile.count }.by(1) + .to change { Packages::PackageFile.count }.by(1) expect(api_response).to have_gitlab_http_status(:created) package_file = project.packages.last.package_files.reload.last @@ -59,7 +79,7 @@ RSpec.shared_examples 'process ml model package upload' do context 'with correct params' do it_behaves_like 'package workhorse uploads' - it_behaves_like 'creates model experiments package files' + it_behaves_like 'creates package files for model versions' # To be reactivated with https://gitlab.com/gitlab-org/gitlab/-/issues/414270 # it_behaves_like 'a package tracking event', '::API::MlModelPackages', 'push_package' end @@ -81,7 +101,7 @@ RSpec.shared_examples 'process ml model package upload' do stub_package_file_object_storage(direct_upload: true) end - it_behaves_like 'creates model experiments package files' + it_behaves_like 'creates package files for model versions' ['123123', '../../123123'].each do |remote_id| context "with invalid remote_id: #{remote_id}" do @@ -102,7 +122,7 @@ RSpec.shared_examples 'process ml model package upload' do stub_package_file_object_storage(direct_upload: false) end - it_behaves_like 'creates model experiments package files' + it_behaves_like 'creates package files for model versions' end end end @@ -112,13 +132,5 @@ RSpec.shared_examples 'process ml model package download' do it { is_expected.to have_gitlab_http_status(:success) } end - context 'when record does not exist' do - it 'response is not found' do - expect_next_instance_of(::Packages::MlModel::PackageFinder) do |instance| - expect(instance).to receive(:execute!).and_raise(ActiveRecord::RecordNotFound) - end - - expect(api_response).to have_gitlab_http_status(:not_found) - end - end + it_behaves_like 'Not found when model version does not exist' end diff --git a/spec/tooling/danger/project_helper_spec.rb b/spec/tooling/danger/project_helper_spec.rb index 2da90ddbd67..a41aba17f56 100644 --- a/spec/tooling/danger/project_helper_spec.rb +++ b/spec/tooling/danger/project_helper_spec.rb @@ -8,7 +8,7 @@ require 'gitlab/dangerfiles/spec_helper' require_relative '../../../danger/plugins/project_helper' -RSpec.describe Tooling::Danger::ProjectHelper do +RSpec.describe Tooling::Danger::ProjectHelper, feature_category: :tooling do include StubENV include_context "with dangerfile" @@ -130,6 +130,7 @@ RSpec.describe Tooling::Danger::ProjectHelper do 'lib/gitlab/background_migration.rb' | [:database, :backend] 'lib/gitlab/background_migration/foo' | [:database, :backend] 'ee/lib/gitlab/background_migration/foo' | [:database, :backend] + 'ee/lib/ee/gitlab/background_migration/foo' | [:database, :backend] 'lib/gitlab/database.rb' | [:database, :backend] 'lib/gitlab/database/foo' | [:database, :backend] 'ee/lib/gitlab/database/foo' | [:database, :backend] @@ -238,7 +239,7 @@ RSpec.describe Tooling::Danger::ProjectHelper do it { is_expected.to eq(expected_categories) } end - context 'having specific changes' do + context 'when having specific changes' do where(:expected_categories, :patch, :changed_files) do [:analytics_instrumentation] | '+data-track-action' | ['components/welcome.vue'] [:analytics_instrumentation] | '+ data: { track_label:' | ['admin/groups/_form.html.haml'] -- cgit v1.2.3