More complete specs wrt. coverage, cleanup

app/models/repository.rb

@@ -31,9 +31,6 @@ class Repository < ActiveRecord::Base
   include Redmine::Ciphering
   include OpenProject::Scm::ManageableRepository
 
-  class BuildFailed < StandardError
-  end
-
   belongs_to :project
   has_many :changesets, order: "#{Changeset.table_name}.committed_on DESC, #{Changeset.table_name}.id DESC"
 
@@ -83,7 +80,7 @@ class Repository < ActiveRecord::Base
   def scm
     @scm ||= scm_adapter.new(url, root_url,
                              login, password, path_encoding)
-    self.root_url = @scm.root_url if root_url.blank?
+    @scm.root_url = @scm.root_url.presence || root_url
     @scm
   end
 
@@ -271,7 +268,8 @@ class Repository < ActiveRecord::Base
   #
   # @param [Symbol] type     SCM tag to determine the type this repository should be built as
   #
-  # @raise [Repository::BuildFailed] Raised when the instance could not be built
+  # @raise [OpenProject::Scm::RepositoryBuildError]
+  #                                  Raised when the instance could not be built
   #                                  given the parameters.
   # @raise [::NameError] Raised when the given +vendor+ could not be resolved to a class.
   def self.build(project, vendor, params, type)
@@ -298,8 +296,9 @@ class Repository < ActiveRecord::Base
     klass = OpenProject::Scm::Manager.registered[vendor]
 
     if klass.nil?
-      raise BuildFailed.new I18n.t('repositories.errors.disabled_or_unknown_vendor',
-                                   vendor: vendor)
+      raise OpenProject::Scm::Exceptions::RepositoryBuildError.new(
+        I18n.t('repositories.errors.disabled_or_unknown_vendor', vendor: vendor)
+      )
     else
       klass
     end
@@ -337,7 +336,9 @@ class Repository < ActiveRecord::Base
     if service.call
       true
     else
-      raise BuildFailed.new service.localized_rejected_reason
+      raise OpenProject::Scm::Exceptions::RepositoryBuildError.new(
+        service.localized_rejected_reason
+      )
     end
   end
 end

app/models/repository/git.rb

@@ -37,10 +37,12 @@ class Repository::Git < Repository
     OpenProject::Scm::Adapters::Git
   end
 
-  def configure(scm_type, args)
+  def configure(scm_type, _args)
     if scm_type == MANAGED_TYPE
       unless manageable?
-        raise BuildError.new I18n.t('repositories.managed.error_not_manageable')
+        raise OpenProject::Scm::Exceptions::RepositoryBuildError.new(
+          I18n.t('repositories.managed.error_not_manageable')
+        )
       end
 
       self.root_url = managed_repository_path

app/models/repository/subversion.rb

@@ -38,10 +38,12 @@ class Repository::Subversion < Repository
     OpenProject::Scm::Adapters::Subversion
   end
 
-  def configure(scm_type, args)
+  def configure(scm_type, _args)
     if scm_type == MANAGED_TYPE
       unless manageable?
-        raise BuildError.new I18n.t('repositories.managed.error_not_manageable')
+        raise OpenProject::Scm::Exceptions::RepositoryBuildError.new(
+          I18n.t('repositories.managed.error_not_manageable')
+        )
       end
 
       self.root_url = managed_repository_path

app/services/scm/delete_managed_repository_service.rb

@@ -29,7 +29,7 @@
 
 ##
 # Implements the asynchronous deletion of a local repository.
-Scm::DeleteRepositoryService = Struct.new :repository do
+Scm::DeleteManagedRepositoryService = Struct.new :repository do
   ##
   # Checks if a given repository may be deleted
   # Registers an asynchronous job to delete the repository on disk.
@@ -38,15 +38,17 @@ Scm::DeleteRepositoryService = Struct.new :repository do
     if repository.manageable? && repository.managed?
 
       # Create necessary changes to repository to mark
-      # it as managed by OP, but create asynchronously.
+      # it as managed by OP, but delete asynchronously.
       managed_path = repository.managed_repository_path
       managed_root = repository.managed_root
 
       if File.directory?(managed_path)
         Delayed::Job.enqueue Scm::DeleteRepositoryJob.new(managed_root, managed_path)
       end
+
+      true
+    else
+      false
     end
-    repository.destroy
-    true
   end
 end

app/services/scm/repository_factory_service.rb

@@ -43,7 +43,9 @@ Scm::RepositoryFactoryService = Struct.new :project, :params do
       if repository.save
         repository
       else
