Commit 727e4c33 authored by Miguel Rincon's avatar Miguel Rincon

Add improvements from maintainer review

- Use mock apollo resolvers to mock Api responses
- Use mount to test forms components
- Refactor error handling in the app
parent 36bfdb8a
<script>
import {
GlButton,
GlForm,
GlFormGroup,
GlFormTextarea,
GlFormCheckbox,
GlFormInput,
GlButton,
GlFormGroup,
GlFormTextarea,
GlSprintf,
} from '@gitlab/ui';
import { __ } from '~/locale';
export default {
components: {
GlButton,
GlForm,
GlFormGroup,
GlFormTextarea,
GlFormCheckbox,
GlFormInput,
GlButton,
GlFormGroup,
GlFormTextarea,
GlSprintf,
},
props: {
defaultBranch: {
type: String,
required: true,
required: false,
default: '',
},
defaultMessage: {
type: String,
......@@ -44,8 +45,11 @@ export default {
};
},
computed: {
isDefaultBranch() {
return this.branch === this.defaultBranch;
},
submitDisabled() {
return !this.message || !this.branch;
return !(this.message && this.branch);
},
},
methods: {
......@@ -101,8 +105,9 @@ export default {
required
/>
<gl-form-checkbox
v-if="branch !== defaultBranch"
v-if="!isDefaultBranch"
v-model="openMergeRequest"
data-testid="new-mr-checkbox"
class="gl-mt-3"
>
<gl-sprintf :message="$options.i18n.startMergeRequest">
......
......@@ -10,7 +10,11 @@ import PipelineEditorApp from './pipeline_editor_app.vue';
export const initPipelineEditor = (selector = '#js-pipeline-editor') => {
const el = document.querySelector(selector);
const { projectPath, defaultBranch, commitId, ciConfigPath, newMergeRequestPath } = el?.dataset;
if (!el) {
return null;
}
const { ciConfigPath, commitId, defaultBranch, newMergeRequestPath, projectPath } = el?.dataset;
Vue.use(VueApollo);
......@@ -24,11 +28,11 @@ export const initPipelineEditor = (selector = '#js-pipeline-editor') => {
render(h) {
return h(PipelineEditorApp, {
props: {
projectPath,
defaultBranch,
commitId,
ciConfigPath,
commitId,
defaultBranch,
newMergeRequestPath,
projectPath,
},
});
},
......
<script>
import { GlLoadingIcon, GlAlert, GlTabs, GlTab } from '@gitlab/ui';
import { GlAlert, GlLoadingIcon, GlTab, GlTabs } from '@gitlab/ui';
import { __, s__, sprintf } from '~/locale';
import { redirectTo, mergeUrlParams, refreshCurrentPage } from '~/lib/utils/url_utility';
import TextEditor from './components/text_editor.vue';
import CommitForm from './components/commit/commit_form.vue';
import PipelineGraph from '~/pipelines/components/pipeline_graph/pipeline_graph.vue';
import commitCIFileMutation from './graphql/mutations/commit_ci_file.mutation.graphql';
import CommitForm from './components/commit/commit_form.vue';
import TextEditor from './components/text_editor.vue';
import commitCiFileMutation from './graphql/mutations/commit_ci_file.mutation.graphql';
import getBlobContent from './graphql/queries/blob_content.graphql';
const MR_SOURCE_BRANCH = 'merge_request[source_branch]';
const MR_TARGET_BRANCH = 'merge_request[target_branch]';
const LOAD_FAILURE_NO_REF = 'LOAD_FAILURE_NO_REF';
const LOAD_FAILURE_NO_FILE = 'LOAD_FAILURE_NO_FILE';
const LOAD_FAILURE_UNKNOWN = 'LOAD_FAILURE_UNKNOWN';
const COMMIT_FAILURE = 'COMMIT_FAILURE';
const DEFAULT_FAILURE = 'DEFAULT_FAILURE';
export default {
components: {
GlLoadingIcon,
GlAlert,
GlTabs,
GlLoadingIcon,
GlTab,
TextEditor,
CommitForm,
GlTabs,
PipelineGraph,
CommitForm,
TextEditor,
},
props: {
projectPath: {
......@@ -49,7 +55,10 @@ export default {
},
data() {
return {
errorMessage: null,
showFailureAlert: false,
failureType: null,
failureReasons: [],
isSaving: false,
editorIsReady: false,
content: '',
......@@ -78,7 +87,7 @@ export default {
},
},
computed: {
loading() {
isLoading() {
return this.$apollo.queries.content.loading;
},
defaultCommitMessage() {
......@@ -88,27 +97,71 @@ export default {
// Note data will loaded as part of https://gitlab.com/gitlab-org/gitlab/-/issues/263141
return {};
},
failure() {
switch (this.failureType) {
case LOAD_FAILURE_NO_REF:
return {
text: this.$options.errorTexts[LOAD_FAILURE_NO_REF],
variant: 'danger',
};
case LOAD_FAILURE_NO_FILE:
return {
text: this.$options.errorTexts[LOAD_FAILURE_NO_FILE],
variant: 'danger',
};
case LOAD_FAILURE_UNKNOWN:
return {
text: this.$options.errorTexts[LOAD_FAILURE_UNKNOWN],
variant: 'danger',
};
case COMMIT_FAILURE:
return {
text: this.$options.errorTexts[COMMIT_FAILURE],
variant: 'danger',
};
default:
return {
text: this.$options.errorTexts[DEFAULT_FAILURE],
variant: 'danger',
};
}
},
},
i18n: {
defaultCommitMessage: __('Update %{sourcePath} file'),
tabEdit: s__('Pipelines|Write pipeline configuration'),
tabGraph: s__('Pipelines|Visualize'),
unknownError: __('Unknown Error'),
fetchErrorMsg: s__('Pipelines|CI file could not be loaded: %{reason}'),
commitErrorMsg: s__('Pipelines|CI file could not be saved: %{reason}'),
},
errorTexts: {
[LOAD_FAILURE_NO_REF]: s__(
'Pipelines|Repository does not have a default branch, please set one.',
),
[LOAD_FAILURE_NO_FILE]: s__('Pipelines|No CI file found in this repository, please add one.'),
[LOAD_FAILURE_UNKNOWN]: s__('Pipelines|The CI configuration was not loaded, please try again.'),
[COMMIT_FAILURE]: s__('Pipelines|The GitLab CI configuration could not be updated.'),
},
methods: {
handleBlobContentError(error) {
const { message: generalReason, networkError } = error;
handleBlobContentError(error = {}) {
const { networkError } = error;
const { data } = networkError?.response ?? {};
// 404 for missing file uses `message`
// 400 for a missing ref uses `error`
const networkReason = data?.message ?? data?.error;
const reason = networkReason ?? generalReason ?? this.$options.i18n.unknownError;
this.errorMessage = sprintf(this.$options.i18n.fetchErrorMsg, { reason });
const { response } = networkError;
if (response?.status === 404) {
// 404 for missing CI file
this.reportFailure(LOAD_FAILURE_NO_FILE);
} else if (response?.status === 400) {
// 400 for a missing ref when no default branch is set
this.reportFailure(LOAD_FAILURE_NO_REF);
} else {
this.reportFailure(LOAD_FAILURE_UNKNOWN);
}
},
dismissFailure() {
this.showFailureAlert = false;
},
reportFailure(type, reasons = []) {
this.showFailureAlert = true;
this.failureType = type;
this.failureReasons = reasons;
},
redirectToNewMergeRequest(sourceBranch) {
const url = mergeUrlParams(
......@@ -130,7 +183,7 @@ export default {
commitCreate: { errors },
},
} = await this.$apollo.mutate({
mutation: commitCIFileMutation,
mutation: commitCiFileMutation,
variables: {
projectPath: this.projectPath,
branch,
......@@ -143,7 +196,8 @@ export default {
});
if (errors?.length) {
throw new Error(errors[0]);
this.reportFailure(COMMIT_FAILURE, errors);
return;
}
if (openMergeRequest) {
......@@ -153,8 +207,7 @@ export default {
refreshCurrentPage();
}
} catch (error) {
const reason = error?.message || this.$options.i18n.unknownError;
this.errorMessage = sprintf(this.$options.i18n.commitErrorMsg, { reason });
this.reportFailure(COMMIT_FAILURE, [error?.message]);
} finally {
this.isSaving = false;
}
......@@ -162,20 +215,25 @@ export default {
onCommitCancel() {
this.contentModel = this.content;
},
onErrorDismiss() {
this.errorMessage = null;
},
},
};
</script>
<template>
<div class="gl-mt-4">
<gl-alert v-if="errorMessage" variant="danger" :dismissible="true" @dismiss="onErrorDismiss">
{{ errorMessage }}
<gl-alert
v-if="showFailureAlert"
:variant="failure.variant"
:dismissible="true"
@dismiss="dismissFailure()"
>
{{ failure.text }}
<ul v-if="failureReasons && failureReasons.length" class="gl-mb-0">
<li v-for="(reason, i) in failureReasons" :key="i">{{ reason }}</li>
</ul>
</gl-alert>
<div class="gl-mt-4">
<gl-loading-icon v-if="loading" size="lg" class="gl-m-3" />
<gl-loading-icon v-if="isLoading" size="lg" class="gl-m-3" />
<div v-else class="file-editor gl-mb-3">
<gl-tabs>
<!-- editor should be mounted when its tab is visible, so the container has a size -->
......
......@@ -19938,12 +19938,6 @@ msgstr ""
msgid "Pipelines|CI Lint"
msgstr ""
msgid "Pipelines|CI file could not be loaded: %{reason}"
msgstr ""
msgid "Pipelines|CI file could not be saved: %{reason}"
msgstr ""
msgid "Pipelines|Child pipeline"
msgstr ""
......@@ -19989,6 +19983,9 @@ msgstr ""
msgid "Pipelines|More Information"
msgstr ""
msgid "Pipelines|No CI file found in this repository, please add one."
msgstr ""
msgid "Pipelines|No triggers have been created yet. Add one using the form above."
msgstr ""
......@@ -20001,6 +19998,9 @@ msgstr ""
msgid "Pipelines|Project cache successfully reset."
msgstr ""
msgid "Pipelines|Repository does not have a default branch, please set one."
msgstr ""
msgid "Pipelines|Revoke"
msgstr ""
......@@ -20010,6 +20010,12 @@ msgstr ""
msgid "Pipelines|Something went wrong while cleaning runners cache."
msgstr ""
msgid "Pipelines|The CI configuration was not loaded, please try again."
msgstr ""
msgid "Pipelines|The GitLab CI configuration could not be updated."
msgstr ""
msgid "Pipelines|There are currently no finished pipelines."
msgstr ""
......
import { shallowMount } from '@vue/test-utils';
import { GlForm, GlFormTextarea, GlFormInput, GlFormCheckbox } from '@gitlab/ui';
import { shallowMount, mount } from '@vue/test-utils';
import { GlFormInput, GlFormTextarea } from '@gitlab/ui';
import CommitForm from '~/pipeline_editor/components/commit/commit_form.vue';
......@@ -15,40 +15,53 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
defaultBranch: mockDefaultBranch,
...props,
},
// attachToDocument is required for input/submit events
attachToDocument: mountFn === mount,
});
};
const findForm = () => wrapper.find(GlForm);
const findCommitTextarea = () => wrapper.find(GlFormTextarea);
const findBranchInput = () => wrapper.find(GlFormInput);
const findMrCheckbox = () => wrapper.find(GlFormCheckbox);
const findNewMrCheckbox = () => wrapper.find('[data-testid="new-mr-checkbox"]');
const findSubmitBtn = () => wrapper.find('[type="submit"]');
const findCancelBtn = () => wrapper.find('[type="reset"]');
beforeEach(() => {
createComponent();
afterEach(() => {
wrapper.destroy();
wrapper = null;
});
it('shows a default commit message', () => {
expect(findCommitTextarea().attributes('value')).toBe(mockCommitMessage);
});
describe('when the form is displayed', () => {
beforeEach(async () => {
createComponent();
});
it('shows a default branch', () => {
expect(findBranchInput().attributes('value')).toBe(mockDefaultBranch);
});
it('shows a default commit message', () => {
expect(findCommitTextarea().attributes('value')).toBe(mockCommitMessage);
});
it('shows buttons', () => {
expect(findSubmitBtn().exists()).toBe(true);
expect(findCancelBtn().exists()).toBe(true);
});
it('shows a default branch', () => {
expect(findBranchInput().attributes('value')).toBe(mockDefaultBranch);
});
it('does not show a new MR checkbox', () => {
expect(findMrCheckbox().exists()).toBe(false);
it('shows buttons', () => {
expect(findSubmitBtn().exists()).toBe(true);
expect(findCancelBtn().exists()).toBe(true);
});
it('does not show a new MR checkbox by default', () => {
expect(findNewMrCheckbox().exists()).toBe(false);
});
});
describe('events', () => {
describe('when buttons are clicked', () => {
beforeEach(async () => {
createComponent({}, mount);
});
it('emits an event when the form submits', () => {
findForm().vm.$emit('submit', new Event('submit'));
findSubmitBtn().trigger('click');
expect(wrapper.emitted('submit')[0]).toEqual([
{
......@@ -60,29 +73,30 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
});
it('emits an event when the form resets', () => {
findForm().vm.$emit('reset', new Event('reset'));
findCancelBtn().trigger('click');
expect(wrapper.emitted('cancel')).toHaveLength(1);
});
});
describe('when values change', () => {
describe('when users inputs values', () => {
const anotherMessage = 'Another commit message';
const anotherBranch = 'my-branch';
beforeEach(() => {
findCommitTextarea().vm.$emit('input', anotherMessage);
findBranchInput().vm.$emit('input', anotherBranch);
createComponent({}, mount);
findCommitTextarea().setValue(anotherMessage);
findBranchInput().setValue(anotherBranch);
});
it('shows a new MR checkbox', () => {
expect(findMrCheckbox().exists()).toBe(true);
expect(findNewMrCheckbox().exists()).toBe(true);
});
it('emits an event with other values', () => {
findMrCheckbox().vm.$emit('input', true);
findForm().vm.$emit('submit', new Event('submit'));
it('emits an event with values', async () => {
await findNewMrCheckbox().setChecked();
await findSubmitBtn().trigger('click');
expect(wrapper.emitted('submit')[0]).toEqual([
{
......@@ -93,29 +107,10 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
]);
});
describe('when values are removed', () => {
beforeEach(() => {
findBranchInput().vm.$emit('input', anotherBranch);
});
it('shows a disables the form', () => {
findCommitTextarea().vm.$emit('input', '');
expect(findMrCheckbox().exists()).toBe(true);
});
it('emits an event with other values', () => {
findMrCheckbox().vm.$emit('input', true);
findForm().vm.$emit('submit', new Event('submit'));
expect(wrapper.emitted('submit')[0]).toEqual([
{
message: anotherMessage,
branch: anotherBranch,
openMergeRequest: true,
},
]);
});
it('when the commit message is empty, submit button is disabled', async () => {
await findCommitTextarea().setValue('');
expect(findSubmitBtn().attributes('disabled')).toBe('disabled');
});
});
});
......@@ -36,7 +36,7 @@ describe('~/pipeline_editor/components/text_editor.vue', () => {
expect(findEditor().props('fileName')).toBe('*.yml');
});
it('bubble up of events', () => {
it('bubbles up events', () => {
findEditor().vm.$emit('editor-ready');
expect(editorReadyListener).toHaveBeenCalled();
......
import { nextTick } from 'vue';
import { shallowMount, createLocalVue } from '@vue/test-utils';
import { GlAlert, GlLoadingIcon, GlTabs, GlTab } from '@gitlab/ui';
import { GlButton, GlAlert, GlLoadingIcon, GlTabs, GlTab } from '@gitlab/ui';
import waitForPromises from 'helpers/wait_for_promises';
import VueApollo from 'vue-apollo';
import createMockApollo from 'jest/helpers/mock_apollo_helper';
import { redirectTo, refreshCurrentPage, objectToQuery } from '~/lib/utils/url_utility';
import {
mockProjectPath,
mockDefaultBranch,
mockCiConfigPath,
mockCommitId,
mockCiYml,
mockNewMergeRequestPath,
mockCommitId,
mockCommitMessage,
mockDefaultBranch,
mockProjectPath,
mockNewMergeRequestPath,
} from './mock_data';
import TextEditor from '~/pipeline_editor/components/text_editor.vue';
......@@ -21,6 +23,7 @@ import PipelineEditorApp from '~/pipeline_editor/pipeline_editor_app.vue';
import CommitForm from '~/pipeline_editor/components/commit/commit_form.vue';
const localVue = createLocalVue();
localVue.use(VueApollo);
jest.mock('~/lib/utils/url_utility', () => ({
redirectTo: jest.fn(),
......@@ -29,15 +32,17 @@ jest.mock('~/lib/utils/url_utility', () => ({
mergeUrlParams: jest.requireActual('~/lib/utils/url_utility').mergeUrlParams,
}));
jest.mock('~/api', () => ({
getRawFile: () => Promise.resolve(mockCiYml),
}));
describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
let wrapper;
let mockMutate;
let mockApollo;
let mockBlobContentData;
const createComponent = ({ props = {}, loading = false } = {}, mountFn = shallowMount) => {
const createComponent = (
{ props = {}, loading = false, options = {} } = {},
mountFn = shallowMount,
) => {
mockMutate = jest.fn().mockResolvedValue({
data: {
commitCreate: {
......@@ -48,17 +53,17 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
});
wrapper = mountFn(PipelineEditorApp, {
localVue,
propsData: {
projectPath: mockProjectPath,
defaultBranch: mockDefaultBranch,
ciConfigPath: mockCiConfigPath,
newMergeRequestPath: mockNewMergeRequestPath,
commitId: mockCommitId,
defaultBranch: mockDefaultBranch,
projectPath: mockProjectPath,
newMergeRequestPath: mockNewMergeRequestPath,
...props,
},
stubs: {
GlTabs,
GlButton,
TextEditor,
CommitForm,
},
......@@ -72,7 +77,29 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
mutate: mockMutate,
},
},
...options,
});
};
const createComponentWithApollo = ({ props = {} } = {}, mountFn = shallowMount) => {
mockApollo = createMockApollo([], {
Query: {
blobContent() {
return {
__typename: 'BlobContent',
rawData: mockBlobContentData(),
};
},
},
});
const options = {
localVue,
mocks: {},
apolloProvider: mockApollo,
};
createComponent({ props, options }, mountFn);
};
const findLoadingIcon = () => wrapper.find(GlLoadingIcon);
......@@ -80,14 +107,14 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
const findTabAt = i => wrapper.findAll(GlTab).at(i);
const findEditorLite = () => wrapper.find(EditorLite);
const findCommitForm = () => wrapper.find(CommitForm);
const findCommitBtnLoadingIcon = () => wrapper.find('[type="submit"]').find(GlLoadingIcon);
beforeEach(async () => {
createComponent();
// await waitForPromises();
beforeEach(() => {
mockBlobContentData = jest.fn();
});
afterEach(() => {
mockBlobContentData.mockReset();
refreshCurrentPage.mockReset();
redirectTo.mockReset();
mockMutate.mockReset();
......@@ -103,48 +130,11 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
expect(findEditorLite().exists()).toBe(false);
});
describe('handle apollo query errors', () => {
class MockError extends Error {
constructor(message, data) {
super(message);
if (data) {
this.networkError = {
response: { data },
};
}
}
}
it('sets a general error message', async () => {
wrapper.vm.handleBlobContentError(new MockError('An error'));
await nextTick();
expect(findAlert().text()).toMatch('CI file could not be loaded: An error');
});
it('sets a 404 error message', async () => {
wrapper.vm.handleBlobContentError(new MockError('Error!', { message: 'file not found' }));
await nextTick();
expect(findAlert().text()).toMatch('CI file could not be loaded: file not found');
});
it('sets a 400 error message', async () => {
wrapper.vm.handleBlobContentError(new MockError('Error!', { error: 'ref is missing' }));
await nextTick();
expect(findAlert().text()).toMatch('CI file could not be loaded: ref is missing');
});
it('sets a unkown error error message', async () => {
wrapper.vm.handleBlobContentError({ message: null });
await nextTick();
expect(findAlert().text()).toMatch('CI file could not be loaded: Unknown Error');
describe('tabs', () => {
beforeEach(() => {
createComponent();
});
});
describe('tabs', () => {
it('displays tabs and their content', async () => {
expect(
findTabAt(0)
......@@ -170,28 +160,30 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
});
describe('when data is set', () => {
beforeEach(() => {
beforeEach(async () => {
createComponent();
wrapper.setData({
content: mockCiYml,
contentModel: mockCiYml,
});
});
it('displays content after the query loads', async () => {
await nextTick();
});
it('displays content after the query loads', () => {
expect(findLoadingIcon().exists()).toBe(false);
expect(findEditorLite().props('value')).toBe(mockCiYml);
});
describe('commit form', () => {
const mockVariables = {
projectPath: mockProjectPath,
filePath: mockCiConfigPath,
content: mockCiYml,
startBranch: mockDefaultBranch,
filePath: mockCiConfigPath,
lastCommitId: mockCommitId,
message: mockCommitMessage,
projectPath: mockProjectPath,
startBranch: mockDefaultBranch,
};
const emitSubmit = event => {
......@@ -224,7 +216,7 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
});
it('shows no saving state', () => {
expect(wrapper.find('[type="submit"]').props('loading')).toBe(false);
expect(findCommitBtnLoadingIcon().exists()).toBe(false);
});
});
......@@ -275,7 +267,7 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
describe('when the commit is ocurring', () => {
it('shows a saving state', async () => {
await mockMutate.mockImplementationOnce(() => {
expect(findCommitForm().props('saving')).toBe(true);
expect(findCommitBtnLoadingIcon().exists()).toBe(true);
return Promise.resolve();
});
......@@ -295,7 +287,9 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
await waitForPromises();
expect(findAlert().text()).toBe('CI file could not be saved: commit failed');
expect(findAlert().text()).toMatchInterpolatedText(
'The GitLab CI configuration could not be updated. commit failed',
);
});
it('shows an unkown error', async () => {
......@@ -305,11 +299,13 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
await waitForPromises();
expect(findAlert().text()).toBe('CI file could not be saved: Unknown Error');
expect(findAlert().text()).toMatchInterpolatedText(
'The GitLab CI configuration could not be updated.',
);
});
});
describe('when the commit is cancelled', () => {
describe('when the commit form is cancelled', () => {
const otherContent = 'other content';
beforeEach(async () => {
......@@ -327,4 +323,52 @@ describe('~/pipeline_editor/pipeline_editor_app.vue', () => {
});
});
});
describe('displays fetch content errors', () => {
it('no error is show when data is set', async () => {
mockBlobContentData.mockResolvedValue(mockCiYml);
createComponentWithApollo();
await waitForPromises();
expect(findAlert().exists()).toBe(false);
expect(findEditorLite().props('value')).toBe(mockCiYml);
});
it('shows a 404 error message', async () => {
mockBlobContentData.mockRejectedValueOnce({
response: {
status: 404,
},
});
createComponentWithApollo();
await waitForPromises();
expect(findAlert().text()).toMatch('No CI file found in this repository, please add one.');
});
it('shows a 400 error message', async () => {
mockBlobContentData.mockRejectedValueOnce({
response: {
status: 400,
},
});
createComponentWithApollo();
await waitForPromises();
expect(findAlert().text()).toMatch(
'Repository does not have a default branch, please set one.',
);
});
it('shows a unkown error message', async () => {
mockBlobContentData.mockRejectedValueOnce(new Error('My error!'));
createComponentWithApollo();
await waitForPromises();
expect(findAlert().text()).toMatch('The CI configuration was not loaded, please try again.');
});
});
});
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment