From eb95100c066d2d70a2128ea9ac6776f720b0777a Mon Sep 17 00:00:00 2001 From: mfluharty Date: Thu, 28 Mar 2019 14:00:44 -0600 Subject: [PATCH] Make corrections to address review feedback Refactor tests to follow conventions Add XSS test Eliminate a few unnecessary lines, comments, and parameters Use Vue.set for nested state changes --- .../javascripts/lib/utils/text_utility.js | 4 +- .../project_selector/project_selector.vue | 4 +- .../components/project_list_item.scss | 3 - .../frequent_items_list_item_spec.js | 105 ++++++++---------- .../frequent_items_search_input_spec.js | 25 +++-- .../project_list_item_spec.js | 104 +++++++++-------- .../project_selector/project_selector_spec.js | 84 ++++++-------- 7 files changed, 150 insertions(+), 179 deletions(-) diff --git a/app/assets/javascripts/lib/utils/text_utility.js b/app/assets/javascripts/lib/utils/text_utility.js index 386d69ea430..1b7f8732c65 100644 --- a/app/assets/javascripts/lib/utils/text_utility.js +++ b/app/assets/javascripts/lib/utils/text_utility.js @@ -172,8 +172,8 @@ export const splitCamelCase = string => * @param {String} string A string namespace, * i.e. "My Group / My Subgroup / My Project" */ -export const truncateNamespace = string => { - if (_.isUndefined(string) || _.isNull(string) || !_.isString(string)) { +export const truncateNamespace = (string = '') => { + if (_.isNull(string) || !_.isString(string)) { return ''; } diff --git a/app/assets/javascripts/vue_shared/components/project_selector/project_selector.vue b/app/assets/javascripts/vue_shared/components/project_selector/project_selector.vue index bb17a9b331e..596fd48f96a 100644 --- a/app/assets/javascripts/vue_shared/components/project_selector/project_selector.vue +++ b/app/assets/javascripts/vue_shared/components/project_selector/project_selector.vue @@ -56,8 +56,8 @@ export default { focusSearchInput() { this.$refs.searchInput.focus(); }, - onInput: _.debounce(function debouncedOnInput(e) { - this.$emit('searched', e.target.value); + onInput: _.debounce(function debouncedOnInput() { + this.$emit('searched', this.searchQuery); }, SEARCH_INPUT_TIMEOUT_MS), }, }; diff --git a/app/assets/stylesheets/components/project_list_item.scss b/app/assets/stylesheets/components/project_list_item.scss index 6f9933e3d70..8e7c2c4398c 100644 --- a/app/assets/stylesheets/components/project_list_item.scss +++ b/app/assets/stylesheets/components/project_list_item.scss @@ -17,9 +17,6 @@ margin-left: -$gl-padding; margin-right: -$gl-padding; - // should be replaced by Bootstrap's - // .overflow-hidden utility class once - // we upgrade Bootstrap to at least 4.2.x .project-namespace-name-container { overflow: hidden; } diff --git a/spec/javascripts/frequent_items/components/frequent_items_list_item_spec.js b/spec/javascripts/frequent_items/components/frequent_items_list_item_spec.js index 92554bd9a69..f00bc2eeb6d 100644 --- a/spec/javascripts/frequent_items/components/frequent_items_list_item_spec.js +++ b/spec/javascripts/frequent_items/components/frequent_items_list_item_spec.js @@ -1,26 +1,31 @@ import Vue from 'vue'; import frequentItemsListItemComponent from '~/frequent_items/components/frequent_items_list_item.vue'; -import mountComponent from 'spec/helpers/vue_mount_component_helper'; +import { shallowMount } from '@vue/test-utils'; import { trimText } from 'spec/helpers/vue_component_helper'; import { mockProject } from '../mock_data'; // can also use 'mockGroup', but not useful to test here const createComponent = () => { const Component = Vue.extend(frequentItemsListItemComponent); - return mountComponent(Component, { - itemId: mockProject.id, - itemName: mockProject.name, - namespace: mockProject.namespace, - webUrl: mockProject.webUrl, - avatarUrl: mockProject.avatarUrl, + return shallowMount(Component, { + propsData: { + itemId: mockProject.id, + itemName: mockProject.name, + namespace: mockProject.namespace, + webUrl: mockProject.webUrl, + avatarUrl: mockProject.avatarUrl, + }, }); }; describe('FrequentItemsListItemComponent', () => { + let wrapper; let vm; beforeEach(() => { - vm = createComponent(); + wrapper = createComponent(); + + ({ vm } = wrapper); }); afterEach(() => { @@ -30,81 +35,61 @@ describe('FrequentItemsListItemComponent', () => { describe('computed', () => { describe('hasAvatar', () => { it('should return `true` or `false` if whether avatar is present or not', () => { - vm.avatarUrl = 'path/to/avatar.png'; + wrapper.setProps({ avatarUrl: 'path/to/avatar.png' }); expect(vm.hasAvatar).toBe(true); - vm.avatarUrl = null; + wrapper.setProps({ avatarUrl: null }); expect(vm.hasAvatar).toBe(false); }); }); describe('highlightedItemName', () => { - it('should enclose part of project name in & which matches with `matcher` prop', done => { - vm.matcher = 'lab'; - - vm.$nextTick() - .then(() => { - expect(vm.$el.querySelector('.js-frequent-items-item-title').innerHTML).toContain( - 'Lab', - ); - }) - .then(done) - .catch(done.fail); + it('should enclose part of project name in & which matches with `matcher` prop', () => { + wrapper.setProps({ matcher: 'lab' }); + + expect(wrapper.find('.js-frequent-items-item-title').html()).toContain( + 'Lab', + ); }); - it('should return project name as it is if `matcher` is not available', done => { - vm.matcher = null; - - vm.$nextTick() - .then(() => { - expect(vm.$el.querySelector('.js-frequent-items-item-title').innerHTML).toBe( - mockProject.name, - ); - }) - .then(done) - .catch(done.fail); + it('should return project name as it is if `matcher` is not available', () => { + wrapper.setProps({ matcher: null }); + + expect(trimText(wrapper.find('.js-frequent-items-item-title').text())).toBe( + mockProject.name, + ); }); }); describe('truncatedNamespace', () => { - it('should truncate project name from namespace string', done => { - vm.namespace = 'platform / nokia-3310'; - - vm.$nextTick() - .then(() => { - expect( - trimText(vm.$el.querySelector('.js-frequent-items-item-namespace').innerHTML), - ).toBe('platform'); - }) - .then(done) - .catch(done.fail); + it('should truncate project name from namespace string', () => { + wrapper.setProps({ namespace: 'platform / nokia-3310' }); + + expect(trimText(wrapper.find('.js-frequent-items-item-namespace').text())).toBe('platform'); }); - it('should truncate namespace string from the middle if it includes more than two groups in path', done => { - vm.namespace = 'platform / hardware / broadcom / Wifi Group / Mobile Chipset / nokia-3310'; - - vm.$nextTick() - .then(() => { - expect( - trimText(vm.$el.querySelector('.js-frequent-items-item-namespace').innerHTML), - ).toBe('platform / ... / Mobile Chipset'); - }) - .then(done) - .catch(done.fail); + it('should truncate namespace string from the middle if it includes more than two groups in path', () => { + wrapper.setProps({ + namespace: 'platform / hardware / broadcom / Wifi Group / Mobile Chipset / nokia-3310', + }); + + expect(trimText(wrapper.find('.js-frequent-items-item-namespace').text())).toBe( + 'platform / ... / Mobile Chipset', + ); }); }); }); describe('template', () => { it('should render component element', () => { - expect(vm.$el.classList.contains('frequent-items-list-item-container')).toBeTruthy(); - expect(vm.$el.querySelectorAll('a').length).toBe(1); - expect(vm.$el.querySelectorAll('.frequent-items-item-avatar-container').length).toBe(1); - expect(vm.$el.querySelectorAll('.frequent-items-item-metadata-container').length).toBe(1); - expect(vm.$el.querySelectorAll('.js-frequent-items-item-title').length).toBe(1); - expect(vm.$el.querySelectorAll('.js-frequent-items-item-namespace').length).toBe(1); + expect(wrapper.classes()).toContain('frequent-items-list-item-container'); + expect(wrapper.findAll('a').length).toBe(1); + expect(wrapper.findAll('.frequent-items-item-avatar-container').length).toBe(1); + expect(wrapper.findAll('.frequent-items-item-metadata-container').length).toBe(1); + expect(wrapper.findAll('.frequent-items-item-title').length).toBe(1); + expect(wrapper.findAll('.frequent-items-item-namespace').length).toBe(1); }); }); }); diff --git a/spec/javascripts/frequent_items/components/frequent_items_search_input_spec.js b/spec/javascripts/frequent_items/components/frequent_items_search_input_spec.js index d564292f1ba..ddbbc5c2d29 100644 --- a/spec/javascripts/frequent_items/components/frequent_items_search_input_spec.js +++ b/spec/javascripts/frequent_items/components/frequent_items_search_input_spec.js @@ -1,19 +1,22 @@ import Vue from 'vue'; import searchComponent from '~/frequent_items/components/frequent_items_search_input.vue'; import eventHub from '~/frequent_items/event_hub'; -import mountComponent from 'spec/helpers/vue_mount_component_helper'; +import { shallowMount } from '@vue/test-utils'; const createComponent = (namespace = 'projects') => { const Component = Vue.extend(searchComponent); - return mountComponent(Component, { namespace }); + return shallowMount(Component, { propsData: { namespace } }); }; describe('FrequentItemsSearchInputComponent', () => { + let wrapper; let vm; beforeEach(() => { - vm = createComponent(); + wrapper = createComponent(); + + ({ vm } = wrapper); }); afterEach(() => { @@ -35,7 +38,7 @@ describe('FrequentItemsSearchInputComponent', () => { describe('mounted', () => { it('should listen `dropdownOpen` event', done => { spyOn(eventHub, '$on'); - const vmX = createComponent(); + const vmX = createComponent().vm; Vue.nextTick(() => { expect(eventHub.$on).toHaveBeenCalledWith( @@ -49,7 +52,7 @@ describe('FrequentItemsSearchInputComponent', () => { describe('beforeDestroy', () => { it('should unbind event listeners on eventHub', done => { - const vmX = createComponent(); + const vmX = createComponent().vm; spyOn(eventHub, '$off'); vmX.$mount(); @@ -67,12 +70,12 @@ describe('FrequentItemsSearchInputComponent', () => { describe('template', () => { it('should render component element', () => { - const inputEl = vm.$el.querySelector('input.form-control'); - - expect(vm.$el.classList.contains('search-input-container')).toBeTruthy(); - expect(inputEl).not.toBe(null); - expect(inputEl.getAttribute('placeholder')).toBe('Search your projects'); - expect(vm.$el.querySelector('.search-icon')).toBeDefined(); + expect(wrapper.classes()).toContain('search-input-container'); + expect(wrapper.contains('input.form-control')).toBe(true); + expect(wrapper.contains('.search-icon')).toBe(true); + expect(wrapper.find('input.form-control').attributes('placeholder')).toBe( + 'Search your projects', + ); }); }); }); diff --git a/spec/javascripts/vue_shared/components/project_selector/project_list_item_spec.js b/spec/javascripts/vue_shared/components/project_selector/project_list_item_spec.js index 8dbdfe97f8f..b95183747bb 100644 --- a/spec/javascripts/vue_shared/components/project_selector/project_list_item_spec.js +++ b/spec/javascripts/vue_shared/components/project_selector/project_list_item_spec.js @@ -1,4 +1,3 @@ -import _ from 'underscore'; import ProjectListItem from '~/vue_shared/components/project_selector/project_list_item.vue'; import { shallowMount, createLocalVue } from '@vue/test-utils'; import { trimText } from 'spec/helpers/vue_component_helper'; @@ -6,99 +5,106 @@ import { trimText } from 'spec/helpers/vue_component_helper'; const localVue = createLocalVue(); describe('ProjectListItem component', () => { + const Component = localVue.extend(ProjectListItem); let wrapper; let vm; + let options; loadJSONFixtures('projects.json'); const project = getJSONFixture('projects.json')[0]; beforeEach(() => { - wrapper = shallowMount(localVue.extend(ProjectListItem), { + options = { propsData: { project, selected: false, }, sync: false, localVue, - }); - - ({ vm } = wrapper); + }; }); afterEach(() => { - vm.$destroy(); + wrapper.vm.$destroy(); }); it('does not render a check mark icon if selected === false', () => { - expect(vm.$el.querySelector('.js-selected-icon.js-unselected')).toBeTruthy(); + wrapper = shallowMount(Component, options); + + expect(wrapper.contains('.js-selected-icon.js-unselected')).toBe(true); }); - it('renders a check mark icon if selected === true', done => { - wrapper.setProps({ selected: true }); + it('renders a check mark icon if selected === true', () => { + options.propsData.selected = true; - vm.$nextTick(() => { - expect(vm.$el.querySelector('.js-selected-icon.js-selected')).toBeTruthy(); - done(); - }); + wrapper = shallowMount(Component, options); + + expect(wrapper.contains('.js-selected-icon.js-selected')).toBe(true); }); it(`emits a "clicked" event when clicked`, () => { + wrapper = shallowMount(Component, options); + ({ vm } = wrapper); + spyOn(vm, '$emit'); - vm.onClick(); + wrapper.vm.onClick(); - expect(vm.$emit).toHaveBeenCalledWith('click'); + expect(wrapper.vm.$emit).toHaveBeenCalledWith('click'); }); it(`renders the project avatar`, () => { - expect(vm.$el.querySelector('.js-project-avatar')).toBeTruthy(); + wrapper = shallowMount(Component, options); + + expect(wrapper.contains('.js-project-avatar')).toBe(true); }); - it(`renders a simple namespace name with a trailing slash`, done => { - project.name_with_namespace = 'a / b'; - wrapper.setProps({ project: _.clone(project) }); + it(`renders a simple namespace name with a trailing slash`, () => { + options.propsData.project.name_with_namespace = 'a / b'; - vm.$nextTick(() => { - const renderedNamespace = trimText(vm.$el.querySelector('.js-project-namespace').textContent); + wrapper = shallowMount(Component, options); + const renderedNamespace = trimText(wrapper.find('.js-project-namespace').text()); - expect(renderedNamespace).toBe('a /'); - done(); - }); + expect(renderedNamespace).toBe('a /'); }); - it(`renders a properly truncated namespace with a trailing slash`, done => { - project.name_with_namespace = 'a / b / c / d / e / f'; - wrapper.setProps({ project: _.clone(project) }); + it(`renders a properly truncated namespace with a trailing slash`, () => { + options.propsData.project.name_with_namespace = 'a / b / c / d / e / f'; - vm.$nextTick(() => { - const renderedNamespace = trimText(vm.$el.querySelector('.js-project-namespace').textContent); + wrapper = shallowMount(Component, options); + const renderedNamespace = trimText(wrapper.find('.js-project-namespace').text()); - expect(renderedNamespace).toBe('a / ... / e /'); - done(); - }); + expect(renderedNamespace).toBe('a / ... / e /'); }); - it(`renders the project name`, done => { - project.name = 'my-test-project'; - wrapper.setProps({ project: _.clone(project) }); + it(`renders the project name`, () => { + options.propsData.project.name = 'my-test-project'; - vm.$nextTick(() => { - const renderedName = trimText(vm.$el.querySelector('.js-project-name').innerHTML); + wrapper = shallowMount(Component, options); + const renderedName = trimText(wrapper.find('.js-project-name').text()); - expect(renderedName).toBe('my-test-project'); - done(); - }); + expect(renderedName).toBe('my-test-project'); }); - it(`renders the project name with highlighting in the case of a search query match`, done => { - project.name = 'my-test-project'; - wrapper.setProps({ project: _.clone(project), matcher: 'pro' }); + it(`renders the project name with highlighting in the case of a search query match`, () => { + options.propsData.project.name = 'my-test-project'; + options.propsData.matcher = 'pro'; + + wrapper = shallowMount(Component, options); + const renderedName = trimText(wrapper.find('.js-project-name').html()); + const expected = 'my-test-project'; + + expect(renderedName).toContain(expected); + }); - vm.$nextTick(() => { - const renderedName = trimText(vm.$el.querySelector('.js-project-name').innerHTML); + it('prevents search query and project name XSS', () => { + const alertSpy = spyOn(window, 'alert'); + options.propsData.project.name = "my-xss-project"; + options.propsData.matcher = "pro"; - const expected = 'my-test-project'; + wrapper = shallowMount(Component, options); + const renderedName = trimText(wrapper.find('.js-project-name').html()); + const expected = 'my-xss-project'; - expect(renderedName).toBe(expected); - done(); - }); + expect(renderedName).toContain(expected); + expect(alertSpy).not.toHaveBeenCalled(); }); }); diff --git a/spec/javascripts/vue_shared/components/project_selector/project_selector_spec.js b/spec/javascripts/vue_shared/components/project_selector/project_selector_spec.js index 88c1dff76a1..ba9ec8f2f19 100644 --- a/spec/javascripts/vue_shared/components/project_selector/project_selector_spec.js +++ b/spec/javascripts/vue_shared/components/project_selector/project_selector_spec.js @@ -38,28 +38,25 @@ describe('ProjectSelector component', () => { }); it('renders the search results', () => { - expect(vm.$el.querySelectorAll('.js-project-list-item').length).toBe(5); + expect(wrapper.findAll('.js-project-list-item').length).toBe(5); }); - it(`triggers a (debounced) search when the search input value changes`, done => { + it(`triggers a (debounced) search when the search input value changes`, () => { spyOn(vm, '$emit'); const query = 'my test query!'; - const searchInput = vm.$el.querySelector('.js-project-selector-input'); - searchInput.value = query; - searchInput.dispatchEvent(new Event('input')); + const searchInput = wrapper.find('.js-project-selector-input'); + searchInput.setValue(query); + searchInput.trigger('input'); - vm.$nextTick(() => { - expect(vm.$emit).not.toHaveBeenCalledWith(); - jasmine.clock().tick(501); + expect(vm.$emit).not.toHaveBeenCalledWith(); + jasmine.clock().tick(501); - expect(vm.$emit).toHaveBeenCalledWith('searched', query); - done(); - }); + expect(vm.$emit).toHaveBeenCalledWith('searched', query); }); - it(`debounces the search input`, done => { + it(`debounces the search input`, () => { spyOn(vm, '$emit'); - const searchInput = vm.$el.querySelector('.js-project-selector-input'); + const searchInput = wrapper.find('.js-project-selector-input'); const updateSearchQuery = (count = 0) => { if (count === 10) { @@ -67,15 +64,12 @@ describe('ProjectSelector component', () => { expect(vm.$emit).toHaveBeenCalledTimes(1); expect(vm.$emit).toHaveBeenCalledWith('searched', `search query #9`); - done(); } else { - searchInput.value = `search query #${count}`; - searchInput.dispatchEvent(new Event('input')); + searchInput.setValue(`search query #${count}`); + searchInput.trigger('input'); - vm.$nextTick(() => { - jasmine.clock().tick(400); - updateSearchQuery(count + 1); - }); + jasmine.clock().tick(400); + updateSearchQuery(count + 1); } }; @@ -83,7 +77,7 @@ describe('ProjectSelector component', () => { }); it(`includes a placeholder in the search box`, () => { - expect(vm.$el.querySelector('.js-project-selector-input').placeholder).toBe( + expect(wrapper.find('.js-project-selector-input').attributes('placeholder')).toBe( 'Search your projects', ); }); @@ -95,58 +89,44 @@ describe('ProjectSelector component', () => { expect(vm.$emit).toHaveBeenCalledWith('projectClicked', _.first(searchResults)); }); - it(`shows a "no results" message if showNoResultsMessage === true`, done => { + it(`shows a "no results" message if showNoResultsMessage === true`, () => { wrapper.setProps({ showNoResultsMessage: true }); - vm.$nextTick(() => { - const noResultsEl = vm.$el.querySelector('.js-no-results-message'); - - expect(noResultsEl).toBeTruthy(); + expect(wrapper.contains('.js-no-results-message')).toBe(true); - expect(trimText(noResultsEl.textContent)).toEqual('Sorry, no projects matched your search'); + const noResultsEl = wrapper.find('.js-no-results-message'); - done(); - }); + expect(trimText(noResultsEl.text())).toEqual('Sorry, no projects matched your search'); }); - it(`shows a "minimum seach query" message if showMinimumSearchQueryMessage === true`, done => { + it(`shows a "minimum seach query" message if showMinimumSearchQueryMessage === true`, () => { wrapper.setProps({ showMinimumSearchQueryMessage: true }); - vm.$nextTick(() => { - const minimumSearchEl = vm.$el.querySelector('.js-minimum-search-query-message'); - - expect(minimumSearchEl).toBeTruthy(); + expect(wrapper.contains('.js-minimum-search-query-message')).toBe(true); - expect(trimText(minimumSearchEl.textContent)).toEqual( - 'Enter at least three characters to search', - ); + const minimumSearchEl = wrapper.find('.js-minimum-search-query-message'); - done(); - }); + expect(trimText(minimumSearchEl.text())).toEqual('Enter at least three characters to search'); }); - it(`shows a error message if showSearchErrorMessage === true`, done => { + it(`shows a error message if showSearchErrorMessage === true`, () => { wrapper.setProps({ showSearchErrorMessage: true }); - vm.$nextTick(() => { - const errorMessageEl = vm.$el.querySelector('.js-search-error-message'); - - expect(errorMessageEl).toBeTruthy(); + expect(wrapper.contains('.js-search-error-message')).toBe(true); - expect(trimText(errorMessageEl.textContent)).toEqual( - 'Something went wrong, unable to search projects', - ); + const errorMessageEl = wrapper.find('.js-search-error-message'); - done(); - }); + expect(trimText(errorMessageEl.text())).toEqual( + 'Something went wrong, unable to search projects', + ); }); it(`focuses the input element when the focusSearchInput() method is called`, () => { - const input = vm.$el.querySelector('.js-project-selector-input'); + const input = wrapper.find('.js-project-selector-input'); - expect(document.activeElement).not.toBe(input); + expect(document.activeElement).not.toBe(input.element); vm.focusSearchInput(); - expect(document.activeElement).toBe(input); + expect(document.activeElement).toBe(input.element); }); }); -- GitLab