-        raise Repository::BuildFailed.new repository.errors.full_messages.join("\n")
+        raise OpenProject::Scm::Exceptions::RepositoryBuildError.new(
+          repository.errors.full_messages.join("\n")
+        )
       end
     end
   end
@@ -55,7 +57,7 @@ Scm::RepositoryFactoryService = Struct.new :project, :params do
   # @return [Boolean] true iff the repository was built
   def build_temporary
     build_guarded do
-     build_with_type(nil)
+      build_with_type(nil)
     end
   end
 
@@ -67,7 +69,7 @@ Scm::RepositoryFactoryService = Struct.new :project, :params do
 
   ##
   # Helper to actually build the repository and return it.
-  # May raise +Repository::BuildFailed+ internally.
+  # May raise +OpenProject::Scm::Exceptions::RepositoryBuildError+ internally.
   #
   # @param [Symbol] scm_type Type to build the repository with. May be nil
   #                          during temporary build
@@ -83,7 +85,7 @@ Scm::RepositoryFactoryService = Struct.new :project, :params do
   def build_guarded
     @repository = yield
     @repository.present?
-  rescue Repository::BuildFailed => e
+  rescue OpenProject::Scm::Exceptions::RepositoryBuildError => e
     @build_failed_msg = e.message
     nil
   end

app/views/repositories/destroy_info.html.erb

@@ -28,30 +28,32 @@ See doc/COPYRIGHT.rdoc for more details.
 ++#%>
 <%= toolbar title: l(:label_confirmation) %>
 <div class="notification-box <%= @repository.managed? ? '-error' : '-warning' %>">
-  <p><strong><%= l('repositories.destroy.title') %></strong><br /></p>
-  <p>
-    <% if @repository.managed? %>
-      <%= l('repositories.destroy.managed_text', project_name: @project.identifier) %>
-      <br/>
-      <%= l('repositories.destroy.managed_path_note', path: @repository.root_url) %>
-    <% else %>
-      <%= simple_format l('repositories.destroy.linked_text',
-                        project_name: @project.identifier, url: @repository.url) %>
-    <% end %>
-  </p>
-  <p>
-    <%= link_to project_repository_path(@project),
-      :confirm => l(:text_are_you_sure),
-      :method => :delete,
-      :class => 'button -highlight' do %>
-      <i class="button--icon icon-delete"></i>
-      <span class="button--text"><%= l(:button_delete) %></span>
-    <% end %>
-    <%= link_to @back_link,
-      :class => 'button' do %>
-      <i class="button--icon icon-close"></i>
-      <span class="button--text"><%= l(:button_cancel) %></span>
-    <% end %>
-  </p>
+  <div class="notification-box--content">
+    <p><strong><%= l('repositories.destroy.title') %></strong><br /></p>
+    <p>
+      <% if @repository.managed? %>
+        <%= l('repositories.destroy.managed_text', project_name: @project.identifier) %>
+        <br/>
+        <%= l('repositories.destroy.managed_path_note', path: @repository.root_url) %>
+      <% else %>
+        <%= simple_format l('repositories.destroy.linked_text',
+                          project_name: @project.identifier, url: @repository.url) %>
+      <% end %>
+    </p>
+    <p>
+      <%= link_to project_repository_path(@project),
+        :confirm => l(:text_are_you_sure),
+        :method => :delete,
+        :class => 'button -highlight' do %>
+        <i class="button--icon icon-delete"></i>
+        <span class="button--text"><%= l(:button_delete) %></span>
+      <% end %>
+      <%= link_to @back_link,
+        :class => 'button' do %>
+        <i class="button--icon icon-close"></i>
+        <span class="button--text"><%= l(:button_cancel) %></span>
+      <% end %>
+    </p>
+  </div>
 </div>
 

lib/open_project/scm/adapters/base.rb

@@ -33,19 +33,17 @@ module OpenProject
   module Scm
     module Adapters
       class Base
+        attr_accessor :url, :root_url
+
         def initialize(url, root_url = nil)
-          @url = url
-          @root_url = root_url
+          self.url = url
+          self.root_url = root_url
         end
 
         def local?
           false
         end
 
-        def remote?
-          !local?
-        end
-
         def available?
           check_availability!
           true
@@ -62,14 +60,6 @@ module OpenProject
           self.class.name.demodulize
         end
 
-        def root_url
-          @root_url
-        end
-
-        def url
-          @url
-        end
-
         def info
           nil
         end
@@ -148,11 +138,6 @@ module OpenProject
           path ||= ''
           (path[-1, 1] == '/') ? path[0..-2] : path
         end
-
-        def retrieve_root_url
-          info = self.info
-          info ? info.root_url : nil
-        end
       end
 
       class Info

lib/open_project/scm/exceptions.rb

