diff options
58 files changed, 896 insertions, 419 deletions
diff --git a/.gitlab/issue_templates/Bug.md b/.gitlab/issue_templates/Bug.md index 34c2e097ba8..6bb21e6a3af 100644 --- a/.gitlab/issue_templates/Bug.md +++ b/.gitlab/issue_templates/Bug.md @@ -25,14 +25,20 @@ logs, and code as it's very hard to read otherwise.) #### Results of GitLab environment info +<details> + (For installations with omnibus-gitlab package run and paste the output of: `sudo gitlab-rake gitlab:env:info`) (For installations from source run and paste the output of: `sudo -u git -H bundle exec rake gitlab:env:info RAILS_ENV=production`) +</details> + #### Results of GitLab application Check +<details> + (For installations with omnibus-gitlab package run and paste the output of: `sudo gitlab-rake gitlab:check SANITIZE=true`) @@ -41,6 +47,8 @@ logs, and code as it's very hard to read otherwise.) (we will only investigate if the tests are passing) +</details> + ### Possible fixes (If you can, link to the line of code that might be responsible for the problem) diff --git a/app/assets/javascripts/issue.js b/app/assets/javascripts/issue.js index 47e675f537e..011043e992f 100644 --- a/app/assets/javascripts/issue.js +++ b/app/assets/javascripts/issue.js @@ -20,57 +20,60 @@ class Issue { }); Issue.initIssueBtnEventListeners(); } + + Issue.$btnNewBranch = $('#new-branch'); + Issue.initMergeRequests(); Issue.initRelatedBranches(); Issue.initCanCreateBranch(); } static initIssueBtnEventListeners() { - var issueFailMessage; - issueFailMessage = 'Unable to update this issue at this time.'; - return $('a.btn-close, a.btn-reopen').on('click', function(e) { - var $this, isClose, shouldSubmit, url; + const issueFailMessage = 'Unable to update this issue at this time.'; + + const closeButtons = $('a.btn-close'); + const isClosedBadge = $('div.status-box-closed'); + const isOpenBadge = $('div.status-box-open'); + const projectIssuesCounter = $('.issue_counter'); + const reopenButtons = $('a.btn-reopen'); + + return closeButtons.add(reopenButtons).on('click', function(e) { + var $this, shouldSubmit, url; e.preventDefault(); e.stopImmediatePropagation(); $this = $(this); - isClose = $this.hasClass('btn-close'); shouldSubmit = $this.hasClass('btn-comment'); if (shouldSubmit) { Issue.submitNoteForm($this.closest('form')); } $this.prop('disabled', true); + Issue.setNewBranchButtonState(true, null); url = $this.attr('href'); return $.ajax({ type: 'PUT', - url: url, - error: function(jqXHR, textStatus, errorThrown) { - var issueStatus; - issueStatus = isClose ? 'close' : 'open'; - return new Flash(issueFailMessage, 'alert'); - }, - success: function(data, textStatus, jqXHR) { - if ('id' in data) { - $(document).trigger('issuable:change'); - let total = Number($('.issue_counter').text().replace(/[^\d]/, '')); - if (isClose) { - $('a.btn-close').addClass('hidden'); - $('a.btn-reopen').removeClass('hidden'); - $('div.status-box-closed').removeClass('hidden'); - $('div.status-box-open').addClass('hidden'); - total -= 1; - } else { - $('a.btn-reopen').addClass('hidden'); - $('a.btn-close').removeClass('hidden'); - $('div.status-box-closed').addClass('hidden'); - $('div.status-box-open').removeClass('hidden'); - total += 1; - } - $('.issue_counter').text(gl.text.addDelimiter(total)); - } else { - new Flash(issueFailMessage, 'alert'); - } - return $this.prop('disabled', false); + url: url + }).fail(function(jqXHR, textStatus, errorThrown) { + new Flash(issueFailMessage); + Issue.initCanCreateBranch(); + }).done(function(data, textStatus, jqXHR) { + if ('id' in data) { + $(document).trigger('issuable:change'); + + const isClosed = $this.hasClass('btn-close'); + closeButtons.toggleClass('hidden', isClosed); + reopenButtons.toggleClass('hidden', !isClosed); + isClosedBadge.toggleClass('hidden', !isClosed); + isOpenBadge.toggleClass('hidden', isClosed); + + let numProjectIssues = Number(projectIssuesCounter.text().replace(/[^\d]/, '')); + numProjectIssues = isClosed ? numProjectIssues - 1 : numProjectIssues + 1; + projectIssuesCounter.text(gl.text.addDelimiter(numProjectIssues)); + } else { + new Flash(issueFailMessage); } + + $this.prop('disabled', false); + Issue.initCanCreateBranch(); }); }); } @@ -86,9 +89,9 @@ class Issue { static initMergeRequests() { var $container; $container = $('#merge-requests'); - return $.getJSON($container.data('url')).error(function() { - return new Flash('Failed to load referenced merge requests', 'alert'); - }).success(function(data) { + return $.getJSON($container.data('url')).fail(function() { + return new Flash('Failed to load referenced merge requests'); + }).done(function(data) { if ('html' in data) { return $container.html(data.html); } @@ -98,9 +101,9 @@ class Issue { static initRelatedBranches() { var $container; $container = $('#related-branches'); - return $.getJSON($container.data('url')).error(function() { - return new Flash('Failed to load related branches', 'alert'); - }).success(function(data) { + return $.getJSON($container.data('url')).fail(function() { + return new Flash('Failed to load related branches'); + }).done(function(data) { if ('html' in data) { return $container.html(data.html); } @@ -108,24 +111,27 @@ class Issue { } static initCanCreateBranch() { - var $container; - $container = $('#new-branch'); // If the user doesn't have the required permissions the container isn't // rendered at all. - if ($container.length === 0) { + if (Issue.$btnNewBranch.length === 0) { return; } - return $.getJSON($container.data('path')).error(function() { - $container.find('.unavailable').show(); - return new Flash('Failed to check if a new branch can be created.', 'alert'); - }).success(function(data) { - if (data.can_create_branch) { - $container.find('.available').show(); - } else { - return $container.find('.unavailable').show(); - } + return $.getJSON(Issue.$btnNewBranch.data('path')).fail(function() { + Issue.setNewBranchButtonState(false, false); + new Flash('Failed to check if a new branch can be created.'); + }).done(function(data) { + Issue.setNewBranchButtonState(false, data.can_create_branch); }); } + + static setNewBranchButtonState(isPending, canCreate) { + if (Issue.$btnNewBranch.length === 0) { + return; + } + + Issue.$btnNewBranch.find('.available').toggle(!isPending && canCreate); + Issue.$btnNewBranch.find('.unavailable').toggle(!isPending && !canCreate); + } } export default Issue; diff --git a/app/assets/stylesheets/framework/calendar.scss b/app/assets/stylesheets/framework/calendar.scss index 9a0f7a14e57..759401a7806 100644 --- a/app/assets/stylesheets/framework/calendar.scss +++ b/app/assets/stylesheets/framework/calendar.scss @@ -5,7 +5,7 @@ direction: rtl; @media (min-width: $screen-sm-min) and (max-width: $screen-md-max) { - overflow-x: scroll; + overflow-x: auto; } } diff --git a/app/assets/stylesheets/framework/filters.scss b/app/assets/stylesheets/framework/filters.scss index 12465d4a70b..279a50eaa33 100644 --- a/app/assets/stylesheets/framework/filters.scss +++ b/app/assets/stylesheets/framework/filters.scss @@ -82,7 +82,7 @@ .input-token:last-child { flex: 1; -webkit-flex: 1; - max-width: initial; + max-width: inherit; } } diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 224b44db397..09dc8b38229 100755 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -38,7 +38,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @collection_type = "MergeRequest" @merge_requests = merge_requests_collection @merge_requests = @merge_requests.page(params[:page]) - @merge_requests = @merge_requests.includes(merge_request_diff: :merge_request) + @merge_requests = @merge_requests.preload(merge_request_diff: :merge_request) @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type) if @merge_requests.out_of_range? && @merge_requests.total_pages != 0 diff --git a/app/helpers/javascript_helper.rb b/app/helpers/javascript_helper.rb index 68c09c922a6..d5e77c7e271 100644 --- a/app/helpers/javascript_helper.rb +++ b/app/helpers/javascript_helper.rb @@ -3,7 +3,8 @@ module JavascriptHelper javascript_include_tag asset_path(js) end - def page_specific_javascript_bundle_tag(js) - javascript_include_tag(*webpack_asset_paths(js)) + # deprecated; use webpack_bundle_tag directly instead + def page_specific_javascript_bundle_tag(bundle) + webpack_bundle_tag(bundle) end end diff --git a/app/helpers/webpack_helper.rb b/app/helpers/webpack_helper.rb new file mode 100644 index 00000000000..6bacda9fe75 --- /dev/null +++ b/app/helpers/webpack_helper.rb @@ -0,0 +1,30 @@ +require 'webpack/rails/manifest' + +module WebpackHelper + def webpack_bundle_tag(bundle) + javascript_include_tag(*gitlab_webpack_asset_paths(bundle)) + end + + # override webpack-rails gem helper until changes can make it upstream + def gitlab_webpack_asset_paths(source, extension: nil) + return "" unless source.present? + + paths = Webpack::Rails::Manifest.asset_paths(source) + if extension + paths = paths.select { |p| p.ends_with? ".#{extension}" } + end + + # include full webpack-dev-server url for rspec tests running locally + if Rails.env.test? && Rails.configuration.webpack.dev_server.enabled + host = Rails.configuration.webpack.dev_server.host + port = Rails.configuration.webpack.dev_server.port + protocol = Rails.configuration.webpack.dev_server.https ? 'https' : 'http' + + paths.map! do |p| + "#{protocol}://#{host}:#{port}#{p}" + end + end + + paths + end +end diff --git a/app/models/label.rb b/app/models/label.rb index 568fa6d44f5..d8b0e250732 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -21,6 +21,8 @@ class Label < ActiveRecord::Base has_many :issues, through: :label_links, source: :target, source_type: 'Issue' has_many :merge_requests, through: :label_links, source: :target, source_type: 'MergeRequest' + before_validation :strip_whitespace_from_title_and_color + validates :color, color: true, allow_blank: false # Don't allow ',' for label titles @@ -193,4 +195,8 @@ class Label < ActiveRecord::Base def sanitize_title(value) CGI.unescapeHTML(Sanitize.clean(value.to_s)) end + + def strip_whitespace_from_title_and_color + %w(color title).each { |attr| self[attr] = self[attr]&.strip } + end end diff --git a/app/views/help/ui.html.haml b/app/views/help/ui.html.haml index 207f80bedfe..615dd56afbd 100644 --- a/app/views/help/ui.html.haml +++ b/app/views/help/ui.html.haml @@ -252,7 +252,7 @@ = icon('chevron-down') .dropdown-menu.dropdown-select.dropdown-menu-selectable .dropdown-title - %span Dropdown Title + %span Dropdown title %button.dropdown-title-button.dropdown-menu-close{ aria: { label: "Close" } } = icon('times') .dropdown-input @@ -291,7 +291,7 @@ = icon('chevron-down') .dropdown-menu.dropdown-select.dropdown-menu-selectable.is-loading .dropdown-title - %span Dropdown Title + %span Dropdown title %button.dropdown-title-button.dropdown-menu-close{ aria: { label: "Close" } } = icon('times') .dropdown-input @@ -335,7 +335,7 @@ = icon('chevron-down') .dropdown-menu.dropdown-select.dropdown-menu-selectable.dropdown-menu-user .dropdown-title - %span Dropdown Title + %span Dropdown title %button.dropdown-title-button.dropdown-menu-close{ aria: { label: "Close" } } = icon('times') .dropdown-input @@ -362,7 +362,7 @@ .dropdown-title %button.dropdown-title-button.dropdown-menu-back{ aria: { label: "Go back" } } = icon('arrow-left') - %span Dropdown Title + %span Dropdown title %button.dropdown-title-button.dropdown-menu-close{ aria: { label: "Close" } } = icon('times') .dropdown-input diff --git a/app/views/layouts/_head.html.haml b/app/views/layouts/_head.html.haml index a611481a0a4..19473b6ab27 100644 --- a/app/views/layouts/_head.html.haml +++ b/app/views/layouts/_head.html.haml @@ -28,9 +28,9 @@ = stylesheet_link_tag "application", media: "all" = stylesheet_link_tag "print", media: "print" - = javascript_include_tag(*webpack_asset_paths("runtime")) - = javascript_include_tag(*webpack_asset_paths("common")) - = javascript_include_tag(*webpack_asset_paths("main")) + = webpack_bundle_tag "runtime" + = webpack_bundle_tag "common" + = webpack_bundle_tag "main" - if content_for?(:page_specific_javascripts) = yield :page_specific_javascripts diff --git a/app/workers/build_coverage_worker.rb b/app/workers/build_coverage_worker.rb index def0ab1dde1..f7ae996bb17 100644 --- a/app/workers/build_coverage_worker.rb +++ b/app/workers/build_coverage_worker.rb @@ -3,7 +3,6 @@ class BuildCoverageWorker include BuildQueue def perform(build_id) - Ci::Build.find_by(id: build_id) - .try(:update_coverage) + Ci::Build.find_by(id: build_id)&.update_coverage end end diff --git a/changelogs/unreleased/2120-issues-search-bar-not-picking-up.yml b/changelogs/unreleased/2120-issues-search-bar-not-picking-up.yml new file mode 100644 index 00000000000..706609b7baf --- /dev/null +++ b/changelogs/unreleased/2120-issues-search-bar-not-picking-up.yml @@ -0,0 +1,4 @@ +--- +title: Fix filtered search input width for IE +merge_request: +author: diff --git a/changelogs/unreleased/27729-improve-webpack-dev-environment.yml b/changelogs/unreleased/27729-improve-webpack-dev-environment.yml new file mode 100644 index 00000000000..d04ea70ab1c --- /dev/null +++ b/changelogs/unreleased/27729-improve-webpack-dev-environment.yml @@ -0,0 +1,4 @@ +--- +title: Add webpack_bundle_tag helper to improve non-localhost GDK configurations +merge_request: 10604 +author: diff --git a/changelogs/unreleased/29056-backport-ee-cleanup-database-file.yml b/changelogs/unreleased/29056-backport-ee-cleanup-database-file.yml new file mode 100644 index 00000000000..0ebb9d57611 --- /dev/null +++ b/changelogs/unreleased/29056-backport-ee-cleanup-database-file.yml @@ -0,0 +1,4 @@ +--- +title: Turns true value and false value database methods from instance to class methods +merge_request: 10583 +author: diff --git a/changelogs/unreleased/fix-missing-capitalisation-buttons.yml b/changelogs/unreleased/fix-missing-capitalisation-buttons.yml new file mode 100644 index 00000000000..b2c40483475 --- /dev/null +++ b/changelogs/unreleased/fix-missing-capitalisation-buttons.yml @@ -0,0 +1,4 @@ +--- +title: Fix missing capitalisation on views +merge_request: +author: diff --git a/changelogs/unreleased/fix-preloading-merge_request_diff.yml b/changelogs/unreleased/fix-preloading-merge_request_diff.yml new file mode 100644 index 00000000000..d38b6b0a707 --- /dev/null +++ b/changelogs/unreleased/fix-preloading-merge_request_diff.yml @@ -0,0 +1,4 @@ +--- +title: Fix bad query for PostgreSQL showing merge requests list +merge_request: 10666 +author: diff --git a/changelogs/unreleased/fix_spaces_in_label_title.yml b/changelogs/unreleased/fix_spaces_in_label_title.yml new file mode 100644 index 00000000000..51f07438edb --- /dev/null +++ b/changelogs/unreleased/fix_spaces_in_label_title.yml @@ -0,0 +1,4 @@ +--- +title: Remove heading and trailing spaces from label's color and title +merge_request: 10603 +author: blackst0ne diff --git a/changelogs/unreleased/reset-new-branch-button.yml b/changelogs/unreleased/reset-new-branch-button.yml new file mode 100644 index 00000000000..318ee46298f --- /dev/null +++ b/changelogs/unreleased/reset-new-branch-button.yml @@ -0,0 +1,4 @@ +--- +title: Reset New branch button when issue state changes +merge_request: 5962 +author: winniehell diff --git a/changelogs/unreleased/user-activity-scroll-bar.yml b/changelogs/unreleased/user-activity-scroll-bar.yml new file mode 100644 index 00000000000..97cccee42cb --- /dev/null +++ b/changelogs/unreleased/user-activity-scroll-bar.yml @@ -0,0 +1,4 @@ +--- +title: Fix preemptive scroll bar on user activity calendar. +merge_request: !10636 +author: diff --git a/config/webpack.config.js b/config/webpack.config.js index 0fea3f8222b..ffb16190093 100644 --- a/config/webpack.config.js +++ b/config/webpack.config.js @@ -11,6 +11,7 @@ var WatchMissingNodeModulesPlugin = require('react-dev-utils/WatchMissingNodeMod var ROOT_PATH = path.resolve(__dirname, '..'); var IS_PRODUCTION = process.env.NODE_ENV === 'production'; var IS_DEV_SERVER = process.argv[1].indexOf('webpack-dev-server') !== -1; +var DEV_SERVER_HOST = process.env.DEV_SERVER_HOST || 'localhost'; var DEV_SERVER_PORT = parseInt(process.env.DEV_SERVER_PORT, 10) || 3808; var DEV_SERVER_LIVERELOAD = process.env.DEV_SERVER_LIVERELOAD !== 'false'; var WEBPACK_REPORT = process.env.WEBPACK_REPORT; @@ -182,12 +183,13 @@ if (IS_PRODUCTION) { if (IS_DEV_SERVER) { config.devtool = 'cheap-module-eval-source-map'; config.devServer = { + host: DEV_SERVER_HOST, port: DEV_SERVER_PORT, headers: { 'Access-Control-Allow-Origin': '*' }, stats: 'errors-only', inline: DEV_SERVER_LIVERELOAD }; - config.output.publicPath = '//localhost:' + DEV_SERVER_PORT + config.output.publicPath; + config.output.publicPath = '//' + DEV_SERVER_HOST + ':' + DEV_SERVER_PORT + config.output.publicPath; config.plugins.push( // watch node_modules for changes if we encounter a missing module compile error new WatchMissingNodeModulesPlugin(path.join(ROOT_PATH, 'node_modules')) diff --git a/db/migrate/20130218141258_convert_closed_to_state_in_issue.rb b/db/migrate/20130218141258_convert_closed_to_state_in_issue.rb index 94c0a6845d5..67a0d3b53eb 100644 --- a/db/migrate/20130218141258_convert_closed_to_state_in_issue.rb +++ b/db/migrate/20130218141258_convert_closed_to_state_in_issue.rb @@ -1,6 +1,6 @@ # rubocop:disable all class ConvertClosedToStateInIssue < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def up execute "UPDATE #{table_name} SET state = 'closed' WHERE closed = #{true_value}" diff --git a/db/migrate/20130218141327_convert_closed_to_state_in_merge_request.rb b/db/migrate/20130218141327_convert_closed_to_state_in_merge_request.rb index 64a9c761352..307fc6a023d 100644 --- a/db/migrate/20130218141327_convert_closed_to_state_in_merge_request.rb +++ b/db/migrate/20130218141327_convert_closed_to_state_in_merge_request.rb @@ -1,6 +1,6 @@ # rubocop:disable all class ConvertClosedToStateInMergeRequest < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def up execute "UPDATE #{table_name} SET state = 'merged' WHERE closed = #{true_value} AND merged = #{true_value}" diff --git a/db/migrate/20130218141344_convert_closed_to_state_in_milestone.rb b/db/migrate/20130218141344_convert_closed_to_state_in_milestone.rb index 41508c2dc95..d12703cf3b2 100644 --- a/db/migrate/20130218141344_convert_closed_to_state_in_milestone.rb +++ b/db/migrate/20130218141344_convert_closed_to_state_in_milestone.rb @@ -1,6 +1,6 @@ # rubocop:disable all class ConvertClosedToStateInMilestone < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def up execute "UPDATE #{table_name} SET state = 'closed' WHERE closed = #{true_value}" diff --git a/db/migrate/20130315124931_user_color_scheme.rb b/db/migrate/20130315124931_user_color_scheme.rb index 06e28a49d9d..09af928fde7 100644 --- a/db/migrate/20130315124931_user_color_scheme.rb +++ b/db/migrate/20130315124931_user_color_scheme.rb @@ -1,6 +1,6 @@ # rubocop:disable all class UserColorScheme < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def up add_column :users, :color_scheme_id, :integer, null: false, default: 1 diff --git a/db/migrate/20131112220935_add_visibility_level_to_projects.rb b/db/migrate/20131112220935_add_visibility_level_to_projects.rb index 5efc17b228e..86d73753adc 100644 --- a/db/migrate/20131112220935_add_visibility_level_to_projects.rb +++ b/db/migrate/20131112220935_add_visibility_level_to_projects.rb @@ -1,6 +1,6 @@ # rubocop:disable all class AddVisibilityLevelToProjects < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def self.up add_column :projects, :visibility_level, :integer, :default => 0, :null => false diff --git a/db/migrate/20140313092127_migrate_already_imported_projects.rb b/db/migrate/20140313092127_migrate_already_imported_projects.rb index f2e91fe1b40..0afc26b8764 100644 --- a/db/migrate/20140313092127_migrate_already_imported_projects.rb +++ b/db/migrate/20140313092127_migrate_already_imported_projects.rb @@ -1,6 +1,6 @@ # rubocop:disable all class MigrateAlreadyImportedProjects < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def up execute("UPDATE projects SET import_status = 'finished' WHERE imported = #{true_value}") diff --git a/db/migrate/20141007100818_add_visibility_level_to_snippet.rb b/db/migrate/20141007100818_add_visibility_level_to_snippet.rb index 688d8578478..0c14f75c154 100644 --- a/db/migrate/20141007100818_add_visibility_level_to_snippet.rb +++ b/db/migrate/20141007100818_add_visibility_level_to_snippet.rb @@ -1,6 +1,6 @@ # rubocop:disable all class AddVisibilityLevelToSnippet < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def up add_column :snippets, :visibility_level, :integer, :default => 0, :null => false diff --git a/db/migrate/20151209144329_migrate_ci_web_hooks.rb b/db/migrate/20151209144329_migrate_ci_web_hooks.rb index cb1e556623a..62a6d334f04 100644 --- a/db/migrate/20151209144329_migrate_ci_web_hooks.rb +++ b/db/migrate/20151209144329_migrate_ci_web_hooks.rb @@ -1,6 +1,6 @@ # rubocop:disable all class MigrateCiWebHooks < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def up execute( diff --git a/db/migrate/20151209145909_migrate_ci_emails.rb b/db/migrate/20151209145909_migrate_ci_emails.rb index 6b7a106814d..5de7b205fb1 100644 --- a/db/migrate/20151209145909_migrate_ci_emails.rb +++ b/db/migrate/20151209145909_migrate_ci_emails.rb @@ -1,6 +1,6 @@ # rubocop:disable all class MigrateCiEmails < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def up # This inserts a new service: BuildsEmailService diff --git a/db/migrate/20151210125232_migrate_ci_slack_service.rb b/db/migrate/20151210125232_migrate_ci_slack_service.rb index 633d5148d97..fff130b7b10 100644 --- a/db/migrate/20151210125232_migrate_ci_slack_service.rb +++ b/db/migrate/20151210125232_migrate_ci_slack_service.rb @@ -1,6 +1,6 @@ # rubocop:disable all class MigrateCiSlackService < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def up properties_query = 'SELECT properties FROM ci_services ' \ diff --git a/db/migrate/20151210125927_migrate_ci_hip_chat_service.rb b/db/migrate/20151210125927_migrate_ci_hip_chat_service.rb index dae084ce180..824f6f84195 100644 --- a/db/migrate/20151210125927_migrate_ci_hip_chat_service.rb +++ b/db/migrate/20151210125927_migrate_ci_hip_chat_service.rb @@ -1,6 +1,6 @@ # rubocop:disable all class MigrateCiHipChatService < ActiveRecord::Migration - include Gitlab::Database + include Gitlab::Database::MigrationHelpers def up # From properties strip `hipchat_` key diff --git a/db/post_migrate/20170301205640_migrate_build_events_to_pipeline_events.rb b/db/post_migrate/20170301205640_migrate_build_events_to_pipeline_events.rb index 2dd14ee5a78..04bf89c9687 100644 --- a/db/post_migrate/20170301205640_migrate_build_events_to_pipeline_events.rb +++ b/db/post_migrate/20170301205640_migrate_build_events_to_pipeline_events.rb @@ -1,6 +1,5 @@ class MigrateBuildEventsToPipelineEvents < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers - include Gitlab::Database DOWNTIME = false diff --git a/doc/ci/img/pipelines.png b/doc/ci/img/pipelines.png Binary files differindex 5937e9d99c8..a604fcb2587 100644 --- a/doc/ci/img/pipelines.png +++ b/doc/ci/img/pipelines.png diff --git a/doc/development/fe_guide/performance.md b/doc/development/fe_guide/performance.md index e74eb729515..2ddcbe13afa 100644 --- a/doc/development/fe_guide/performance.md +++ b/doc/development/fe_guide/performance.md @@ -48,8 +48,8 @@ Steps to split page-specific JavaScript from the main `main.js`: ```haml - content_for :page_specific_javascripts do - = page_specific_javascript_bundle_tag('lib_chart') - = page_specific_javascript_bundle_tag('graphs') + = webpack_bundle_tag 'lib_chart' + = webpack_bundle_tag 'graphs' ``` The above loads `chart.js` and `graphs_bundle.js` for this page only. `chart.js` diff --git a/doc/development/fe_guide/testing.md b/doc/development/fe_guide/testing.md index 8d3513d3566..a4631fd0073 100644 --- a/doc/development/fe_guide/testing.md +++ b/doc/development/fe_guide/testing.md @@ -13,10 +13,19 @@ for more information on general testing practices at GitLab. ## Karma test suite GitLab uses the [Karma][karma] test runner with [Jasmine][jasmine] as its test -framework for our JavaScript unit tests. For tests that rely on DOM +framework for our JavaScript unit tests. For tests that rely on DOM manipulation we use fixtures which are pre-compiled from HAML source files and served during testing by the [jasmine-jquery][jasmine-jquery] plugin. +JavaScript tests live in `spec/javascripts/`, matching the folder structure +of `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js` +has a corresponding `spec/javascripts/behaviors/autosize_spec.js` file. + +Keep in mind that in a CI environment, these tests are run in a headless +browser and you will not have access to certain APIs, such as +[`Notification`](https://developer.mozilla.org/en-US/docs/Web/API/notification), +which will have to be stubbed. + ### Running frontend tests `rake karma` runs the frontend-only (JavaScript) tests. @@ -80,24 +89,23 @@ If an integration test depends on JavaScript to run correctly, you need to make sure the spec is configured to enable JavaScript when the tests are run. If you don't do this you'll see vague error messages from the spec runner. -To enable a JavaScript driver in an `rspec` test, add `js: true` to the +To enable a JavaScript driver in an `rspec` test, add `:js` to the individual spec or the context block containing multiple specs that need JavaScript enabled: ```ruby - # For one spec -it 'presents information about abuse report', js: true do - # assertions... +it 'presents information about abuse report', :js do + # assertions... end -describe "Admin::AbuseReports", js: true do - it 'presents information about abuse report' do - # assertions... - end - it 'shows buttons for adding to abuse report' do - # assertions... - end +describe "Admin::AbuseReports", :js do + it 'presents information about abuse report' do + # assertions... + end + it 'shows buttons for adding to abuse report' do + # assertions... + end end ``` @@ -113,13 +121,12 @@ file for the failing spec, add the `@javascript` flag above the Scenario: ``` @javascript Scenario: Developer can approve merge request - Given I am a "Shop" developer - And I visit project "Shop" merge requests page - And merge request 'Bug NS-04' must be approved - And I click link "Bug NS-04" - When I click link "Approve" - Then I should see approved merge request "Bug NS-04" - + Given I am a "Shop" developer + And I visit project "Shop" merge requests page + And merge request 'Bug NS-04' must be approved + And I click link "Bug NS-04" + When I click link "Approve" + Then I should see approved merge request "Bug NS-04" ``` [capybara]: http://teamcapybara.github.io/capybara/ diff --git a/doc/development/testing.md b/doc/development/testing.md index 5bc958f5a96..ad540ec13db 100644 --- a/doc/development/testing.md +++ b/doc/development/testing.md @@ -9,52 +9,179 @@ this guide defines a rule that contradicts the thoughtbot guide, this guide takes precedence. Some guidelines may be repeated verbatim to stress their importance. -## Factories +## Definitions + +### Unit tests + +Formal definition: https://en.wikipedia.org/wiki/Unit_testing + +These kind of tests ensure that a single unit of code (a method) works as +expected (given an input, it has a predictable output). These tests should be +isolated as much as possible. For example, model methods that don't do anything +with the database shouldn't need a DB record. Classes that don't need database +records should use stubs/doubles as much as possible. + +| Code path | Tests path | Testing engine | Notes | +| --------- | ---------- | -------------- | ----- | +| `app/finders/` | `spec/finders/` | RSpec | | +| `app/helpers/` | `spec/helpers/` | RSpec | | +| `app/db/{post_,}migrate/` | `spec/migrations/` | RSpec | | +| `app/policies/` | `spec/policies/` | RSpec | | +| `app/presenters/` | `spec/presenters/` | RSpec | | +| `app/routing/` | `spec/routing/` | RSpec | | +| `app/serializers/` | `spec/serializers/` | RSpec | | +| `app/services/` | `spec/services/` | RSpec | | +| `app/tasks/` | `spec/tasks/` | RSpec | | +| `app/uploaders/` | `spec/uploaders/` | RSpec | | +| `app/views/` | `spec/views/` | RSpec | | +| `app/workers/` | `spec/workers/` | RSpec | | +| `app/assets/javascripts/` | `spec/javascripts/` | Karma | More details in the [JavaScript](#javascript) section. | + +### Integration tests + +Formal definition: https://en.wikipedia.org/wiki/Integration_testing + +These kind of tests ensure that individual parts of the application work well together, without the overhead of the actual app environment (i.e. the browser). These tests should assert at the request/response level: status code, headers, body. They're useful to test permissions, redirections, what view is rendered etc. + +| Code path | Tests path | Testing engine | Notes | +| --------- | ---------- | -------------- | ----- | +| `app/controllers/` | `spec/controllers/` | RSpec | | +| `app/mailers/` | `spec/mailers/` | RSpec | | +| `lib/api/` | `spec/requests/api/` | RSpec | | +| `lib/ci/api/` | `spec/requests/ci/api/` | RSpec | | +| `app/assets/javascripts/` | `spec/javascripts/` | Karma | More details in the [JavaScript](#javascript) section. | + +#### About controller tests + +In an ideal world, controllers should be thin. However, when this is not the +case, it's acceptable to write a system/feature test without JavaScript instead +of a controller test. The reason is that testing a fat controller usually +involves a lot of stubbing, things like: -GitLab uses [factory_girl] as a test fixture replacement. - -- Factory definitions live in `spec/factories/`, named using the pluralization - of their corresponding model (`User` factories are defined in `users.rb`). -- There should be only one top-level factory definition per file. -- FactoryGirl methods are mixed in to all RSpec groups. This means you can (and - should) call `create(...)` instead of `FactoryGirl.create(...)`. -- Make use of [traits] to clean up definitions and usages. -- When defining a factory, don't define attributes that are not required for the - resulting record to pass validation. -- When instantiating from a factory, don't supply attributes that aren't - required by the test. -- Factories don't have to be limited to `ActiveRecord` objects. - [See example](https://gitlab.com/gitlab-org/gitlab-ce/commit/0b8cefd3b2385a21cfed779bd659978c0402766d). - -[factory_girl]: https://github.com/thoughtbot/factory_girl -[traits]: http://www.rubydoc.info/gems/factory_girl/file/GETTING_STARTED.md#Traits - -## JavaScript - -GitLab uses [Karma] to run its [Jasmine] JavaScript specs. They can be run on -the command line via `bundle exec karma`. - -- JavaScript tests live in `spec/javascripts/`, matching the folder structure - of `app/assets/javascripts/`: `app/assets/javascripts/behaviors/autosize.js` - has a corresponding `spec/javascripts/behaviors/autosize_spec.js` file. -- Haml fixtures required for JavaScript tests live in - `spec/javascripts/fixtures`. They should contain the bare minimum amount of - markup necessary for the test. - - > **Warning:** Keep in mind that a Rails view may change and - invalidate your test, but everything will still pass because your fixture - doesn't reflect the latest view. Because of this we encourage you to - generate fixtures from actual rails views whenever possible. - -- Keep in mind that in a CI environment, these tests are run in a headless - browser and you will not have access to certain APIs, such as - [`Notification`](https://developer.mozilla.org/en-US/docs/Web/API/notification), - which will have to be stubbed. - -[Karma]: https://github.com/karma-runner/karma -[Jasmine]: https://github.com/jasmine/jasmine +```ruby +controller.instance_variable_set(:@user, user) +``` -For more information, see the [frontend testing guide](fe_guide/testing.md). +and use methods which are deprecated in Rails 5 ([#23768]). + +[#23768]: https://gitlab.com/gitlab-org/gitlab-ce/issues/23768 + +#### About Karma + +As you may have noticed, Karma is both in the Unit tests and the Integration +tests category. That's because Karma is a tool that provides an environment to +run JavaScript tests, so you can either run unit tests (e.g. test a single +JavaScript method), or integration tests (e.g. test a component that is composed +of multiple components). + +### System tests or Feature tests + +Formal definition: https://en.wikipedia.org/wiki/System_testing. + +These kind of tests ensure the application works as expected from a user point +of view (aka black-box testing). These tests should test a happy path for a +given page or set of pages, and a test case should be added for any regression +that couldn't have been caught at lower levels with better tests (i.e. if a +regression is found, regression tests should be added at the lowest-level +possible). + +| Tests path | Testing engine | Notes | +| ---------- | -------------- | ----- | +| `spec/features/` | [Capybara] + [RSpec] | If your spec has the `:js` metadata, the browser driver will be [Poltergeist], otherwise it's using [RackTest]. | +| `features/` | Spinach | Spinach tests are deprecated, [you shouldn't add new Spinach tests](#spinach-feature-tests). | + +[Capybara]: https://github.com/teamcapybara/capybara +[RSpec]: https://github.com/rspec/rspec-rails#feature-specs +[Poltergeist]: https://github.com/teamcapybara/capybara#poltergeist +[RackTest]: https://github.com/teamcapybara/capybara#racktest + +#### Best practices + +- Create only the necessary records in the database +- Test a happy path and a less happy path but that's it +- Every other possible path should be tested with Unit or Integration tests +- Test what's displayed on the page, not the internals of ActiveRecord models. + For instance, if you want to verify that a record was created, add + expectations that its attributes are displayed on the page, not that + `Model.count` increased by one. +- It's ok to look for DOM elements but don't abuse it since it makes the tests + more brittle + +If we're confident that the low-level components work well (and we should be if +we have enough Unit & Integration tests), we shouldn't need to duplicate their +thorough testing at the System test level. + +It's very easy to add tests, but a lot harder to remove or improve tests, so one +should take care of not introducing too many (slow and duplicated) specs. + +The reasons why we should follow these best practices are as follows: + +- System tests are slow to run since they spin up the entire application stack + in a headless browser, and even slower when they integrate a JS driver +- When system tests run with a JavaScript driver, the tests are run in a + different thread than the application. This means it does not share a + database connection and your test will have to commit the transactions in + order for the running application to see the data (and vice-versa). In that + case we need to truncate the database after each spec instead of simply + rolling back a transaction (the faster strategy that's in use for other kind + of tests). This is slower than transactions, however, so we want to use + truncation only when necessary. + +### Black-box tests or End-to-end tests + +GitLab consists of [multiple pieces] such as [GitLab Shell], [GitLab Workhorse], +[Gitaly], [GitLab Pages], [GitLab Runner], and GitLab Rails. All theses pieces +are configured and packaged by [GitLab Omnibus]. + +[GitLab QA] is a tool that allows to test that all these pieces integrate well +together by building a Docker image for a given version of GitLab Rails and +running feature tests (i.e. using Capybara) against it. + +The actual test scenarios and steps are [part of GitLab Rails] so that they're +always in-sync with the codebase. + +[multiple pieces]: ./architecture.md#components +[GitLab Shell]: https://gitlab.com/gitlab-org/gitlab-shell +[GitLab Workhorse]: https://gitlab.com/gitlab-org/gitlab-workhorse +[Gitaly]: https://gitlab.com/gitlab-org/gitaly +[GitLab Pages]: https://gitlab.com/gitlab-org/gitlab-pages +[GitLab Runner]: https://gitlab.com/gitlab-org/gitlab-ci-multi-runner +[GitLab Omnibus]: https://gitlab.com/gitlab-org/omnibus-gitlab +[GitLab QA]: https://gitlab.com/gitlab-org/gitlab-qa +[part of GitLab Rails]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/qa + +## How to test at the correct level? + +As many things in life, deciding what to test at each level of testing is a +trade-off: + +- Unit tests are usually cheap, and you should consider them like the basement + of your house: you need them to be confident that your code is behaving + correctly. However if you run only unit tests without integration / system tests, you might [miss] the [big] [picture]! +- Integration tests are a bit more expensive, but don't abuse them. A feature test + is often better than an integration test that is stubbing a lot of internals. +- System tests are expensive (compared to unit tests), even more if they require + a JavaScript driver. Make sure to follow the guidelines in the [Speed](#test-speed) + section. + +Another way to see it is to think about the "cost of tests", this is well +explained [in this article][tests-cost] and the basic idea is that the cost of a +test includes: + +- The time it takes to write the test +- The time it takes to run the test every time the suite runs +- The time it takes to understand the test +- The time it takes to fix the test if it breaks and the underlying code is OK +- Maybe, the time it takes to change the code to make the code testable. + +[miss]: https://twitter.com/ThePracticalDev/status/850748070698651649 +[big]: https://twitter.com/timbray/status/822470746773409794 +[picture]: https://twitter.com/withzombies/status/829716565834752000 +[tests-cost]: https://medium.com/table-xi/high-cost-tests-and-high-value-tests-a86e27a54df#.2ulyh3a4e + +## Frontend testing + +Please consult the [dedicated "Frontend testing" guide](./fe_guide/testing.md). ## RSpec @@ -117,53 +244,124 @@ it 'is overdue' do end ``` -### Test speed +### System / Feature tests -GitLab has a massive test suite that, without parallelization, can take more -than an hour to run. It's important that we make an effort to write tests that -are accurate and effective _as well as_ fast. +- Feature specs should be named `ROLE_ACTION_spec.rb`, such as + `user_changes_password_spec.rb`. +- Use only one `feature` block per feature spec file. +- Use scenario titles that describe the success and failure paths. +- Avoid scenario titles that add no information, such as "successfully". +- Avoid scenario titles that repeat the feature title. -Here are some things to keep in mind regarding test performance: +### Matchers -- `double` and `spy` are faster than `FactoryGirl.build(...)` -- `FactoryGirl.build(...)` and `.build_stubbed` are faster than `.create`. -- Don't `create` an object when `build`, `build_stubbed`, `attributes_for`, - `spy`, or `double` will do. Database persistence is slow! -- Use `create(:empty_project)` instead of `create(:project)` when you don't need - the underlying Git repository. Filesystem operations are slow! -- Don't mark a feature as requiring JavaScript (through `@javascript` in - Spinach or `js: true` in RSpec) unless it's _actually_ required for the test - to be valid. Headless browser testing is slow! +Custom matchers should be created to clarify the intent and/or hide the +complexity of RSpec expectations.They should be placed under +`spec/support/matchers/`. Matchers can be placed in subfolder if they apply to +a certain type of specs only (e.g. features, requests etc.) but shouldn't be if +they apply to multiple type of specs. -### Features / Integration +### Shared contexts -GitLab uses [rspec-rails feature specs] to test features in a browser -environment. These are [capybara] specs running on the headless [poltergeist] -driver. +All shared contexts should be be placed under `spec/support/shared_contexts/`. +Shared contexts can be placed in subfolder if they apply to a certain type of +specs only (e.g. features, requests etc.) but shouldn't be if they apply to +multiple type of specs. -- Feature specs live in `spec/features/` and should be named - `ROLE_ACTION_spec.rb`, such as `user_changes_password_spec.rb`. -- Use only one `feature` block per feature spec file. -- Use scenario titles that describe the success and failure paths. -- Avoid scenario titles that add no information, such as "successfully." -- Avoid scenario titles that repeat the feature title. +Each file should include only one context and have a descriptive name, e.g. +`spec/support/shared_contexts/controllers/githubish_import_controller_shared_context.rb`. -[rspec-rails feature specs]: https://github.com/rspec/rspec-rails#feature-specs -[capybara]: https://github.com/teamcapybara/capybara -[poltergeist]: https://github.com/teampoltergeist/poltergeist +### Shared examples -## Spinach (feature) tests +All shared examples should be be placed under `spec/support/shared_examples/`. +Shared examples can be placed in subfolder if they apply to a certain type of +specs only (e.g. features, requests etc.) but shouldn't be if they apply to +multiple type of specs. -GitLab [moved from Cucumber to Spinach](https://github.com/gitlabhq/gitlabhq/pull/1426) -for its feature/integration tests in September 2012. +Each file should include only one context and have a descriptive name, e.g. +`spec/support/shared_examples/controllers/githubish_import_controller_shared_example.rb`. -As of March 2016, we are [trying to avoid adding new Spinach -tests](https://gitlab.com/gitlab-org/gitlab-ce/issues/14121) going forward, -opting for [RSpec feature](#features-integration) specs. +### Helpers -Adding new Spinach scenarios is acceptable _only if_ the new scenario requires -no more than one new `step` definition. If more than that is required, the -test should be re-implemented using RSpec instead. +Helpers are usually modules that provide some methods to hide the complexity of +specific RSpec examples. You can define helpers in RSpec files if they're not +intended to be shared with other specs. Otherwise, they should be be placed +under `spec/support/helpers/`. Helpers can be placed in subfolder if they apply +to a certain type of specs only (e.g. features, requests etc.) but shouldn't be +if they apply to multiple type of specs. + +Helpers should follow the Rails naming / namespacing convention. For instance +`spec/support/helpers/cycle_analytics_helpers.rb` should define: + +```ruby +module Spec + module Support + module Helpers + module CycleAnalyticsHelpers + def create_commit_referencing_issue(issue, branch_name: random_git_name) + project.repository.add_branch(user, branch_name, 'master') + create_commit("Commit for ##{issue.iid}", issue.project, user, branch_name) + end + end + end + end +end +``` + +Helpers should not change the RSpec config. For instance, the helpers module +described above should not include: + +```ruby +RSpec.configure do |config| + config.include Spec::Support::Helpers::CycleAnalyticsHelpers +end +``` + +### Factories + +GitLab uses [factory_girl] as a test fixture replacement. + +- Factory definitions live in `spec/factories/`, named using the pluralization + of their corresponding model (`User` factories are defined in `users.rb`). +- There should be only one top-level factory definition per file. +- FactoryGirl methods are mixed in to all RSpec groups. This means you can (and + should) call `create(...)` instead of `FactoryGirl.create(...)`. +- Make use of [traits] to clean up definitions and usages. +- When defining a factory, don't define attributes that are not required for the + resulting record to pass validation. +- When instantiating from a factory, don't supply attributes that aren't + required by the test. +- Factories don't have to be limited to `ActiveRecord` objects. + [See example](https://gitlab.com/gitlab-org/gitlab-ce/commit/0b8cefd3b2385a21cfed779bd659978c0402766d). + +[factory_girl]: https://github.com/thoughtbot/factory_girl +[traits]: http://www.rubydoc.info/gems/factory_girl/file/GETTING_STARTED.md#Traits + +### Fixtures + +All fixtures should be be placed under `spec/fixtures/`. + +### Config + +RSpec config files are files that change the RSpec config (i.e. +`RSpec.configure do |config|` blocks). They should be placed under +`spec/support/config/`. + +Each file should be related to a specific domain, e.g. +`spec/support/config/capybara.rb`, `spec/support/config/carrierwave.rb`, etc. + +Helpers can be included in the `spec/support/config/rspec.rb` file. If a +helpers module applies only to a certain kind of specs, it should add modifiers +to the `config.include` call. For instance if +`spec/support/helpers/cycle_analytics_helpers.rb` applies to `:lib` and +`type: :model` specs only, you would write the following: + +```ruby +RSpec.configure do |config| + config.include Spec::Support::Helpers::CycleAnalyticsHelpers, :lib + config.include Spec::Support::Helpers::CycleAnalyticsHelpers, type: :model +end +``` ## Testing Rake Tasks @@ -201,6 +399,77 @@ describe 'gitlab:shell rake tasks' do end ``` +## Test speed + +GitLab has a massive test suite that, without [parallelization], can take hours +to run. It's important that we make an effort to write tests that are accurate +and effective _as well as_ fast. + +Here are some things to keep in mind regarding test performance: + +- `double` and `spy` are faster than `FactoryGirl.build(...)` +- `FactoryGirl.build(...)` and `.build_stubbed` are faster than `.create`. +- Don't `create` an object when `build`, `build_stubbed`, `attributes_for`, + `spy`, or `double` will do. Database persistence is slow! +- Use `create(:empty_project)` instead of `create(:project)` when you don't need + the underlying Git repository. Filesystem operations are slow! +- Don't mark a feature as requiring JavaScript (through `@javascript` in + Spinach or `:js` in RSpec) unless it's _actually_ required for the test + to be valid. Headless browser testing is slow! + +[parallelization]: #test-suite-parallelization-on-the-ci + +### Test suite parallelization on the CI + +Our current CI parallelization setup is as follows: + +1. The `knapsack` job in the prepare stage that is supposed to ensure we have a + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file: + - The `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file is fetched + from S3, if it's not here we initialize the file with `{}`. +1. Each `rspec x y` job are run with `knapsack rspec` and should have an evenly + distributed share of tests: + - It works because the jobs have access to the + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` since the "artifacts + from all previous stages are passed by default". [^1] + - the jobs set their own report path to + `KNAPSACK_REPORT_PATH=knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json`. + - if knapsack is doing its job, test files that are run should be listed under + `Report specs`, not under `Leftover specs`. +1. The `update-knapsack` job takes all the + `knapsack/${CI_PROJECT_NAME}/${JOB_NAME[0]}_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json` + files from the `rspec x y` jobs and merge them all together into a single + `knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file that is then + uploaded to S3. + +After that, the next pipeline will use the up-to-date +`knapsack/${CI_PROJECT_NAME}/rspec_report-master.json` file. The same strategy +is used for Spinach tests as well. + +### Monitoring + +The GitLab test suite is [monitored] and a [public dashboard] is available for +everyone to see. Feel free to look at the slowest test files and try to improve +them. + +[monitored]: ./performance.md#rspec-profiling +[public dashboard]: https://redash.gitlab.com/public/dashboards/l1WhHXaxrCWM5Ai9D7YDqHKehq6OU3bx5gssaiWe?org_slug=default + +## Spinach (feature) tests + +GitLab [moved from Cucumber to Spinach](https://github.com/gitlabhq/gitlabhq/pull/1426) +for its feature/integration tests in September 2012. + +As of March 2016, we are [trying to avoid adding new Spinach +tests](https://gitlab.com/gitlab-org/gitlab-ce/issues/14121) going forward, +opting for [RSpec feature](#features-integration) specs. + +Adding new Spinach scenarios is acceptable _only if_ the new scenario requires +no more than one new `step` definition. If more than that is required, the +test should be re-implemented using RSpec instead. + --- [Return to Development documentation](README.md) + +[^1]: /ci/yaml/README.html#dependencies diff --git a/doc/topics/authentication/index.md b/doc/topics/authentication/index.md index 47d3f05999d..eafd2fd9d04 100644 --- a/doc/topics/authentication/index.md +++ b/doc/topics/authentication/index.md @@ -15,11 +15,11 @@ This page gathers all the resources for the topic **Authentication** within GitL ## GitLab administrators - [LDAP (Community Edition)](../../administration/auth/ldap.md) -- [LDAP (Enterprise Edition)](https://docs.gitlab.com/ee/administration/auth/ldap-ee.md) +- [LDAP (Enterprise Edition)](https://docs.gitlab.com/ee/administration/auth/ldap-ee.html) - [Enforce Two-factor Authentication (2FA)](../../security/two_factor_authentication.md#enforce-two-factor-authentication-2fa) - **Articles:** - [Feature Highlight: LDAP Integration](https://about.gitlab.com/2014/07/10/feature-highlight-ldap-sync/) - - [Debugging LDAP](https://about.gitlab.com/handbook/support/workflows/ldap/debugging_ldap.md) + - [Debugging LDAP](https://about.gitlab.com/handbook/support/workflows/ldap/debugging_ldap.html) - **Integrations:** - [OmniAuth](../../integration/omniauth.md) - [Authentiq OmniAuth Provider](../../administration/auth/authentiq.md#authentiq-omniauth-provider) @@ -27,7 +27,7 @@ This page gathers all the resources for the topic **Authentication** within GitL - [CAS OmniAuth Provider](../../integration/cas.md) - [SAML OmniAuth Provider](../../integration/saml.md) - [Okta SSO provider](../../administration/auth/okta.md) - - [Kerberos integration (GitLab EE)](https://docs.gitlab.com/ee/integration/kerberos.md) + - [Kerberos integration (GitLab EE)](https://docs.gitlab.com/ee/integration/kerberos.html) ## API diff --git a/doc/topics/index.md b/doc/topics/index.md index 4a16f6f629c..ad388dff822 100644 --- a/doc/topics/index.md +++ b/doc/topics/index.md @@ -7,9 +7,10 @@ you through better understanding GitLab's concepts through our regular docs, and, when available, through articles (guides, tutorials, technical overviews, blog posts) and videos. +- [Authentication](authentication/index.md) - [Continuous Integration (GitLab CI)](../ci/README.md) - [Git](git/index.md) - [GitLab Installation](../install/README.md) - [GitLab Pages](../user/project/pages/index.md) ->**Note:** More topics will be available soon.
\ No newline at end of file +>**Note:** More topics will be available soon. diff --git a/doc/user/project/cycle_analytics.md b/doc/user/project/cycle_analytics.md index 62afd8cf247..8f6b530c033 100644 --- a/doc/user/project/cycle_analytics.md +++ b/doc/user/project/cycle_analytics.md @@ -5,10 +5,10 @@ Cycle Analytics measures the time it takes to go from an [idea to production] for each project you have. This is achieved by not only indicating the total time it -takes to reach at that point, but the total time is broken down into the +takes to reach that point, but the total time is broken down into the multiple stages an idea has to pass through to be shipped. -Cycle Analytics is that it is tightly coupled with the [GitLab flow] and +Cycle Analytics is tightly coupled with the [GitLab flow] and calculates a separate median for each stage. ## Overview diff --git a/doc/user/project/integrations/kubernetes.md b/doc/user/project/integrations/kubernetes.md index 2a890acde4d..f7d5e3a8ab2 100644 --- a/doc/user/project/integrations/kubernetes.md +++ b/doc/user/project/integrations/kubernetes.md @@ -60,7 +60,7 @@ to use terminals. Support is currently limited to the first container in the first pod of your environment. When enabled, the Kubernetes service adds [web terminal](../../../ci/environments.md#web-terminals) -support to your environments. This is based on the `exec` functionality found in +support to your [environments](../../../ci/environments.md). This is based on the `exec` functionality found in Docker and Kubernetes, so you get a new shell session within your existing containers. To use this integration, you should deploy to Kubernetes using the deployment variables above, ensuring any pods you create are labelled with diff --git a/lib/gitlab/ci/trace/stream.rb b/lib/gitlab/ci/trace/stream.rb index 2af94e2c60e..41dcf846fed 100644 --- a/lib/gitlab/ci/trace/stream.rb +++ b/lib/gitlab/ci/trace/stream.rb @@ -76,11 +76,14 @@ module Gitlab stream.each_line do |line| matches = line.scan(regex) next unless matches.is_a?(Array) + next if matches.empty? match = matches.flatten.last coverage = match.gsub(/\d+(\.\d+)?/).first - return coverage.to_f if coverage.present? + return coverage if coverage.present? end + + nil rescue # if bad regex or something goes wrong we dont want to interrupt transition # so we just silentrly ignore error for now diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index 63b8d0d3b9d..d0bd1299671 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -57,16 +57,16 @@ module Gitlab postgresql? ? "RANDOM()" : "RAND()" end - def true_value - if Gitlab::Database.postgresql? + def self.true_value + if postgresql? "'t'" else 1 end end - def false_value - if Gitlab::Database.postgresql? + def self.false_value + if postgresql? "'f'" else 0 diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index a6873ac63a0..6dabbe0264c 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -115,6 +115,14 @@ module Gitlab execute('SET statement_timeout TO 0') if Database.postgresql? end + def true_value + Database.true_value + end + + def false_value + Database.false_value + end + # Updates the value of a column in batches. # # This method updates the table in batches of 5% of the total row count. diff --git a/lib/tasks/gitlab/update_templates.rake b/lib/tasks/gitlab/update_templates.rake index dbdfb335a5c..cb2adc81c9d 100644 --- a/lib/tasks/gitlab/update_templates.rake +++ b/lib/tasks/gitlab/update_templates.rake @@ -44,7 +44,7 @@ namespace :gitlab do ), Template.new( "https://gitlab.com/gitlab-org/gitlab-ci-yml.git", - /(\.{1,2}|LICENSE|Pages|autodeploy|\.gitlab-ci.yml)\z/ + /(\.{1,2}|LICENSE|CONTRIBUTING.md|Pages|autodeploy|\.gitlab-ci.yml)\z/ ) ].freeze diff --git a/qa/qa/page/main/menu.rb b/qa/qa/page/main/menu.rb index 45db7a92fa4..7ce4e9009f5 100644 --- a/qa/qa/page/main/menu.rb +++ b/qa/qa/page/main/menu.rb @@ -11,7 +11,7 @@ module QA end def go_to_admin_area - within_user_menu { click_link 'Admin Area' } + within_user_menu { click_link 'Admin area' } end def sign_out diff --git a/spec/features/discussion_comments/commit_spec.rb b/spec/features/discussion_comments/commit_spec.rb new file mode 100644 index 00000000000..96e0b78f6b9 --- /dev/null +++ b/spec/features/discussion_comments/commit_spec.rb @@ -0,0 +1,18 @@ +require 'spec_helper' + +describe 'Discussion Comments Merge Request', :feature, :js do + include RepoHelpers + + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:merge_request) { create(:merge_request, source_project: project) } + + before do + project.add_master(user) + login_as(user) + + visit namespace_project_commit_path(project.namespace, project, sample_commit.id) + end + + it_behaves_like 'discussion comments', 'commit' +end diff --git a/spec/features/discussion_comments/issue_spec.rb b/spec/features/discussion_comments/issue_spec.rb new file mode 100644 index 00000000000..ccc9efccd18 --- /dev/null +++ b/spec/features/discussion_comments/issue_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe 'Discussion Comments Issue', :feature, :js do + let(:user) { create(:user) } + let(:project) { create(:empty_project) } + let(:issue) { create(:issue, project: project) } + + before do + project.add_master(user) + login_as(user) + + visit namespace_project_issue_path(project.namespace, project, issue) + end + + it_behaves_like 'discussion comments', 'issue' +end diff --git a/spec/features/discussion_comments/merge_request_spec.rb b/spec/features/discussion_comments/merge_request_spec.rb new file mode 100644 index 00000000000..f99ebeb9cd9 --- /dev/null +++ b/spec/features/discussion_comments/merge_request_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe 'Discussion Comments Merge Request', :feature, :js do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:merge_request) { create(:merge_request, source_project: project) } + + before do + project.add_master(user) + login_as(user) + + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + end + + it_behaves_like 'discussion comments', 'merge request' +end diff --git a/spec/features/discussion_comments/snippets_spec.rb b/spec/features/discussion_comments/snippets_spec.rb new file mode 100644 index 00000000000..19a306511b2 --- /dev/null +++ b/spec/features/discussion_comments/snippets_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe 'Discussion Comments Issue', :feature, :js do + let(:user) { create(:user) } + let(:project) { create(:empty_project) } + let(:snippet) { create(:project_snippet, :private, project: project, author: user) } + + before do + project.add_master(user) + login_as(user) + + visit namespace_project_snippet_path(project.namespace, project, snippet) + end + + it_behaves_like 'discussion comments', 'snippet' +end diff --git a/spec/javascripts/issue_spec.js b/spec/javascripts/issue_spec.js index aabc8bea12f..9a2570ef7e9 100644 --- a/spec/javascripts/issue_spec.js +++ b/spec/javascripts/issue_spec.js @@ -1,18 +1,17 @@ -/* eslint-disable space-before-function-paren, no-var, one-var, one-var-declaration-per-line, no-use-before-define, comma-dangle, max-len */ +/* eslint-disable space-before-function-paren, one-var, one-var-declaration-per-line, no-use-before-define, comma-dangle, max-len */ import Issue from '~/issue'; require('~/lib/utils/text_utility'); describe('Issue', function() { - var INVALID_URL = 'http://goesnowhere.nothing/whereami'; - var $boxClosed, $boxOpen, $btnClose, $btnReopen; + let $boxClosed, $boxOpen, $btnClose, $btnReopen; preloadFixtures('issues/closed-issue.html.raw'); preloadFixtures('issues/issue-with-task-list.html.raw'); preloadFixtures('issues/open-issue.html.raw'); function expectErrorMessage() { - var $flashMessage = $('div.flash-alert'); + const $flashMessage = $('div.flash-alert'); expect($flashMessage).toExist(); expect($flashMessage).toBeVisible(); expect($flashMessage).toHaveText('Unable to update this issue at this time.'); @@ -26,10 +25,28 @@ describe('Issue', function() { expectVisibility($btnReopen, !isIssueOpen); } - function expectPendingRequest(req, $triggeredButton) { - expect(req.type).toBe('PUT'); - expect(req.url).toBe($triggeredButton.attr('href')); - expect($triggeredButton).toHaveProp('disabled', true); + function expectNewBranchButtonState(isPending, canCreate) { + if (Issue.$btnNewBranch.length === 0) { + return; + } + + const $available = Issue.$btnNewBranch.find('.available'); + expect($available).toHaveText('New branch'); + + if (!isPending && canCreate) { + expect($available).toBeVisible(); + } else { + expect($available).toBeHidden(); + } + + const $unavailable = Issue.$btnNewBranch.find('.unavailable'); + expect($unavailable).toHaveText('New branch unavailable'); + + if (!isPending && !canCreate) { + expect($unavailable).toBeVisible(); + } else { + expect($unavailable).toBeHidden(); + } } function expectVisibility($element, shouldBeVisible) { @@ -81,100 +98,107 @@ describe('Issue', function() { }); }); - describe('close issue', function() { - beforeEach(function() { - loadFixtures('issues/open-issue.html.raw'); - findElements(); - this.issue = new Issue(); - - expectIssueState(true); - }); + [true, false].forEach((isIssueInitiallyOpen) => { + describe(`with ${isIssueInitiallyOpen ? 'open' : 'closed'} issue`, function() { + const action = isIssueInitiallyOpen ? 'close' : 'reopen'; + + function ajaxSpy(req) { + if (req.url === this.$triggeredButton.attr('href')) { + expect(req.type).toBe('PUT'); + expect(this.$triggeredButton).toHaveProp('disabled', true); + expectNewBranchButtonState(true, false); + return this.issueStateDeferred; + } else if (req.url === Issue.$btnNewBranch.data('path')) { + expect(req.type).toBe('get'); + expectNewBranchButtonState(true, false); + return this.canCreateBranchDeferred; + } + + expect(req.url).toBe('unexpected'); + return null; + } + + beforeEach(function() { + if (isIssueInitiallyOpen) { + loadFixtures('issues/open-issue.html.raw'); + } else { + loadFixtures('issues/closed-issue.html.raw'); + } + + findElements(); + this.issue = new Issue(); + expectIssueState(isIssueInitiallyOpen); + this.$triggeredButton = isIssueInitiallyOpen ? $btnClose : $btnReopen; + + this.$projectIssuesCounter = $('.issue_counter'); + this.$projectIssuesCounter.text('1,001'); + + this.issueStateDeferred = new jQuery.Deferred(); + this.canCreateBranchDeferred = new jQuery.Deferred(); + + spyOn(jQuery, 'ajax').and.callFake(ajaxSpy.bind(this)); + }); - it('closes an issue', function() { - spyOn(jQuery, 'ajax').and.callFake(function(req) { - expectPendingRequest(req, $btnClose); - req.success({ + it(`${action}s the issue`, function() { + this.$triggeredButton.trigger('click'); + this.issueStateDeferred.resolve({ id: 34 }); - }); - - $btnClose.trigger('click'); + this.canCreateBranchDeferred.resolve({ + can_create_branch: !isIssueInitiallyOpen + }); - expectIssueState(false); - expect($btnClose).toHaveProp('disabled', false); - expect($('.issue_counter')).toHaveText(0); - }); + expectIssueState(!isIssueInitiallyOpen); + expect(this.$triggeredButton).toHaveProp('disabled', false); + expect(this.$projectIssuesCounter.text()).toBe(isIssueInitiallyOpen ? '1,000' : '1,002'); + expectNewBranchButtonState(false, !isIssueInitiallyOpen); + }); - it('fails to close an issue with success:false', function() { - spyOn(jQuery, 'ajax').and.callFake(function(req) { - expectPendingRequest(req, $btnClose); - req.success({ + it(`fails to ${action} the issue if saved:false`, function() { + this.$triggeredButton.trigger('click'); + this.issueStateDeferred.resolve({ saved: false }); - }); - - $btnClose.attr('href', INVALID_URL); - $btnClose.trigger('click'); - - expectIssueState(true); - expect($btnClose).toHaveProp('disabled', false); - expectErrorMessage(); - expect($('.issue_counter')).toHaveText(1); - }); + this.canCreateBranchDeferred.resolve({ + can_create_branch: isIssueInitiallyOpen + }); - it('fails to closes an issue with HTTP error', function() { - spyOn(jQuery, 'ajax').and.callFake(function(req) { - expectPendingRequest(req, $btnClose); - req.error(); + expectIssueState(isIssueInitiallyOpen); + expect(this.$triggeredButton).toHaveProp('disabled', false); + expectErrorMessage(); + expect(this.$projectIssuesCounter.text()).toBe('1,001'); + expectNewBranchButtonState(false, isIssueInitiallyOpen); }); - $btnClose.attr('href', INVALID_URL); - $btnClose.trigger('click'); - - expectIssueState(true); - expect($btnClose).toHaveProp('disabled', true); - expectErrorMessage(); - expect($('.issue_counter')).toHaveText(1); - }); - - it('updates counter', () => { - spyOn(jQuery, 'ajax').and.callFake(function(req) { - expectPendingRequest(req, $btnClose); - req.success({ - id: 34 + it(`fails to ${action} the issue if HTTP error occurs`, function() { + this.$triggeredButton.trigger('click'); + this.issueStateDeferred.reject(); + this.canCreateBranchDeferred.resolve({ + can_create_branch: isIssueInitiallyOpen }); - }); - expect($('.issue_counter')).toHaveText(1); - $('.issue_counter').text('1,001'); - expect($('.issue_counter').text()).toEqual('1,001'); - $btnClose.trigger('click'); - expect($('.issue_counter').text()).toEqual('1,000'); - }); - }); + expectIssueState(isIssueInitiallyOpen); + expect(this.$triggeredButton).toHaveProp('disabled', true); + expectErrorMessage(); + expect(this.$projectIssuesCounter.text()).toBe('1,001'); + expectNewBranchButtonState(false, isIssueInitiallyOpen); + }); - describe('reopen issue', function() { - beforeEach(function() { - loadFixtures('issues/closed-issue.html.raw'); - findElements(); - this.issue = new Issue(); + it('disables the new branch button if Ajax call fails', function() { + this.$triggeredButton.trigger('click'); + this.issueStateDeferred.reject(); + this.canCreateBranchDeferred.reject(); - expectIssueState(false); - }); - - it('reopens an issue', function() { - spyOn(jQuery, 'ajax').and.callFake(function(req) { - expectPendingRequest(req, $btnReopen); - req.success({ - id: 34 - }); + expectNewBranchButtonState(false, false); }); - $btnReopen.trigger('click'); + it('does not trigger Ajax call if new branch button is missing', function() { + Issue.$btnNewBranch = $(); + this.canCreateBranchDeferred = null; - expectIssueState(true); - expect($btnReopen).toHaveProp('disabled', false); - expect($('.issue_counter')).toHaveText(1); + this.$triggeredButton.trigger('click'); + this.issueStateDeferred.reject(); + }); }); }); }); diff --git a/spec/javascripts/lib/utils/poll_spec.js b/spec/javascripts/lib/utils/poll_spec.js index e3429c2a1cb..918b6d32c43 100644 --- a/spec/javascripts/lib/utils/poll_spec.js +++ b/spec/javascripts/lib/utils/poll_spec.js @@ -4,6 +4,20 @@ import Poll from '~/lib/utils/poll'; Vue.use(VueResource); +const waitForAllCallsToFinish = (service, waitForCount, successCallback) => { + const timer = () => { + setTimeout(() => { + if (service.fetch.calls.count() === waitForCount) { + successCallback(); + } else { + timer(); + } + }, 5); + }; + + timer(); +}; + class ServiceMock { constructor(endpoint) { this.service = Vue.resource(endpoint); @@ -16,6 +30,7 @@ class ServiceMock { describe('Poll', () => { let callbacks; + let service; beforeEach(() => { callbacks = { @@ -23,8 +38,11 @@ describe('Poll', () => { error: () => {}, }; + service = new ServiceMock('endpoint'); + spyOn(callbacks, 'success'); spyOn(callbacks, 'error'); + spyOn(service, 'fetch').and.callThrough(); }); it('calls the success callback when no header for interval is provided', (done) => { @@ -35,19 +53,20 @@ describe('Poll', () => { Vue.http.interceptors.push(successInterceptor); new Poll({ - resource: new ServiceMock('endpoint'), + resource: service, method: 'fetch', successCallback: callbacks.success, errorCallback: callbacks.error, }).makeRequest(); - setTimeout(() => { + waitForAllCallsToFinish(service, 1, () => { expect(callbacks.success).toHaveBeenCalled(); expect(callbacks.error).not.toHaveBeenCalled(); + + Vue.http.interceptors = _.without(Vue.http.interceptors, successInterceptor); + done(); }, 0); - - Vue.http.interceptors = _.without(Vue.http.interceptors, successInterceptor); }); it('calls the error callback whe the http request returns an error', (done) => { @@ -58,19 +77,19 @@ describe('Poll', () => { Vue.http.interceptors.push(errorInterceptor); new Poll({ - resource: new ServiceMock('endpoint'), + resource: service, method: 'fetch', successCallback: callbacks.success, errorCallback: callbacks.error, }).makeRequest(); - setTimeout(() => { + waitForAllCallsToFinish(service, 1, () => { expect(callbacks.success).not.toHaveBeenCalled(); expect(callbacks.error).toHaveBeenCalled(); - done(); - }, 0); + Vue.http.interceptors = _.without(Vue.http.interceptors, errorInterceptor); - Vue.http.interceptors = _.without(Vue.http.interceptors, errorInterceptor); + done(); + }); }); it('should call the success callback when the interval header is -1', (done) => { @@ -81,7 +100,7 @@ describe('Poll', () => { Vue.http.interceptors.push(intervalInterceptor); new Poll({ - resource: new ServiceMock('endpoint'), + resource: service, method: 'fetch', successCallback: callbacks.success, errorCallback: callbacks.error, @@ -90,10 +109,11 @@ describe('Poll', () => { setTimeout(() => { expect(callbacks.success).toHaveBeenCalled(); expect(callbacks.error).not.toHaveBeenCalled(); + + Vue.http.interceptors = _.without(Vue.http.interceptors, intervalInterceptor); + done(); }, 0); - - Vue.http.interceptors = _.without(Vue.http.interceptors, intervalInterceptor); }); it('starts polling when http status is 200 and interval header is provided', (done) => { @@ -103,26 +123,28 @@ describe('Poll', () => { Vue.http.interceptors.push(pollInterceptor); - const service = new ServiceMock('endpoint'); - spyOn(service, 'fetch').and.callThrough(); - - new Poll({ + const Polling = new Poll({ resource: service, method: 'fetch', data: { page: 1 }, successCallback: callbacks.success, errorCallback: callbacks.error, - }).makeRequest(); + }); + + Polling.makeRequest(); + + waitForAllCallsToFinish(service, 2, () => { + Polling.stop(); - setTimeout(() => { expect(service.fetch.calls.count()).toEqual(2); expect(service.fetch).toHaveBeenCalledWith({ page: 1 }); expect(callbacks.success).toHaveBeenCalled(); expect(callbacks.error).not.toHaveBeenCalled(); - done(); - }, 5); - Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + + done(); + }); }); describe('stop', () => { @@ -133,9 +155,6 @@ describe('Poll', () => { Vue.http.interceptors.push(pollInterceptor); - const service = new ServiceMock('endpoint'); - spyOn(service, 'fetch').and.callThrough(); - const Polling = new Poll({ resource: service, method: 'fetch', @@ -150,14 +169,15 @@ describe('Poll', () => { Polling.makeRequest(); - setTimeout(() => { + waitForAllCallsToFinish(service, 1, () => { expect(service.fetch.calls.count()).toEqual(1); expect(service.fetch).toHaveBeenCalledWith({ page: 1 }); expect(Polling.stop).toHaveBeenCalled(); - done(); - }, 100); - Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + + done(); + }); }); }); @@ -169,10 +189,6 @@ describe('Poll', () => { Vue.http.interceptors.push(pollInterceptor); - const service = new ServiceMock('endpoint'); - - spyOn(service, 'fetch').and.callThrough(); - const Polling = new Poll({ resource: service, method: 'fetch', @@ -187,17 +203,22 @@ describe('Poll', () => { }); spyOn(Polling, 'stop').and.callThrough(); + spyOn(Polling, 'restart').and.callThrough(); Polling.makeRequest(); - setTimeout(() => { + waitForAllCallsToFinish(service, 2, () => { + Polling.stop(); + expect(service.fetch.calls.count()).toEqual(2); expect(service.fetch).toHaveBeenCalledWith({ page: 1 }); expect(Polling.stop).toHaveBeenCalled(); - done(); - }, 10); + expect(Polling.restart).toHaveBeenCalled(); - Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + Vue.http.interceptors = _.without(Vue.http.interceptors, pollInterceptor); + + done(); + }); }); }); }); diff --git a/spec/lib/gitlab/ci/trace/stream_spec.rb b/spec/lib/gitlab/ci/trace/stream_spec.rb index f1a1a71c528..2e57ccef182 100644 --- a/spec/lib/gitlab/ci/trace/stream_spec.rb +++ b/spec/lib/gitlab/ci/trace/stream_spec.rb @@ -167,7 +167,7 @@ describe Gitlab::Ci::Trace::Stream do let(:data) { 'Coverage 1033 / 1051 LOC (98.29%) covered' } let(:regex) { '\(\d+.\d+\%\) covered' } - it { is_expected.to eq(98.29) } + it { is_expected.to eq("98.29") } end context 'valid content & bad regex' do @@ -188,14 +188,14 @@ describe Gitlab::Ci::Trace::Stream do let(:data) { ' (98.39%) covered. (98.29%) covered' } let(:regex) { '\(\d+.\d+\%\) covered' } - it { is_expected.to eq(98.29) } + it { is_expected.to eq("98.29") } end context 'using a regex capture' do let(:data) { 'TOTAL 9926 3489 65%' } let(:regex) { 'TOTAL\s+\d+\s+\d+\s+(\d{1,3}\%)' } - it { is_expected.to eq(65) } + it { is_expected.to eq("65") } end end end diff --git a/spec/lib/gitlab/ci/trace_spec.rb b/spec/lib/gitlab/ci/trace_spec.rb index 69e8dc9220d..9cb0b62590a 100644 --- a/spec/lib/gitlab/ci/trace_spec.rb +++ b/spec/lib/gitlab/ci/trace_spec.rb @@ -40,12 +40,24 @@ describe Gitlab::Ci::Trace do describe '#extract_coverage' do let(:regex) { '\(\d+.\d+\%\) covered' } - before do - trace.set('Coverage 1033 / 1051 LOC (98.29%) covered') + context 'matching coverage' do + before do + trace.set('Coverage 1033 / 1051 LOC (98.29%) covered') + end + + it "returns valid coverage" do + expect(trace.extract_coverage(regex)).to eq("98.29") + end end - it "returns valid coverage" do - expect(trace.extract_coverage(regex)).to eq(98.29) + context 'no coverage' do + before do + trace.set('No coverage') + end + + it 'returs nil' do + expect(trace.extract_coverage(regex)).to be_nil + end end end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index 8d109cba71a..a044b871730 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -175,6 +175,50 @@ describe Gitlab::Database::MigrationHelpers, lib: true do end end + describe '#true_value' do + context 'using PostgreSQL' do + before do + expect(Gitlab::Database).to receive(:postgresql?).and_return(true) + end + + it 'returns the appropriate value' do + expect(model.true_value).to eq("'t'") + end + end + + context 'using MySQL' do + before do + expect(Gitlab::Database).to receive(:postgresql?).and_return(false) + end + + it 'returns the appropriate value' do + expect(model.true_value).to eq(1) + end + end + end + + describe '#false_value' do + context 'using PostgreSQL' do + before do + expect(Gitlab::Database).to receive(:postgresql?).and_return(true) + end + + it 'returns the appropriate value' do + expect(model.false_value).to eq("'f'") + end + end + + context 'using MySQL' do + before do + expect(Gitlab::Database).to receive(:postgresql?).and_return(false) + end + + it 'returns the appropriate value' do + expect(model.false_value).to eq(0) + end + end + end + describe '#update_column_in_batches' do before do create_list(:empty_project, 5) diff --git a/spec/lib/gitlab/database_spec.rb b/spec/lib/gitlab/database_spec.rb index 4ce4e6e1034..9b1d66a1b1c 100644 --- a/spec/lib/gitlab/database_spec.rb +++ b/spec/lib/gitlab/database_spec.rb @@ -150,13 +150,13 @@ describe Gitlab::Database, lib: true do it 'returns correct value for PostgreSQL' do expect(described_class).to receive(:postgresql?).and_return(true) - expect(MigrationTest.new.true_value).to eq "'t'" + expect(described_class.true_value).to eq "'t'" end it 'returns correct value for MySQL' do expect(described_class).to receive(:postgresql?).and_return(false) - expect(MigrationTest.new.true_value).to eq 1 + expect(described_class.true_value).to eq 1 end end @@ -164,13 +164,13 @@ describe Gitlab::Database, lib: true do it 'returns correct value for PostgreSQL' do expect(described_class).to receive(:postgresql?).and_return(true) - expect(MigrationTest.new.false_value).to eq "'f'" + expect(described_class.false_value).to eq "'f'" end it 'returns correct value for MySQL' do expect(described_class).to receive(:postgresql?).and_return(false) - expect(MigrationTest.new.false_value).to eq 0 + expect(described_class.false_value).to eq 0 end end end diff --git a/spec/models/label_spec.rb b/spec/models/label_spec.rb index a9139f7d4ab..80ca19acdda 100644 --- a/spec/models/label_spec.rb +++ b/spec/models/label_spec.rb @@ -42,11 +42,27 @@ describe Label, models: true do end end + describe '#color' do + it 'strips color' do + label = described_class.new(color: ' #abcdef ') + label.valid? + + expect(label.color).to eq('#abcdef') + end + end + describe '#title' do it 'sanitizes title' do label = described_class.new(title: '<b>foo & bar?</b>') expect(label.title).to eq('foo & bar?') end + + it 'strips title' do + label = described_class.new(title: ' label ') + label.valid? + + expect(label.title).to eq('label') + end end describe 'priorization' do diff --git a/spec/features/discussion_comments_spec.rb b/spec/support/features/discussion_comments_shared_example.rb index ae778118c5c..1a061ef069e 100644 --- a/spec/features/discussion_comments_spec.rb +++ b/spec/support/features/discussion_comments_shared_example.rb @@ -1,5 +1,3 @@ -require 'spec_helper' - shared_examples 'discussion comments' do |resource_name| let(:form_selector) { '.js-main-target-form' } let(:dropdown_selector) { "#{form_selector} .comment-type-dropdown" } @@ -9,11 +7,9 @@ shared_examples 'discussion comments' do |resource_name| let(:close_selector) { "#{form_selector} .btn-comment-and-close" } let(:comments_selector) { '.timeline > .note.timeline-entry' } - it 'should show a comment type toggle' do + it 'clicking "Comment" will post a comment' do expect(page).to have_selector toggle_selector - end - it 'clicking "Comment" will post a comment' do find("#{form_selector} .note-textarea").send_keys('a') find(submit_selector).click @@ -49,44 +45,29 @@ shared_examples 'discussion comments' do |resource_name| find(toggle_selector).click end - it 'opens a comment type dropdown with "Comment" and "Start discussion"' do + it 'has a "Comment" item (selected by default) and "Start discussion" item' do expect(page).to have_selector menu_selector - end - - it 'has a "Comment" item' do - menu = find(menu_selector) - - expect(menu).to have_content 'Comment' - expect(menu).to have_content "Add a general comment to this #{resource_name}." - end - it 'has a "Start discussion" item' do - menu = find(menu_selector) - - expect(menu).to have_content 'Start discussion' - expect(menu).to have_content "Discuss a specific suggestion or question#{' that needs to be resolved' if resource_name == 'merge request'}." - end - - it 'has the "Comment" item selected by default' do find("#{menu_selector} li", match: :first) items = all("#{menu_selector} li") expect(items.first).to have_content 'Comment' + expect(items.first).to have_content "Add a general comment to this #{resource_name}." expect(items.first).to have_selector '.fa-check' expect(items.first['class']).to match 'droplab-item-selected' expect(items.last).to have_content 'Start discussion' + expect(items.last).to have_content "Discuss a specific suggestion or question#{' that needs to be resolved' if resource_name == 'merge request'}." expect(items.last).not_to have_selector '.fa-check' expect(items.last['class']).not_to match 'droplab-item-selected' end - it 'closes the menu when clicking the toggle' do + it 'closes the menu when clicking the toggle or body' do find(toggle_selector).click expect(page).not_to have_selector menu_selector - end - it 'closes the menu when clicking the body' do + find(toggle_selector).click find('body').click expect(page).not_to have_selector menu_selector @@ -104,12 +85,10 @@ shared_examples 'discussion comments' do |resource_name| all("#{menu_selector} li").last.click end - it 'updates the note_type input to "DiscussionNote"' do - expect(find("#{form_selector} #note_type", visible: false).value).to eq('DiscussionNote') - end - - it 'updates the submit button text' do + it 'updates the submit button text, note_type input and closes the dropdown' do expect(find(dropdown_selector)).to have_content 'Start discussion' + expect(find("#{form_selector} #note_type", visible: false).value).to eq('DiscussionNote') + expect(page).not_to have_selector menu_selector end if resource_name =~ /(issue|merge request)/ @@ -124,10 +103,6 @@ shared_examples 'discussion comments' do |resource_name| end end - it 'closes the dropdown' do - expect(page).not_to have_selector menu_selector - end - it 'clicking "Start discussion" will post a discussion' do find(submit_selector).click @@ -176,12 +151,10 @@ shared_examples 'discussion comments' do |resource_name| find("#{menu_selector} li", match: :first).click end - it 'clears the note_type input"' do - expect(find("#{form_selector} #note_type", visible: false).value).to eq('') - end - - it 'updates the submit button text' do + it 'updates the submit button text, clears the note_type input and closes the dropdown' do expect(find(dropdown_selector)).to have_content 'Comment' + expect(find("#{form_selector} #note_type", visible: false).value).to eq('') + expect(page).not_to have_selector menu_selector end if resource_name =~ /(issue|merge request)/ @@ -196,10 +169,6 @@ shared_examples 'discussion comments' do |resource_name| end end - it 'closes the dropdown' do - expect(page).not_to have_selector menu_selector - end - it 'should have "Comment" selected when opening the menu' do find(toggle_selector).click @@ -242,54 +211,3 @@ shared_examples 'discussion comments' do |resource_name| end end end - -describe 'Discussion Comments', :feature, :js do - include RepoHelpers - - let(:user) { create(:user) } - let(:project) { create(:project) } - - before do - project.team << [user, :developer] - - login_as(user) - end - - describe 'on a merge request' do - let(:merge_request) { create(:merge_request, source_project: project) } - - before do - visit namespace_project_merge_request_path(project.namespace, project, merge_request) - end - - it_behaves_like 'discussion comments', 'merge request' - end - - describe 'on an issue' do - let(:issue) { create(:issue, project: project) } - - before do - visit namespace_project_issue_path(project.namespace, project, issue) - end - - it_behaves_like 'discussion comments', 'issue' - end - - describe 'on an snippet' do - let(:snippet) { create(:project_snippet, :private, project: project, author: user) } - - before do - visit namespace_project_snippet_path(project.namespace, project, snippet) - end - - it_behaves_like 'discussion comments', 'snippet' - end - - describe 'on a commit' do - before do - visit namespace_project_commit_path(project.namespace, project, sample_commit.id) - end - - it_behaves_like 'discussion comments', 'commit' - end -end diff --git a/vendor/gitlab-ci-yml/CONTRIBUTING.md b/vendor/gitlab-ci-yml/CONTRIBUTING.md new file mode 100644 index 00000000000..6e5160a2487 --- /dev/null +++ b/vendor/gitlab-ci-yml/CONTRIBUTING.md @@ -0,0 +1,5 @@ +The canonical repository for `.gitlab-ci.yml` templates is +https://gitlab.com/gitlab-org/gitlab-ci-yml. + +GitLab only mirrors the templates. Please submit your merge requests to +https://gitlab.com/gitlab-org/gitlab-ci-yml. |
