From 2ebd699ede8f213f6e8f21ba7d1d9904197b2984 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 28 Oct 2022 12:11:31 +0000 Subject: Add latest changes from gitlab-org/gitlab@master --- .../markdown/render_sandboxed_mermaid_spec.js | 145 ++++++++++++++---- .../issues/list/components/issues_list_app_spec.js | 56 ++----- spec/frontend/notebook/cells/markdown_spec.js | 101 ++++++++----- spec/frontend/notebook/cells/output/index_spec.js | 14 ++ .../notebook/cells/output/markdown_spec.js | 44 ++++++ spec/frontend/notebook/mock_data.js | 2 + .../releases/components/asset_links_form_spec.js | 42 +++++- spec/graphql/graphql_triggers_spec.rb | 69 +++++---- spec/graphql/types/subscription_type_spec.rb | 1 + .../hashie_mash_permitted_patch_spec.rb | 29 ++++ .../duplicate_jobs/cookie_spec.rb | 130 ---------------- .../duplicate_jobs/duplicate_job_spec.rb | 164 +++++++++++++-------- spec/services/issues/update_service_spec.rb | 29 +++- spec/services/work_items/update_service_spec.rb | 28 ++++ 14 files changed, 516 insertions(+), 338 deletions(-) create mode 100644 spec/frontend/notebook/cells/output/markdown_spec.js create mode 100644 spec/frontend/notebook/mock_data.js create mode 100644 spec/initializers/hashie_mash_permitted_patch_spec.rb delete mode 100644 spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/cookie_spec.rb (limited to 'spec') diff --git a/spec/frontend/behaviors/markdown/render_sandboxed_mermaid_spec.js b/spec/frontend/behaviors/markdown/render_sandboxed_mermaid_spec.js index 2b9442162aa..de0e5063e49 100644 --- a/spec/frontend/behaviors/markdown/render_sandboxed_mermaid_spec.js +++ b/spec/frontend/behaviors/markdown/render_sandboxed_mermaid_spec.js @@ -1,34 +1,127 @@ -import $ from 'jquery'; +import { createWrapper } from '@vue/test-utils'; +import { __ } from '~/locale'; import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures'; -import renderMermaid from '~/behaviors/markdown/render_sandboxed_mermaid'; +import renderMermaid, { + MAX_CHAR_LIMIT, + MAX_MERMAID_BLOCK_LIMIT, + LAZY_ALERT_SHOWN_CLASS, +} from '~/behaviors/markdown/render_sandboxed_mermaid'; -describe('Render mermaid diagrams for Gitlab Flavoured Markdown', () => { - it('Does something', () => { - document.body.dataset.page = ''; - setHTMLFixture(` -
- 
-          
-            graph TD;
-            A-->B
-            A-->C
-            B-->D
-            C-->D
-          
-
- - - -
`); - const els = $('pre.js-syntax-highlight').find('.js-render-mermaid'); - - renderMermaid(els); +describe('Mermaid diagrams renderer', () => { + // Finders + const findMermaidIframes = () => document.querySelectorAll('iframe[src="/-/sandbox/mermaid"]'); + const findDangerousMermaidAlert = () => + createWrapper(document.querySelector('[data-testid="alert-warning"]')); + // Helpers + const renderDiagrams = () => { + renderMermaid([...document.querySelectorAll('.js-render-mermaid')]); jest.runAllTimers(); - expect(document.querySelector('pre.js-syntax-highlight').classList).toContain('gl-sr-only'); + }; + + beforeEach(() => { + document.body.dataset.page = ''; + }); + afterEach(() => { resetHTMLFixture(); }); + + it('renders a mermaid diagram', () => { + setHTMLFixture('
'); + + expect(findMermaidIframes()).toHaveLength(0); + + renderDiagrams(); + + expect(document.querySelector('pre').classList).toContain('gl-sr-only'); + expect(findMermaidIframes()).toHaveLength(1); + }); + + describe('within a details element', () => { + beforeEach(() => { + setHTMLFixture('
'); + renderDiagrams(); + }); + + it('does not render the diagram on load', () => { + expect(findMermaidIframes()).toHaveLength(0); + }); + + it('render the diagram when the details element is opened', () => { + document.querySelector('details').setAttribute('open', true); + document.querySelector('details').dispatchEvent(new Event('toggle')); + jest.runAllTimers(); + + expect(findMermaidIframes()).toHaveLength(1); + }); + }); + + describe('dangerous diagrams', () => { + describe(`when the diagram's source exceeds ${MAX_CHAR_LIMIT} characters`, () => { + beforeEach(() => { + setHTMLFixture( + `
+            ${Array(MAX_CHAR_LIMIT + 1)
+              .fill('a')
+              .join('')}
+
`, + ); + renderDiagrams(); + }); + it('does not render the diagram on load', () => { + expect(findMermaidIframes()).toHaveLength(0); + }); + + it('shows a warning about performance impact when rendering the diagram', () => { + expect(document.querySelector('pre').classList).toContain(LAZY_ALERT_SHOWN_CLASS); + expect(findDangerousMermaidAlert().exists()).toBe(true); + expect(findDangerousMermaidAlert().text()).toContain( + __('Warning: Displaying this diagram might cause performance issues on this page.'), + ); + }); + + it("renders the diagram when clicking on the alert's button", () => { + findDangerousMermaidAlert().find('button').trigger('click'); + jest.runAllTimers(); + + expect(findMermaidIframes()).toHaveLength(1); + }); + }); + + it(`stops rendering diagrams once the total rendered source exceeds ${MAX_CHAR_LIMIT} characters`, () => { + setHTMLFixture( + `
+          ${Array(MAX_CHAR_LIMIT - 1)
+            .fill('a')
+            .join('')}
+          2
+          3
+          4
+
`, + ); + renderDiagrams(); + + expect(findMermaidIframes()).toHaveLength(3); + }); + + // Note: The test case below is provided for convenience but should remain skipped as the DOM + // operations it requires are too expensive and would significantly slow down the test suite. + // eslint-disable-next-line jest/no-disabled-tests + it.skip(`stops rendering diagrams when the rendered diagrams count exceeds ${MAX_MERMAID_BLOCK_LIMIT}`, () => { + setHTMLFixture( + `
+          ${Array(MAX_MERMAID_BLOCK_LIMIT + 1)
+            .fill('')
+            .join('')}
+
`, + ); + renderDiagrams(); + + expect([...document.querySelectorAll('.js-render-mermaid')]).toHaveLength( + MAX_MERMAID_BLOCK_LIMIT + 1, + ); + expect(findMermaidIframes()).toHaveLength(MAX_MERMAID_BLOCK_LIMIT); + }); + }); }); diff --git a/spec/frontend/issues/list/components/issues_list_app_spec.js b/spec/frontend/issues/list/components/issues_list_app_spec.js index 5133c02b190..16631752d6d 100644 --- a/spec/frontend/issues/list/components/issues_list_app_spec.js +++ b/spec/frontend/issues/list/components/issues_list_app_spec.js @@ -131,7 +131,6 @@ describe('CE IssuesListApp component', () => { const mountComponent = ({ provide = {}, data = {}, - workItems = false, issuesQueryResponse = mockIssuesQueryResponse, issuesCountsQueryResponse = mockIssuesCountsQueryResponse, sortPreferenceMutationResponse = jest.fn().mockResolvedValue(setSortPreferenceMutationResponse), @@ -150,9 +149,6 @@ describe('CE IssuesListApp component', () => { apolloProvider: createMockApollo(requestHandlers), router, provide: { - glFeatures: { - workItems, - }, ...defaultProvide, ...provide, }, @@ -1060,45 +1056,23 @@ describe('CE IssuesListApp component', () => { }); describe('fetching issues', () => { - describe('when work_items feature flag is disabled', () => { - beforeEach(() => { - wrapper = mountComponent({ workItems: false }); - jest.runOnlyPendingTimers(); - }); - - it('fetches issue, incident, and test case types', () => { - const types = [ - WORK_ITEM_TYPE_ENUM_ISSUE, - WORK_ITEM_TYPE_ENUM_INCIDENT, - WORK_ITEM_TYPE_ENUM_TEST_CASE, - ]; - - expect(mockIssuesQueryResponse).toHaveBeenCalledWith(expect.objectContaining({ types })); - expect(mockIssuesCountsQueryResponse).toHaveBeenCalledWith( - expect.objectContaining({ types }), - ); - }); + beforeEach(() => { + wrapper = mountComponent(); + jest.runOnlyPendingTimers(); }); - describe('when work_items feature flag is enabled', () => { - beforeEach(() => { - wrapper = mountComponent({ workItems: true }); - jest.runOnlyPendingTimers(); - }); - - it('fetches issue, incident, test case, and task types', () => { - const types = [ - WORK_ITEM_TYPE_ENUM_ISSUE, - WORK_ITEM_TYPE_ENUM_INCIDENT, - WORK_ITEM_TYPE_ENUM_TEST_CASE, - WORK_ITEM_TYPE_ENUM_TASK, - ]; - - expect(mockIssuesQueryResponse).toHaveBeenCalledWith(expect.objectContaining({ types })); - expect(mockIssuesCountsQueryResponse).toHaveBeenCalledWith( - expect.objectContaining({ types }), - ); - }); + it('fetches issue, incident, test case, and task types', () => { + const types = [ + WORK_ITEM_TYPE_ENUM_ISSUE, + WORK_ITEM_TYPE_ENUM_INCIDENT, + WORK_ITEM_TYPE_ENUM_TEST_CASE, + WORK_ITEM_TYPE_ENUM_TASK, + ]; + + expect(mockIssuesQueryResponse).toHaveBeenCalledWith(expect.objectContaining({ types })); + expect(mockIssuesCountsQueryResponse).toHaveBeenCalledWith( + expect.objectContaining({ types }), + ); }); }); }); diff --git a/spec/frontend/notebook/cells/markdown_spec.js b/spec/frontend/notebook/cells/markdown_spec.js index c757b55faf4..a7776bd5b69 100644 --- a/spec/frontend/notebook/cells/markdown_spec.js +++ b/spec/frontend/notebook/cells/markdown_spec.js @@ -5,20 +5,22 @@ import markdownTableJson from 'test_fixtures/blob/notebook/markdown-table.json'; import basicJson from 'test_fixtures/blob/notebook/basic.json'; import mathJson from 'test_fixtures/blob/notebook/math.json'; import MarkdownComponent from '~/notebook/cells/markdown.vue'; +import Prompt from '~/notebook/cells/prompt.vue'; const Component = Vue.extend(MarkdownComponent); window.katex = katex; -function buildCellComponent(cell, relativePath = '') { +function buildCellComponent(cell, relativePath = '', hidePrompt) { return mount(Component, { propsData: { cell, + hidePrompt, }, provide: { relativeRawPath: relativePath, }, - }).vm; + }); } function buildMarkdownComponent(markdownContent, relativePath = '') { @@ -33,7 +35,7 @@ function buildMarkdownComponent(markdownContent, relativePath = '') { } describe('Markdown component', () => { - let vm; + let wrapper; let cell; let json; @@ -43,21 +45,30 @@ describe('Markdown component', () => { // eslint-disable-next-line prefer-destructuring cell = json.cells[1]; - vm = buildCellComponent(cell); + wrapper = buildCellComponent(cell); await nextTick(); }); - it('does not render prompt', () => { - expect(vm.$el.querySelector('.prompt span')).toBeNull(); + const findPrompt = () => wrapper.findComponent(Prompt); + + it('renders a prompt by default', () => { + expect(findPrompt().exists()).toBe(true); + }); + + it('does not render a prompt if hidePrompt is true', () => { + wrapper = buildCellComponent(cell, '', true); + expect(findPrompt().exists()).toBe(false); }); it('does not render the markdown text', () => { - expect(vm.$el.querySelector('.markdown').innerHTML.trim()).not.toEqual(cell.source.join('')); + expect(wrapper.vm.$el.querySelector('.markdown').innerHTML.trim()).not.toEqual( + cell.source.join(''), + ); }); it('renders the markdown HTML', () => { - expect(vm.$el.querySelector('.markdown h1')).not.toBeNull(); + expect(wrapper.vm.$el.querySelector('.markdown h1')).not.toBeNull(); }); it('sanitizes Markdown output', async () => { @@ -68,11 +79,11 @@ describe('Markdown component', () => { }); await nextTick(); - expect(vm.$el.querySelector('a').getAttribute('href')).toBeNull(); + expect(wrapper.vm.$el.querySelector('a').getAttribute('href')).toBeNull(); }); it('sanitizes HTML', async () => { - const findLink = () => vm.$el.querySelector('.xss-link'); + const findLink = () => wrapper.vm.$el.querySelector('.xss-link'); Object.assign(cell, { source: ['XSS\n'], }); @@ -97,11 +108,11 @@ describe('Markdown component', () => { ["for embedded images, it doesn't", '![](data:image/jpeg;base64)\n', 'src="data:'], ["for images urls, it doesn't", '![](http://image.png)\n', 'src="http:'], ])('%s', async ([testMd, mustContain]) => { - vm = buildMarkdownComponent([testMd], '/raw/'); + wrapper = buildMarkdownComponent([testMd], '/raw/'); await nextTick(); - expect(vm.$el.innerHTML).toContain(mustContain); + expect(wrapper.vm.$el.innerHTML).toContain(mustContain); }); }); @@ -111,13 +122,13 @@ describe('Markdown component', () => { }); it('renders images and text', async () => { - vm = buildCellComponent(json.cells[0]); + wrapper = buildCellComponent(json.cells[0]); await nextTick(); - const images = vm.$el.querySelectorAll('img'); + const images = wrapper.vm.$el.querySelectorAll('img'); expect(images.length).toBe(5); - const columns = vm.$el.querySelectorAll('td'); + const columns = wrapper.vm.$el.querySelectorAll('td'); expect(columns.length).toBe(6); expect(columns[0].textContent).toEqual('Hello '); @@ -141,81 +152,93 @@ describe('Markdown component', () => { }); it('renders multi-line katex', async () => { - vm = buildCellComponent(json.cells[0]); + wrapper = buildCellComponent(json.cells[0]); await nextTick(); - expect(vm.$el.querySelector('.katex')).not.toBeNull(); + expect(wrapper.vm.$el.querySelector('.katex')).not.toBeNull(); }); it('renders inline katex', async () => { - vm = buildCellComponent(json.cells[1]); + wrapper = buildCellComponent(json.cells[1]); await nextTick(); - expect(vm.$el.querySelector('p:first-child .katex')).not.toBeNull(); + expect(wrapper.vm.$el.querySelector('p:first-child .katex')).not.toBeNull(); }); it('renders multiple inline katex', async () => { - vm = buildCellComponent(json.cells[1]); + wrapper = buildCellComponent(json.cells[1]); await nextTick(); - expect(vm.$el.querySelectorAll('p:nth-child(2) .katex')).toHaveLength(4); + expect(wrapper.vm.$el.querySelectorAll('p:nth-child(2) .katex')).toHaveLength(4); }); it('output cell in case of katex error', async () => { - vm = buildMarkdownComponent(['Some invalid $a & b$ inline formula $b & c$\n', '\n']); + wrapper = buildMarkdownComponent(['Some invalid $a & b$ inline formula $b & c$\n', '\n']); await nextTick(); // expect one paragraph with no katex formula in it - expect(vm.$el.querySelectorAll('p')).toHaveLength(1); - expect(vm.$el.querySelectorAll('p .katex')).toHaveLength(0); + expect(wrapper.vm.$el.querySelectorAll('p')).toHaveLength(1); + expect(wrapper.vm.$el.querySelectorAll('p .katex')).toHaveLength(0); }); it('output cell and render remaining formula in case of katex error', async () => { - vm = buildMarkdownComponent([ + wrapper = buildMarkdownComponent([ 'An invalid $a & b$ inline formula and a vaild one $b = c$\n', '\n', ]); await nextTick(); // expect one paragraph with no katex formula in it - expect(vm.$el.querySelectorAll('p')).toHaveLength(1); - expect(vm.$el.querySelectorAll('p .katex')).toHaveLength(1); + expect(wrapper.vm.$el.querySelectorAll('p')).toHaveLength(1); + expect(wrapper.vm.$el.querySelectorAll('p .katex')).toHaveLength(1); }); it('renders math formula in list object', async () => { - vm = buildMarkdownComponent(["- list with inline $a=2$ inline formula $a' + b = c$\n", '\n']); + wrapper = buildMarkdownComponent([ + "- list with inline $a=2$ inline formula $a' + b = c$\n", + '\n', + ]); await nextTick(); // expect one list with a katex formula in it - expect(vm.$el.querySelectorAll('li')).toHaveLength(1); - expect(vm.$el.querySelectorAll('li .katex')).toHaveLength(2); + expect(wrapper.vm.$el.querySelectorAll('li')).toHaveLength(1); + expect(wrapper.vm.$el.querySelectorAll('li .katex')).toHaveLength(2); }); it("renders math formula with tick ' in it", async () => { - vm = buildMarkdownComponent(["- list with inline $a=2$ inline formula $a' + b = c$\n", '\n']); + wrapper = buildMarkdownComponent([ + "- list with inline $a=2$ inline formula $a' + b = c$\n", + '\n', + ]); await nextTick(); // expect one list with a katex formula in it - expect(vm.$el.querySelectorAll('li')).toHaveLength(1); - expect(vm.$el.querySelectorAll('li .katex')).toHaveLength(2); + expect(wrapper.vm.$el.querySelectorAll('li')).toHaveLength(1); + expect(wrapper.vm.$el.querySelectorAll('li .katex')).toHaveLength(2); }); it('renders math formula with less-than-operator < in it', async () => { - vm = buildMarkdownComponent(['- list with inline $a=2$ inline formula $a + b < c$\n', '\n']); + wrapper = buildMarkdownComponent([ + '- list with inline $a=2$ inline formula $a + b < c$\n', + '\n', + ]); await nextTick(); // expect one list with a katex formula in it - expect(vm.$el.querySelectorAll('li')).toHaveLength(1); - expect(vm.$el.querySelectorAll('li .katex')).toHaveLength(2); + expect(wrapper.vm.$el.querySelectorAll('li')).toHaveLength(1); + expect(wrapper.vm.$el.querySelectorAll('li .katex')).toHaveLength(2); }); it('renders math formula with greater-than-operator > in it', async () => { - vm = buildMarkdownComponent(['- list with inline $a=2$ inline formula $a + b > c$\n', '\n']); + wrapper = buildMarkdownComponent([ + '- list with inline $a=2$ inline formula $a + b > c$\n', + '\n', + ]); await nextTick(); // expect one list with a katex formula in it - expect(vm.$el.querySelectorAll('li')).toHaveLength(1); - expect(vm.$el.querySelectorAll('li .katex')).toHaveLength(2); + expect(wrapper.vm.$el.querySelectorAll('li')).toHaveLength(1); + expect(wrapper.vm.$el.querySelectorAll('li .katex')).toHaveLength(2); }); }); }); diff --git a/spec/frontend/notebook/cells/output/index_spec.js b/spec/frontend/notebook/cells/output/index_spec.js index 8bf049235a9..585cbb68eeb 100644 --- a/spec/frontend/notebook/cells/output/index_spec.js +++ b/spec/frontend/notebook/cells/output/index_spec.js @@ -1,12 +1,15 @@ import { mount } from '@vue/test-utils'; import json from 'test_fixtures/blob/notebook/basic.json'; import Output from '~/notebook/cells/output/index.vue'; +import MarkdownOutput from '~/notebook/cells/output/markdown.vue'; +import { relativeRawPath, markdownCellContent } from '../../mock_data'; describe('Output component', () => { let wrapper; const createComponent = (output) => { wrapper = mount(Output, { + provide: { relativeRawPath }, propsData: { outputs: [].concat(output), count: 1, @@ -95,6 +98,17 @@ describe('Output component', () => { }); }); + describe('Markdown output', () => { + beforeEach(() => { + const markdownType = { data: { 'text/markdown': markdownCellContent } }; + createComponent(markdownType); + }); + + it('renders a markdown component', () => { + expect(wrapper.findComponent(MarkdownOutput).props('rawCode')).toBe(markdownCellContent); + }); + }); + describe('default to plain text', () => { beforeEach(() => { const unknownType = json.cells[6]; diff --git a/spec/frontend/notebook/cells/output/markdown_spec.js b/spec/frontend/notebook/cells/output/markdown_spec.js new file mode 100644 index 00000000000..e3490ed3bea --- /dev/null +++ b/spec/frontend/notebook/cells/output/markdown_spec.js @@ -0,0 +1,44 @@ +import { mount } from '@vue/test-utils'; +import MarkdownOutput from '~/notebook/cells/output/markdown.vue'; +import Prompt from '~/notebook/cells/prompt.vue'; +import Markdown from '~/notebook/cells/markdown.vue'; +import { relativeRawPath, markdownCellContent } from '../../mock_data'; + +describe('markdown output cell', () => { + let wrapper; + + const createComponent = ({ count = 0, index = 0 } = {}) => { + wrapper = mount(MarkdownOutput, { + provide: { relativeRawPath }, + propsData: { + rawCode: markdownCellContent, + count, + index, + }, + }); + }; + + beforeEach(() => { + createComponent(); + }); + + const findPrompt = () => wrapper.findComponent(Prompt); + const findMarkdown = () => wrapper.findComponent(Markdown); + + it.each` + index | count | showOutput + ${0} | ${1} | ${true} + ${1} | ${2} | ${false} + ${2} | ${3} | ${false} + `('renders a prompt', ({ index, count, showOutput }) => { + createComponent({ count, index }); + expect(findPrompt().props()).toMatchObject({ count, showOutput, type: 'Out' }); + }); + + it('renders a Markdown component', () => { + expect(findMarkdown().props()).toMatchObject({ + cell: { source: markdownCellContent }, + hidePrompt: true, + }); + }); +}); diff --git a/spec/frontend/notebook/mock_data.js b/spec/frontend/notebook/mock_data.js new file mode 100644 index 00000000000..b1419e1256f --- /dev/null +++ b/spec/frontend/notebook/mock_data.js @@ -0,0 +1,2 @@ +export const relativeRawPath = '/test'; +export const markdownCellContent = ['# Test']; diff --git a/spec/frontend/releases/components/asset_links_form_spec.js b/spec/frontend/releases/components/asset_links_form_spec.js index 1ff5766b074..b1e9d8d1256 100644 --- a/spec/frontend/releases/components/asset_links_form_spec.js +++ b/spec/frontend/releases/components/asset_links_form_spec.js @@ -292,6 +292,42 @@ describe('Release edit component', () => { }); }); + describe('remove button state', () => { + describe('when there is only one link', () => { + beforeEach(() => { + factory({ + release: { + ...release, + assets: { + links: release.assets.links.slice(0, 1), + }, + }, + }); + }); + + it('remove asset link button should not be present', () => { + expect(wrapper.find('.remove-button').exists()).toBe(false); + }); + }); + + describe('when there are multiple links', () => { + beforeEach(() => { + factory({ + release: { + ...release, + assets: { + links: release.assets.links.slice(0, 2), + }, + }, + }); + }); + + it('remove asset link button should be visible', () => { + expect(wrapper.find('.remove-button').exists()).toBe(true); + }); + }); + }); + describe('empty state', () => { describe('when the release fetched from the API has no links', () => { beforeEach(() => { @@ -325,12 +361,6 @@ describe('Release edit component', () => { it('does not call the addEmptyAssetLink store method when the component is created', () => { expect(actions.addEmptyAssetLink).not.toHaveBeenCalled(); }); - - it('calls addEmptyAssetLink when the final link is deleted by the user', () => { - wrapper.find('.remove-button').vm.$emit('click'); - - expect(actions.addEmptyAssetLink).toHaveBeenCalledTimes(1); - }); }); }); }); diff --git a/spec/graphql/graphql_triggers_spec.rb b/spec/graphql/graphql_triggers_spec.rb index a4a643582f5..a54cb8a7988 100644 --- a/spec/graphql/graphql_triggers_spec.rb +++ b/spec/graphql/graphql_triggers_spec.rb @@ -3,76 +3,89 @@ require 'spec_helper' RSpec.describe GraphqlTriggers do + let_it_be(:issuable, refind: true) { create(:work_item) } + describe '.issuable_assignees_updated' do - it 'triggers the issuableAssigneesUpdated subscription' do - assignees = create_list(:user, 2) - issue = create(:issue, assignees: assignees) + let(:assignees) { create_list(:user, 2) } + before do + issuable.update!(assignees: assignees) + end + + it 'triggers the issuableAssigneesUpdated subscription' do expect(GitlabSchema.subscriptions).to receive(:trigger).with( 'issuableAssigneesUpdated', - { issuable_id: issue.to_gid }, - issue + { issuable_id: issuable.to_gid }, + issuable ) - GraphqlTriggers.issuable_assignees_updated(issue) + GraphqlTriggers.issuable_assignees_updated(issuable) end end describe '.issuable_title_updated' do it 'triggers the issuableTitleUpdated subscription' do - work_item = create(:work_item) - expect(GitlabSchema.subscriptions).to receive(:trigger).with( 'issuableTitleUpdated', - { issuable_id: work_item.to_gid }, - work_item + { issuable_id: issuable.to_gid }, + issuable ).and_call_original - GraphqlTriggers.issuable_title_updated(work_item) + GraphqlTriggers.issuable_title_updated(issuable) end end describe '.issuable_description_updated' do it 'triggers the issuableDescriptionUpdated subscription' do - work_item = create(:work_item) - expect(GitlabSchema.subscriptions).to receive(:trigger).with( 'issuableDescriptionUpdated', - { issuable_id: work_item.to_gid }, - work_item + { issuable_id: issuable.to_gid }, + issuable ).and_call_original - GraphqlTriggers.issuable_description_updated(work_item) + GraphqlTriggers.issuable_description_updated(issuable) end end describe '.issuable_labels_updated' do - it 'triggers the issuableLabelsUpdated subscription' do - project = create(:project) - labels = create_list(:label, 3, project: project) - issue = create(:issue, labels: labels) + let(:labels) { create_list(:label, 3, project: create(:project)) } + + before do + issuable.update!(labels: labels) + end + it 'triggers the issuableLabelsUpdated subscription' do expect(GitlabSchema.subscriptions).to receive(:trigger).with( 'issuableLabelsUpdated', - { issuable_id: issue.to_gid }, - issue + { issuable_id: issuable.to_gid }, + issuable ) - GraphqlTriggers.issuable_labels_updated(issue) + GraphqlTriggers.issuable_labels_updated(issuable) end end describe '.issuable_dates_updated' do it 'triggers the issuableDatesUpdated subscription' do - work_item = create(:work_item) - expect(GitlabSchema.subscriptions).to receive(:trigger).with( 'issuableDatesUpdated', - { issuable_id: work_item.to_gid }, - work_item + { issuable_id: issuable.to_gid }, + issuable + ).and_call_original + + GraphqlTriggers.issuable_dates_updated(issuable) + end + end + + describe '.issuable_milestone_updated' do + it 'triggers the issuableMilestoneUpdated subscription' do + expect(GitlabSchema.subscriptions).to receive(:trigger).with( + 'issuableMilestoneUpdated', + { issuable_id: issuable.to_gid }, + issuable ).and_call_original - GraphqlTriggers.issuable_dates_updated(work_item) + GraphqlTriggers.issuable_milestone_updated(issuable) end end diff --git a/spec/graphql/types/subscription_type_spec.rb b/spec/graphql/types/subscription_type_spec.rb index c23a14deaf3..04f0c72b06f 100644 --- a/spec/graphql/types/subscription_type_spec.rb +++ b/spec/graphql/types/subscription_type_spec.rb @@ -11,6 +11,7 @@ RSpec.describe GitlabSchema.types['Subscription'] do issuable_description_updated issuable_labels_updated issuable_dates_updated + issuable_milestone_updated merge_request_reviewers_updated merge_request_merge_status_updated ] diff --git a/spec/initializers/hashie_mash_permitted_patch_spec.rb b/spec/initializers/hashie_mash_permitted_patch_spec.rb new file mode 100644 index 00000000000..0e9f8a485ff --- /dev/null +++ b/spec/initializers/hashie_mash_permitted_patch_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Hashie::Mash#permitted patch' do + let(:mash) { Hashie::Mash.new } + + before do + load Rails.root.join('config/initializers/hashie_mash_permitted_patch.rb') + end + + describe '#respond_to? with :permitted?' do + it 'returns false' do + expect(Gitlab::AppLogger).to receive(:info).with( + { message: 'Hashie::Mash#respond_to?(:permitted?)', caller: instance_of(Array) }) + + expect(mash.respond_to?(:permitted?)).to be false + end + end + + describe '#permitted' do + it 'raises ArgumentError' do + expect(Gitlab::AppLogger).to receive(:info).with( + { message: 'Hashie::Mash#permitted?', caller: instance_of(Array) }) + + expect { mash.permitted? }.to raise_error(ArgumentError) + end + end +end diff --git a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/cookie_spec.rb b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/cookie_spec.rb deleted file mode 100644 index 10e642b7713..00000000000 --- a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/cookie_spec.rb +++ /dev/null @@ -1,130 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::Cookie, :clean_gitlab_redis_shared_state, -:clean_gitlab_redis_queues do - describe 'serialization' do - it 'can round-trip a hash' do - h = { 'hello' => 'world', 'foo' => 'bar' } - expect(described_class.deserialize(described_class.serialize(h))).to eq(h) - end - - it 'can merge by concatenating' do - h1 = { 'foo' => 'bar', 'baz' => 'qux' } - h2 = { 'foo' => 'other bar', 'hello' => 'world' } - concatenated = described_class.serialize(h1) + described_class.serialize(h2) - expect(described_class.deserialize(concatenated)).to eq(h1.merge(h2)) - end - end - - shared_examples 'with Redis persistence' do - let(:cookie) { described_class.new(key) } - let(:key) { 'redis_key' } - let(:hash) { { 'hello' => 'world' } } - - describe '.set' do - subject { cookie.set(hash, expiry) } - - let(:expiry) { 10 } - - it 'stores the hash' do - expect(subject).to be_truthy - with_redis do |redis| - expect(redis.get(key)).to eq("hello=world\n") - expect(redis.ttl(key)).to be_within(1).of(expiry) - end - end - - context 'when the key is set' do - before do - with_redis { |r| r.set(key, 'foobar') } - end - - it 'does not overwrite existing keys' do - expect(subject).to be_falsey - with_redis do |redis| - expect(redis.get(key)).to eq('foobar') - expect(redis.ttl(key)).to eq(-1) - end - end - end - end - - describe '.get' do - subject { cookie.get } - - it { expect(subject).to eq({}) } - - context 'when the key is set' do - before do - with_redis { |r| r.set(key, "hello=world\n") } - end - - it { expect(subject).to eq({ 'hello' => 'world' }) } - end - end - - describe '.append' do - subject { cookie.append(hash) } - - it 'does not create the key' do - subject - - with_redis do |redis| - expect(redis.get(key)).to eq(nil) - end - end - - context 'when the key exists' do - before do - with_redis { |r| r.set(key, 'existing data', ex: 10) } - end - - it 'appends without modifying ttl' do - subject - - with_redis do |redis| - expect(redis.get(key)).to eq("existing datahello=world\n") - expect(redis.ttl(key)).to be_within(1).of(10) - end - end - end - end - end - - context 'with multi-store feature flags turned on' do - def with_redis(&block) - Gitlab::Redis::DuplicateJobs.with(&block) - end - - it 'use Gitlab::Redis::DuplicateJobs.with' do - expect(Gitlab::Redis::DuplicateJobs).to receive(:with).and_call_original - expect(Sidekiq).not_to receive(:redis) - - described_class.new('hello').get - end - - it_behaves_like 'with Redis persistence' - end - - context 'when both multi-store feature flags are off' do - def with_redis(&block) - Sidekiq.redis(&block) - end - - before do - stub_feature_flags(use_primary_and_secondary_stores_for_duplicate_jobs: false) - stub_feature_flags(use_primary_store_as_default_for_duplicate_jobs: false) - end - - it 'use Sidekiq.redis' do - expect(Sidekiq).to receive(:redis).and_call_original - expect(Gitlab::Redis::DuplicateJobs).not_to receive(:with) - - described_class.new('hello').get - end - - it_behaves_like 'with Redis persistence' - end -end diff --git a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb index abe083f911b..1e8cb76905b 100644 --- a/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb +++ b/spec/lib/gitlab/sidekiq_middleware/duplicate_jobs/duplicate_job_spec.rb @@ -527,9 +527,8 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi end context 'with Redis cookies' do - let(:cookie_key) do - "#{idempotency_key}:cookie" - end + let(:cookie_key) { "#{idempotency_key}:cookie" } + let(:cookie) { get_redis_msgpack(cookie_key) } def with_redis(&block) Gitlab::Redis::DuplicateJobs.with(&block) @@ -541,15 +540,16 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi shared_examples 'sets Redis keys with correct TTL' do it "adds an idempotency key with correct ttl" do - expected_cookie = <<~COOKIE - jid=123 - existing_wal_location:main=#{wal_locations['main']} - existing_wal_location:ci=#{wal_locations['ci']} - COOKIE - expect { duplicate_job.check! } - .to change { read_idempotency_key_with_ttl(cookie_key) } - .from([nil, -2]) - .to([expected_cookie, be_within(1).of(expected_ttl)]) + expected_cookie = { + 'jid' => '123', + 'offsets' => {}, + 'wal_locations' => {}, + 'existing_wal_locations' => wal_locations + } + + duplicate_job.check! + expect(cookie).to eq(expected_cookie) + expect(redis_ttl(cookie_key)).to be_within(1).of(expected_ttl) end end @@ -576,15 +576,17 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi context 'when there was already a job with same arguments in the same queue' do before do - set_idempotency_key(cookie_key, "jid=existing-jid\n") + set_idempotency_key(cookie_key, existing_cookie.to_msgpack) end + let(:existing_cookie) { { 'jid' => 'existing-jid' } } + it { expect(duplicate_job.check!).to eq('existing-jid') } it "does not change the existing key's TTL" do expect { duplicate_job.check! } - .not_to change { read_idempotency_key_with_ttl(cookie_key) } - .from(["jid=existing-jid\n", -1]) + .not_to change { redis_ttl(cookie_key) } + .from(-1) end it 'sets the existing jid' do @@ -601,20 +603,22 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi { main: ::ActiveRecord::Base, ci: ::ActiveRecord::Base }) - set_idempotency_key(cookie_key, initial_cookie) + with_redis { |r| r.set(cookie_key, initial_cookie.to_msgpack, ex: expected_ttl) } # read existing_wal_locations duplicate_job.check! end let(:initial_cookie) do - <<~COOKIE - jid=foobar - existing_wal_location:main=0/D525E3A0 - existing_wal_location:ci=AB/12340 - COOKIE + { + 'jid' => 'foobar', + 'existing_wal_locations' => { 'main' => '0/D525E3A0', 'ci' => 'AB/12340' }, + 'offsets' => {}, + 'wal_locations' => {} + } end + let(:expected_ttl) { 123 } let(:new_wal) do { # offset is relative to `existing_wal` @@ -626,34 +630,76 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi let(:wal_locations) { new_wal.transform_values { |v| v[:location] } } it 'stores a wal location to redis with an offset relative to existing wal location' do - expected_cookie = initial_cookie + <<~COOKIE - wal_location:main:#{new_wal['main'][:offset]}=#{new_wal['main'][:location]} - wal_location:ci:#{new_wal['ci'][:offset]}=#{new_wal['ci'][:location]} - COOKIE + duplicate_job.update_latest_wal_location! + + expect(cookie['wal_locations']).to eq(wal_locations) + expect(cookie['offsets']).to eq(new_wal.transform_values { |v| v[:offset].to_i }) + expect(redis_ttl(cookie_key)).to be_within(1).of(expected_ttl) + end + end + + describe 'UPDATE_WAL_COOKIE_SCRIPT' do + subject do + with_redis do |redis| + redis.eval(described_class::UPDATE_WAL_COOKIE_SCRIPT, keys: [cookie_key], argv: argv) + end + end + + let(:argv) { ['c1', 1, 'loc1', 'c2', 2, 'loc2', 'c3', 3, 'loc3'] } + + it 'does not create the key' do + subject + + expect(with_redis { |r| r.get(cookie_key) }).to eq(nil) + end + + context 'when the key exists' do + let(:existing_cookie) { { 'offsets' => {}, 'wal_locations' => {} } } + let(:expected_ttl) { 123 } + + before do + with_redis { |r| r.set(cookie_key, existing_cookie.to_msgpack, ex: expected_ttl) } + end + + it 'updates all connections' do + subject + + expect(cookie['wal_locations']).to eq({ 'c1' => 'loc1', 'c2' => 'loc2', 'c3' => 'loc3' }) + expect(cookie['offsets']).to eq({ 'c1' => 1, 'c2' => 2, 'c3' => 3 }) + end + + it 'preserves the ttl' do + subject + + expect(redis_ttl(cookie_key)).to be_within(1).of(expected_ttl) + end - expect { duplicate_job.update_latest_wal_location! } - .to change { read_idempotency_key_with_ttl(cookie_key) } - .from([initial_cookie, -1]) - .to([expected_cookie, -1]) + context 'and low offsets' do + let(:existing_cookie) do + { + 'offsets' => { 'c1' => 0, 'c2' => 2 }, + 'wal_locations' => { 'c1' => 'loc1old', 'c2' => 'loc2old' } + } + end + + it 'updates only some connections' do + subject + + expect(cookie['wal_locations']).to eq({ 'c1' => 'loc1', 'c2' => 'loc2old', 'c3' => 'loc3' }) + expect(cookie['offsets']).to eq({ 'c1' => 1, 'c2' => 2, 'c3' => 3 }) + end + end end end describe '#latest_wal_locations' do context 'when job was deduplicated and wal locations were already persisted' do before do - cookie = <<~COOKIE - jid=foobar - wal_location:main:1=main1 - wal_location:ci:2:=ci2 - wal_location:main:5=main5 - wal_location:ci:6=ci6 - wal_location:main:3=main3 - wal_location:ci:4=ci4 - COOKIE + cookie = { 'wal_locations' => { 'main' => 'abc', 'ci' => 'def' } }.to_msgpack set_idempotency_key(cookie_key, cookie) end - it { expect(duplicate_job.latest_wal_locations).to eq({ 'main' => 'main5', 'ci' => 'ci6' }) } + it { expect(duplicate_job.latest_wal_locations).to eq({ 'main' => 'abc', 'ci' => 'def' }) } end context 'when job is not deduplication and wal locations were not persisted' do @@ -668,30 +714,22 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi context 'when the key exists in redis' do before do - set_idempotency_key(cookie_key, "jid=existing-jid\n") + set_idempotency_key(cookie_key, "garbage") end shared_examples 'deleting the duplicate job' do shared_examples 'deleting keys from redis' do |key_name| it "removes the #{key_name} from redis" do expect { duplicate_job.delete! } - .to change { read_idempotency_key_with_ttl(key) } - .from([from_value, -1]) - .to([nil, -2]) - end - end - - shared_examples 'does not delete key from redis' do |key_name| - it "does not remove the #{key_name} from redis" do - expect { duplicate_job.delete! } - .to not_change { read_idempotency_key_with_ttl(key) } - .from([from_value, -1]) + .to change { with_redis { |r| r.get(key) } } + .from(from_value) + .to(nil) end end it_behaves_like 'deleting keys from redis', 'cookie key' do let(:key) { cookie_key } - let(:from_value) { "jid=existing-jid\n" } + let(:from_value) { "garbage" } end end @@ -730,8 +768,7 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi it 'sets the key in Redis' do duplicate_job.set_deduplicated_flag! - cookie = with_redis { |redis| redis.get(cookie_key) } - expect(cookie).to include("\ndeduplicated=1\n") + expect(cookie['deduplicated']).to eq('1') end it 'sets, gets and cleans up the deduplicated flag' do @@ -754,9 +791,7 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi duplicate_job.check! duplicate_job.set_deduplicated_flag! - cookie = with_redis { |redis| redis.get(cookie_key) } - - expect(cookie).not_to include('deduplicated=') + expect(cookie['deduplicated']).to eq(nil) end it 'does not set the deduplicated flag' do @@ -783,7 +818,7 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi end it 'returns true if the existing jid is different from the job jid' do - set_idempotency_key(cookie_key, "jid=a different jid\n") + set_idempotency_key(cookie_key, { 'jid' => 'a different jid' }.to_msgpack) duplicate_job.check! expect(duplicate_job.duplicate?).to be(true) @@ -794,13 +829,12 @@ RSpec.describe Gitlab::SidekiqMiddleware::DuplicateJobs::DuplicateJob, :clean_gi with_redis { |r| r.set(key, value) } end - def read_idempotency_key_with_ttl(key) - with_redis do |redis| - redis.pipelined do |p| - p.get(key) - p.ttl(key) - end - end + def get_redis_msgpack(key) + MessagePack.unpack(with_redis { |redis| redis.get(key) }) + end + + def redis_ttl(key) + with_redis { |redis| redis.ttl(key) } end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 20b1a1f58bb..ebc870406e8 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -104,10 +104,33 @@ RSpec.describe Issues::UpdateService, :mailer do expect(issue.issue_customer_relations_contacts.last.contact).to eq contact end - it 'updates issue milestone when passing `milestone` param' do - update_issue(milestone: milestone) + context 'when updating milestone' do + before do + update_issue({ milestone: nil }) + end - expect(issue.milestone).to eq milestone + it 'updates issue milestone when passing `milestone` param' do + expect { update_issue({ milestone: milestone }) } + .to change(issue, :milestone).to(milestone).from(nil) + end + + it "triggers 'issuableMilestoneUpdated'" do + expect(GraphqlTriggers).to receive(:issuable_milestone_updated).with(issue).and_call_original + + update_issue({ milestone: milestone }) + end + + context 'when milestone remains unchanged' do + before do + update_issue({ title: 'abc', milestone: milestone }) + end + + it "does not trigger 'issuableMilestoneUpdated'" do + expect(GraphqlTriggers).not_to receive(:issuable_milestone_updated) + + update_issue({ milestone: milestone }) + end + end end context 'when sentry identifier is given' do diff --git a/spec/services/work_items/update_service_spec.rb b/spec/services/work_items/update_service_spec.rb index 1761d1104dd..68efb4c220b 100644 --- a/spec/services/work_items/update_service_spec.rb +++ b/spec/services/work_items/update_service_spec.rb @@ -311,6 +311,34 @@ RSpec.describe WorkItems::UpdateService do end end end + + context 'for milestone widget' do + let_it_be(:milestone) { create(:milestone, project: project) } + + let(:widget_params) { { milestone_widget: { milestone_id: milestone.id } } } + + context 'when milestone is updated' do + it "triggers 'issuableMilestoneUpdated'" do + expect(work_item.milestone).to eq(nil) + expect(GraphqlTriggers).to receive(:issuable_milestone_updated).with(work_item).and_call_original + + update_work_item + end + end + + context 'when milestone remains unchanged' do + before do + update_work_item + end + + it "does not trigger 'issuableMilestoneUpdated'" do + expect(work_item.milestone).to eq(milestone) + expect(GraphqlTriggers).not_to receive(:issuable_milestone_updated) + + update_work_item + end + end + end end describe 'label updates' do -- cgit v1.2.3