@@ -33,6 +33,10 @@ module OpenProject
       class ScmError < StandardError
       end
 
+      # Exception marking an error in the repository build process
+      class RepositoryBuildError < ScmError
+      end
+
       # Exception marking an error in the execution of a local command.
       class CommandFailed < ScmError
         attr_reader :program

spec/app/services/scm/create_managed_repository_service_spec.rb

@@ -0,0 +1,111 @@
+#-- encoding: UTF-8
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+
+require 'spec_helper'
+
+describe Scm::CreateManagedRepositoryService do
+  let(:user) { FactoryGirl.build(:user) }
+  let(:project) { FactoryGirl.build(:project) }
+
+  let(:repository) { FactoryGirl.build(:repository_subversion) }
+  subject(:service) { Scm::CreateManagedRepositoryService.new(repository) }
+
+  let(:config)   { {} }
+
+  before do
+    allow(OpenProject::Configuration).to receive(:[]).and_call_original
+    allow(OpenProject::Configuration).to receive(:[]).with('scm').and_return(config)
+  end
+
+  shared_examples 'does not create a filesystem repository' do
+    it 'does not create a filesystem repository' do
+      expect(repository.managed?).to be false
+      expect(service.call).to be false
+    end
+  end
+
+  context 'with no managed configuration' do
+    it_behaves_like 'does not create a filesystem repository'
+  end
+
+  context 'with managed repository' do
+
+    # Must not .create a managed repository, or it will call this service itself!
+    let(:repository) {
+      repo = Repository::Subversion.new(scm_type: :managed)
+      repo.project = project
+      repo
+    }
+
+    context 'but no managed config' do
+      it 'does not create a filesystem repository' do
+        expect(repository.managed?).to be true
+        expect(service.call).to be false
+      end
+    end
+  end
+
+  context 'with managed config' do
+    include_context 'with tmpdir'
+    let(:config) {
+      {
+        Subversion: { manages: File.join(tmpdir, 'svn') },
+        Git:        { manages: File.join(tmpdir, 'git') }
+      }
+    }
+
+    let(:repository) {
+      repo = Repository::Subversion.new(scm_type: :managed)
+      repo.project = project
+      repo.configure(:managed, nil)
+      repo
+    }
+
+    before do
+      allow_any_instance_of(Scm::CreateRepositoryJob)
+        .to receive(:repository).and_return(repository)
+    end
+
+    it 'creates the repository' do
+      expect(service.call).to be true
+      expect(File.directory?(repository.managed_repository_path)).to be true
+    end
+
+    context 'with pre-existing path on filesystem' do
+      before do
+        allow(File).to receive(:directory?).and_return(true)
+      end
+
+      it 'does not create the repository' do
+        expect(service.call).to be false
+        expect(service.localized_rejected_reason)
+          .to eq(I18n.t('repositories.errors.exists_on_filesystem'))
+      end
+    end
+  end
+end

spec/app/services/scm/delete_managed_repository_service_spec.rb

@@ -0,0 +1,121 @@
+#-- encoding: UTF-8
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+
+require 'spec_helper'
+
+describe Scm::DeleteManagedRepositoryService do
+  let(:user) { FactoryGirl.build(:user) }
+  let(:project) { FactoryGirl.build(:project) }
+
+  let(:repository) { FactoryGirl.build(:repository_subversion) }
+  subject(:service) { Scm::DeleteManagedRepositoryService.new(repository) }
+
+  let(:config)   { {} }
+
+  before do
+    allow(OpenProject::Configuration).to receive(:[]).and_call_original
+    allow(OpenProject::Configuration).to receive(:[]).with('scm').and_return(config)
+  end
+
+  shared_examples 'does not delete the repository' do
+    it 'does not delete the repository' do
+      expect(repository.managed?).to be false
+      expect(service.call).to be false
+    end
+  end
+
+  context 'with no managed configuration' do
+    it_behaves_like 'does not delete the repository'
+  end
+
+  context 'with managed repository, but no config' do
+    let(:repository) { FactoryGirl.build(:repository_subversion, scm_type: :managed) }
+
+    it 'does not delete the repository' do
+      expect(repository.managed?).to be true
+      expect(service.call).to be false
+    end
+  end
+
+  context 'with managed repository and managed config' do
+    include_context 'with tmpdir'
+    let(:config) {
+      {
+        Subversion: { manages: File.join(tmpdir, 'svn') },
+        Git:        { manages: File.join(tmpdir, 'git') }
+      }
+    }
+
+    # Must not .create a managed repository, or it will call this service itself!
+    let(:repository) {
+      repo = Repository::Subversion.new(scm_type: :managed)
+      repo.project = project
+      repo.configure(:managed, nil)
+
+      repo.save!
+      repo
+    }
+
+    before do
+      allow_any_instance_of(Scm::CreateRepositoryJob)
+        .to receive(:repository).and_return(repository)
+    end
+
+    it 'deletes the repository' do
+      expect(File.directory?(repository.managed_repository_path)).to be true
+      expect(service.call).to be true
+      expect(File.directory?(repository.managed_repository_path)).to be false
+    end
+
+    context 'and parent project' do
+      let(:parent) { FactoryGirl.create(:project) }
+      let(:project) { FactoryGirl.create(:project, parent: parent) }
+      let(:repo_path) {
+        Pathname.new(File.join(tmpdir, 'svn', parent.identifier, "#{project.identifier}.svn"))
+      }
+
+      it 'deletes the parent path when empty' do
+        expect(service.call).to be true
+        path = Pathname.new(repository.managed_repository_path)
+        expect(path).to eq(repo_path)
+
+        expect(path.exist?).to be false
+        expect(path.parent.exist?).to be false
+      end
+
+      it 'keeps the parent path when not empty' do
+        other_repo = repo_path.parent + 'foobar'
+        other_repo.mkdir
+
+        expect(service.call).to be true
+        expect(repo_path.exist?).to be false
+        expect(repo_path.parent.exist?).to be true
+      end
+    end
+  end
+end

spec/app/services/scm/repository_factory_service_spec.rb

@@ -0,0 +1,133 @@
+#-- encoding: UTF-8
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+
+require 'spec_helper'
+
+describe Scm::RepositoryFactoryService do
+  let(:user) { FactoryGirl.build(:user) }
+  let(:project) { FactoryGirl.build(:project) }
+
+  let(:enabled_scms) { ['Subversion', 'Git'] }
+
+  let(:params_hash) { {} }
+  let(:params) { ActionController::Parameters.new params_hash }
+
+  subject(:service) { Scm::RepositoryFactoryService.new(project, params) }
+
+  before do
+    allow(Setting).to receive(:enabled_scm).and_return(enabled_scms)
+  end
+
+  context 'with empty hash' do
+    it 'should not build a repository' do
+      expect(service.build_temporary).not_to be true
+      expect(service.repository).to be_nil
+    end
+  end
+
+  context 'with valid vendor' do
+    let(:params_hash) {
+      { scm_vendor: 'Subversion' }
+    }
+
+    it 'should allow temporary build repository' do
+      expect(service.build_temporary).to be true
+      expect(service.repository).not_to be_nil
+    end
+
+    it 'should not allow to persist a repository' do
+      expect { service.build_and_save }
+        .to raise_error(ActionController::ParameterMissing)
+
+      expect(service.repository).to be_nil
+    end
+  end
+
+  context 'with invalid vendor' do
+    let(:params_hash) {
+      { scm_vendor: 'NotSubversion', scm_type: 'foo' }
+    }
+
+    it 'should not allow to temporary build repository' do
+      expect { service.build_temporary }.not_to raise_error
+      
+      expect(service.repository).to be_nil
+      expect(service.build_error).to include('The SCM vendor NotSubversion is disabled')
+    end
+
+    it 'should not allow to persist a repository' do
+      expect { service.build_temporary }.not_to raise_error
+
+      expect(service.repository).to be_nil
+      expect(service.build_error).to include('The SCM vendor NotSubversion is disabled')
+    end
+  end
+
+  context 'with vendor and type' do
+    let(:params_hash) {
+      { scm_vendor: 'Subversion', scm_type: 'existing' }
+    }
+
+    it 'should not allow to persist a repository without URL' do
+      expect(service.build_and_save).not_to be true
+
+      expect(service.repository).to be_nil
+      expect(service.build_error).to include("URL can't be blank")
+    end
+  end
+
+  context 'with invalid hash' do
+    let(:params_hash) {
+      {
+        scm_vendor: 'Subversion', scm_type: 'existing',
+        repository: { url: '/tmp/foo.svn' }
+      }
+    }
+
+    it 'should not allow to persist a repository URL' do
+      expect(service.build_and_save).not_to be true
+
+      expect(service.repository).to be_nil
+      expect(service.build_error).to include("URL is invalid")
+    end
+  end
+
+  context 'with valid hash' do
+    let(:params_hash) {
+      {
+        scm_vendor: 'Subversion', scm_type: 'existing',
+        repository: { url: 'file:///tmp/foo.svn' }
+      }
+    }
+
+    it 'should allow to persist a repository without URL' do
+      expect(service.build_and_save).to be true
+      expect(service.repository).to be_kind_of(Repository::Subversion)
+    end
+  end
+end

spec/controllers/repositories_controller_spec.rb

@@ -84,6 +84,17 @@ describe RepositoriesController, type: :controller do
       it_behaves_like 'successful settings response'
     end
 
+    context 'with #destroy' do
+      before do
+        allow(repository).to receive(:destroy).and_return(true)
+        xhr :delete, :destroy
+      end
+
+      it 'redirects to settings' do
+        expect(response).to redirect_to(settings_project_path(id: project.id, tab: 'repository'))
+      end
+    end
+
     context 'with #update' do
       before do
         xhr :put, :update
@@ -94,12 +105,13 @@ describe RepositoriesController, type: :controller do
 
     context 'with #create' do
       before do
-        xhr :post, :create, scm_vendor: 'Subversion', scm_type: 'local', url: 'file:///tmp/repo.svn/'
+        xhr :post, :create, scm_vendor: 'Subversion',
+            scm_type: 'local', url: 'file:///tmp/repo.svn/'
       end
 
       it 'renders a JS redirect' do
-        expect(response.body)
-          .to match(/window\.location = '\/projects\/(#{project.identifier}|#{project.id})\/settings\/repository'/)
+        path = "\/projects\/(#{project.identifier}|#{project.id})\/settings\/repository"
+        expect(response.body).to match(/window\.location = '#{path}'/)
       end
     end
   end

spec/features/repositories/create_repository_spec.rb

@@ -39,7 +39,8 @@ describe 'Create repository', type: :feature, js: true do
   let(:enabled_scms) { %w[Subversion Git] }
   let(:config) { nil }
 
-  let(:scm_vendor_input) { find('select[name="scm_vendor"]') }
+  let(:scm_vendor_input_css) { 'select[name="scm_vendor"]' }
+  let(:scm_vendor_input) { find(scm_vendor_input_css) }
 
   before do
     allow(User).to receive(:current).and_return current_user
@@ -76,19 +77,26 @@ describe 'Create repository', type: :feature, js: true do
   end
 
   describe 'with submitted vendor form' do
-    let(:scm_types) { page.all('input[name="scm_type"]') }
     before do
       settings_page.visit_repository_settings
-      scm_vendor_input.find('option', text: vendor).select_option
+      find("option[value='#{vendor}']").select_option
     end
 
-    shared_examples "displays only the type" do |type|
+    shared_examples 'displays only the type' do |type|
       it 'should display one type, but expanded' do
-        expect(scm_vendor_input.value).to eq(vendor)
-        expect(scm_types.length).to eq(1)
-        expect(scm_types[0].value).to eq(type)
-        expect(scm_types[0][:selected]).to be_truthy
-        expect(scm_types[0][:disabled]).to be_falsey
+        # There seems to be an issue with how the
+        # select is accessed after the async form loading
+        # Thus we explitly find it here to allow some wait
+        # even though it is available in let
+        scm_vendor = find(scm_vendor_input_css)
+        expect(scm_vendor.value).to eq(vendor)
+
+        page.assert_selector('input[name="scm_type"]', :count => 1)
+        scm_type = find('input[name="scm_type"]')
+
+        expect(scm_type.value).to eq(type)
+        expect(scm_type[:selected]).to be_truthy
+        expect(scm_type[:disabled]).to be_falsey
 
         content = find("#toggleable-attribute-group--content-#{type}")
         expect(content).not_to be_nil
@@ -121,7 +129,7 @@ describe 'Create repository', type: :feature, js: true do
         expect(content[:hidden]).to be_falsey
 
         find('input[type="radio"][value="managed"]').set(true)
-        content = find("#toggleable-attribute-group--content-managed")
+        content = find('#toggleable-attribute-group--content-managed')
         expect(content).not_to be_nil
         expect(content[:hidden]).to be_falsey
       end
@@ -155,16 +163,15 @@ describe 'Create repository', type: :feature, js: true do
       it_behaves_like 'displays only the type', 'existing'
 
       context 'and managed repositories' do
-        Dir.mktmpdir do |dir|
-          let(:config) {
-            { Subversion: { manages: dir } }
-          }
-          it_behaves_like 'has managed and other type', 'existing'
-          it_behaves_like 'it can create the managed repository'
-          it_behaves_like 'it can create the repository of type with url',
-                          'existing',
-                          'file:///tmp/svn/foo.svn'
-        end
+        include_context 'with tmpdir'
+        let(:config) {
+          { Subversion: { manages: tmpdir } }
+        }
+        it_behaves_like 'has managed and other type', 'existing'
+        it_behaves_like 'it can create the managed repository'
+        it_behaves_like 'it can create the repository of type with url',
+                        'existing',
+                        'file:///tmp/svn/foo.svn'
       end
     end
 
@@ -180,17 +187,16 @@ describe 'Create repository', type: :feature, js: true do
       end
 
       context 'and managed repositories' do
-        Dir.mktmpdir do |dir|
-          let(:config) {
-            { Git: { manages: dir } }
-          }
+        include_context 'with tmpdir'
+        let(:config) {
+          { Git: { manages: tmpdir } }
+        }
 
-          it_behaves_like 'has managed and other type', 'local'
-          it_behaves_like 'it can create the managed repository'
-          it_behaves_like 'it can create the repository of type with url',
-                          'local',
-                          '/tmp/git/foo.git'
-        end
+        it_behaves_like 'has managed and other type', 'local'
+        it_behaves_like 'it can create the managed repository'
+        it_behaves_like 'it can create the repository of type with url',
+                        'local',
+                        '/tmp/git/foo.git'
       end
     end
   end

spec/features/repositories/repository_settings_spec.rb

@@ -29,7 +29,7 @@
 require 'spec_helper'
 require 'features/repositories/repository_settings_page'
 
-describe 'Manage repository', type: :feature, js: true do
+describe 'Repository Settings', type: :feature, js: true do
   let(:current_user) { FactoryGirl.create (:admin) }
   let(:project) { FactoryGirl.create(:project) }
   let(:settings_page) { RepositorySettingsPage.new(project) }
@@ -62,10 +62,11 @@ describe 'Manage repository', type: :feature, js: true do
     end
 
     it 'deletes the repository' do
+      expect(Repository.exists?(repository)).to be true
       find('a.icon-delete', text: I18n.t(:button_delete)).click
 
       # Confirm the notification warning
-      warning = type == 'managed' ? '-error' : '-warning'
+      warning = (type == 'managed') ? '-error' : '-warning'
       expect(page).to have_selector(".notification-box.#{warning}")
       find('a', text: I18n.t(:button_delete)).click
 
@@ -79,6 +80,9 @@ describe 'Manage repository', type: :feature, js: true do
       expect(selected.text).to eq('--- Please select ---')
       expect(selected[:disabled]).to be_truthy
       expect(selected[:selected]).to be_truthy
+
+      # Project should have no repository
+      expect(Repository.exists?(repository)).to be false
     end
   end
 
@@ -95,37 +99,35 @@ describe 'Manage repository', type: :feature, js: true do
   it_behaves_like 'manages the repository with', 'Git', 'local'
 
   context 'managed repositories' do
-    Dir.mktmpdir do |dir|
-      let(:config) {
-        {
-          Subversion: { manages: File.join(dir, 'svn') },
-          Git:        { manages: File.join(dir, 'git') }
-        }
+    include_context 'with tmpdir'
+    let(:config) {
+      {
+        Subversion: { manages: File.join(tmpdir, 'svn') },
+        Git:        { manages: File.join(tmpdir, 'git') }
       }
+    }
 
-      let(:repository) {
-        repo = Repository.build(
-          project,
-          managed_vendor,
-          # Need to pass AC params here manually to simulate a regular repository build
-          ActionController::Parameters.new({}),
-          :managed
-        )
+    let(:repository) {
+      repo = Repository.build(
+        project,
+        managed_vendor,
+        # Need to pass AC params here manually to simulate a regular repository build
+        ActionController::Parameters.new({}),
+        :managed
+      )
 
-        repo.save!
-        repo
-      }
+      repo.save!
+      repo
+    }
 
-      context 'Subversion' do
-        let(:managed_vendor) { 'Subversion' }
-        it_behaves_like 'manages the repository', 'Subversion', 'managed'
-      end
+    context 'Subversion' do
+      let(:managed_vendor) { 'Subversion' }
+      it_behaves_like 'manages the repository', 'Subversion', 'managed'
+    end
 
-      context 'Git' do
-        let(:managed_vendor) { 'Git' }
-        it_behaves_like 'manages the repository', 'Git', 'managed'
-      end
+    context 'Git' do
+      let(:managed_vendor) { 'Git' }
+      it_behaves_like 'manages the repository', 'Git', 'managed'
     end
   end
-
 end

spec/lib/open_project/scm/adapters/git_adapter_spec.rb

@@ -66,6 +66,8 @@ describe OpenProject::Scm::Adapters::Git do
           .and_return(git_string)
 
         expect(adapter.client_version).to eq(expected_version)
+        expect(adapter.client_available).to be true
+        expect(adapter.client_version_string).to eq(expected_version.join('.'))
       end
     end
 
@@ -94,24 +96,23 @@ describe OpenProject::Scm::Adapters::Git do
   end
 
   describe 'empty repository' do
-    Dir.mktmpdir do |dir|
-      let(:url) { dir }
+    include_context 'with tmpdir'
+    let(:url) { tmpdir }
 
-      before do
-        adapter.initialize_bare_git
-      end
+    before do
+      adapter.initialize_bare_git
+    end
 
-      describe '.check_availability!' do
-        it 'should be marked empty' do
-          expect { adapter.check_availability! }
-            .to raise_error(OpenProject::Scm::Exceptions::ScmEmpty)
-        end
+    describe '.check_availability!' do
+      it 'should be marked empty' do
+        expect { adapter.check_availability! }
+          .to raise_error(OpenProject::Scm::Exceptions::ScmEmpty)
       end
     end
   end
 
   describe 'local repository' do
-    with_filesystem_repository('git', 'git') do |repo_dir|
+    with_git_repository do |repo_dir|
       let(:url) { repo_dir }
 
       it 'reads the git version' do
@@ -290,10 +291,6 @@ describe OpenProject::Scm::Adapters::Git do
             expect(rev.author).to eq('Felix Schäfer <felix@fachschaften.org>')
           end
 
-          it 'retrieves associated revisions' do
-            entries = adapter.entries('', '83ca5fd')
-          end
-
           it 'can be retrieved by tag' do
             entries = adapter.entries(nil, 'tag01.annotated')
             expect(entries.length).to eq(3)

spec/lib/open_project/scm/adapters/subversion_adapter_spec.rb

@@ -30,9 +30,10 @@
 require 'spec_helper'
 
 describe OpenProject::Scm::Adapters::Subversion do
-  let(:url) { '/tmp/bar.svn' }
+  let(:root_url) { '/tmp/bar.svn' }
+  let(:url) { "file://#{root_url}" }
   let(:config) { {} }
-  let(:adapter) { OpenProject::Scm::Adapters::Subversion.new url }
+  let(:adapter) { OpenProject::Scm::Adapters::Subversion.new url, root_url }
 
   before do
     allow(adapter).to receive(:config).and_return(config)
@@ -57,6 +58,8 @@ describe OpenProject::Scm::Adapters::Subversion do
           .and_return(svn_string)
 
         expect(adapter.client_version).to eq(expected_version)
+        expect(adapter.client_available).to be true
+        expect(adapter.client_version_string).to eq(expected_version.join('.'))
       end
     end
 
@@ -66,9 +69,71 @@ describe OpenProject::Scm::Adapters::Subversion do
     it_behaves_like 'correct client version', "1.6.2\r\n1.8.1\r\n1.9.1", [1, 6, 2]
   end
 
+  describe 'invalid repository' do
+    describe '.check_availability!' do
+      it 'should not be available' do
+        expect(Dir.exists?(url)).to be false
+        expect(adapter).not_to be_available
+        expect { adapter.check_availability! }
+          .to raise_error(OpenProject::Scm::Exceptions::ScmUnavailable)
+      end
+
+      it 'should raise a meaningful error if shell output fails' do
+        error_string = <<-ERR.strip_heredoc
+          svn: E215004: Authentication failed and interactive prompting is disabled; see the --force-interactive option
+          svn: E215004: Unable to connect to a repository at URL 'file:///tmp/bar.svn'
+          svn: E215004: No more credentials or we tried too many times.
+          Authentication failed
+        ERR
+
+        allow(adapter).to receive(:popen3)
+          .and_yield(StringIO.new(''), StringIO.new(error_string))
+
+        expect { adapter.check_availability! }
+          .to raise_error(OpenProject::Scm::Exceptions::ScmUnauthorized)
+      end
+    end
+  end
+
+  describe 'empty repository' do
+    include_context 'with tmpdir'
+    let(:root_url) { tmpdir }
+
+    describe '.create_empty_svn' do
+      context 'with valid root_url' do
+        it 'should create the repository' do
+          expect(Dir.exists?(root_url)).to be true
+          expect(Dir.entries(root_url).length).to eq 2
+          expect { adapter.create_empty_svn }.not_to raise_error
+
+          expect(Dir.exists?(root_url)).to be true
+          expect(Dir.entries(root_url).length).to be >= 5
+        end
+      end
+      context 'with non-existing root_url' do
+        let(:root_url) { File.join(tmpdir, 'foo', 'bar') }
+
+        it 'should fail' do
+          expect { adapter.create_empty_svn }
+            .to raise_error(OpenProject::Scm::Exceptions::CommandFailed)
+
+          expect(Dir.exists?(root_url)).to be false
+        end
+      end
+    end
+
+    describe '.check_availability!' do
+      it 'should be marked empty' do
+        adapter.create_empty_svn
+        expect { adapter.check_availability! }
+          .to raise_error(OpenProject::Scm::Exceptions::ScmEmpty)
+      end
+    end
+  end
+
   describe 'local repository' do
-    with_filesystem_repository('subversion', 'svn') do |repo_dir|
-      let(:url) { "file://#{repo_dir}" }
+    with_subversion_repository do |repo_dir|
+      let(:root_url) { repo_dir }
 
       it 'reads the Subversion version' do
         expect(adapter.client_version.length).to be >= 3
@@ -202,6 +267,7 @@ describe OpenProject::Scm::Adapters::Subversion do
 
           expect(revisions.length).to eq(1)
           expect(revisions[0].identifier).to eq('11')
+          expect(revisions[0].format_identifier).to eq('11')
 
           paths = revisions[0].paths
           expect(paths.length).to eq(2)

spec/models/changeset_spec.rb

@@ -31,7 +31,7 @@ require 'spec_helper'
 describe Changeset, type: :model do
   let(:email) { 'bob@bobbit.org' }
 
-  with_created_subversion_repository do
+  with_virtual_subversion_repository do
     let(:changeset) {
       FactoryGirl.build(:changeset,
                         repository: repository,

spec/models/repository/git_spec.rb

@@ -85,7 +85,7 @@ describe Repository::Git, type: :model do
   end
 
   describe 'with an actual repository' do
-    with_filesystem_repository('git', 'git') do |repo_dir|
+    with_git_repository do |repo_dir|
       let(:url)      { repo_dir }
       let(:instance) { FactoryGirl.create(:repository_git, path_encoding: encoding, url: url) }
 

spec/models/repository/subversion_spec.rb

@@ -98,7 +98,7 @@ describe Repository::Subversion, type: :model do
   end
 
   describe 'with an actual repository' do
-    with_filesystem_repository('subversion', 'svn') do |repo_dir|
+    with_subversion_repository do |repo_dir|
       let(:url)      { "file://#{repo_dir}" }
       let(:instance) { FactoryGirl.create(:repository_subversion, url: url) }
 

spec/models/user_deletion_spec.rb

@@ -397,7 +397,7 @@ describe User, 'deletion', type: :model do
   end
 
   describe 'WHEN the user has created a changeset' do
-    with_created_subversion_repository do
+    with_virtual_subversion_repository do
       let(:associated_instance) do
         FactoryGirl.build(:changeset,
                           repository_id: repository.id,
@@ -412,7 +412,7 @@ describe User, 'deletion', type: :model do
   end
 
   describe 'WHEN the user has updated a changeset' do
-    with_created_subversion_repository do
+    with_virtual_subversion_repository do
       let(:associated_instance) do
         FactoryGirl.build(:changeset,
                           repository_id: repository.id,

spec/support/repository_helpers.rb

@@ -26,6 +26,14 @@
 #
 # See doc/COPYRIGHT.rdoc for more details.
 #++
+
+##
+# Create a temporary +vendor+ repository from the stored fixture.
+# Automatically extracts and destroys said repository,
+# however does not provide single example isolation
+# due to performance.
+# As we do not write to the repository, we don't need this kind
+# of isolation.
 def with_filesystem_repository(vendor, command = nil, &block)
   repo_dir = File.join(Rails.root, 'tmp', 'test', "#{vendor}_repository")
   fixture = File.join(Rails.root, "spec/fixtures/repositories/#{vendor}_repository.tar.gz")
@@ -52,7 +60,21 @@ def with_filesystem_repository(vendor, command = nil, &block)
   block.call(repo_dir)
 end
 
-def with_created_subversion_repository(&block)
+def with_subversion_repository(&block)
+  with_filesystem_repository('subversion', 'svn', &block)
+end
+
+def with_git_repository(&block)
+  with_filesystem_repository('git', 'git', &block)
+end
+
+##
+# Many specs required any repository to be available,
+# often Filesystem adapter was used, even though
+# no actual filesystem access occured.
+# Instead, we wrap these repository specs in a virtual
+# subversion repository which does not exist on disk.
+def with_virtual_subversion_repository(&block)
   let(:repository) { FactoryGirl.create(:repository_subversion) }
 
   before do

spec/support/tempdir.rb

@@ -0,0 +1,37 @@
+#-- encoding: UTF-8
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+shared_context 'with tmpdir' do
+  around do |example|
+    Dir.mktmpdir do |dir|
+      @tmpdir = dir
+      example.run
+    end
+  end
+  attr_reader :tmpdir
+end