Daily Refactor #21: Move methods in the RoutingTest

I tried to start this week’s refactorings with some major changes to one of the worst areas of Redmine, the IssuesController. Weighing in at over 576 lines and 20 actions, it is full of really bad smells and hasn’t getting any better over time. But after spending two hours on it, I was getting nowhere and decided to start over. So instead of going for the big impact changes, I’m going to tackle the little changes one by one.

The Refactoring

Today’s refactoring involves moving some of the IssuesController’s routing tests from the functional tests into the integration tests.

Before

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141

# test/functional/issues_controller_test.rb
class IssuesControllerTest  :get, :path => '/issues'},
      :controller => 'issues', :action => 'index'
    )
  end
 
  # ...
 
  def test_index_with_project_routing
    assert_routing(
      {:method => :get, :path => '/projects/23/issues'},
      :controller => 'issues', :action => 'index', :project_id => '23'
    )
  end
 
  # ...
 
  def test_index_with_project_routing
    assert_routing(
      {:method => :get, :path => 'projects/23/issues'},
      :controller => 'issues', :action => 'index', :project_id => '23'
    )
  end
 
  # ...
 
  def test_index_with_project_routing_formatted
    assert_routing(
      {:method => :get, :path => 'projects/23/issues.pdf'},
      :controller => 'issues', :action => 'index', :project_id => '23', :format => 'pdf'
    )
    assert_routing(
      {:method => :get, :path => 'projects/23/issues.atom'},
      :controller => 'issues', :action => 'index', :project_id => '23', :format => 'atom'
    )
  end
 
  # ...
 
  def test_index_formatted
    assert_routing(
      {:method => :get, :path => 'issues.pdf'},
      :controller => 'issues', :action => 'index', :format => 'pdf'
    )
    assert_routing(
      {:method => :get, :path => 'issues.atom'},
      :controller => 'issues', :action => 'index', :format => 'atom'
    )
  end
 
  # ...
 
  def test_show_routing
    assert_routing(
      {:method => :get, :path => '/issues/64'},
      :controller => 'issues', :action => 'show', :id => '64'
    )
  end
 
  def test_show_routing_formatted
    assert_routing(
      {:method => :get, :path => '/issues/2332.pdf'},
      :controller => 'issues', :action => 'show', :id => '2332', :format => 'pdf'
    )
    assert_routing(
      {:method => :get, :path => '/issues/23123.atom'},
      :controller => 'issues', :action => 'show', :id => '23123', :format => 'atom'
    )
  end
 
  # ...
 
  def test_new_routing
    assert_routing(
      {:method => :get, :path => '/projects/1/issues/new'},
      :controller => 'issues', :action => 'new', :project_id => '1'
    )
    assert_recognizes(
      {:controller => 'issues', :action => 'new', :project_id => '1'},
      {:method => :post, :path => '/projects/1/issues'}
    )
  end
 
  # ...
 
  def test_copy_routing
    assert_routing(
      {:method => :get, :path => '/projects/world_domination/issues/567/copy'},
      :controller => 'issues', :action => 'new', :project_id => 'world_domination', :copy_from => '567'
    )
  end
 
  # ...
 
  def test_edit_routing
    assert_routing(
      {:method => :get, :path => '/issues/1/edit'},
      :controller => 'issues', :action => 'edit', :id => '1'
    )
    assert_recognizes( #TODO: use a PUT on the issue URI isntead, need to adjust form
      {:controller => 'issues', :action => 'edit', :id => '1'},
      {:method => :post, :path => '/issues/1/edit'}
    )
  end
 
  # ...
 
  def test_reply_routing
    assert_routing(
      {:method => :post, :path => '/issues/1/quoted'},
      :controller => 'issues', :action => 'reply', :id => '1'
    )
  end
 
  # ...
 
  def test_move_routing
    assert_routing(
      {:method => :get, :path => '/issues/1/move'},
      :controller => 'issues', :action => 'move', :id => '1'
    )
    assert_recognizes(
      {:controller => 'issues', :action => 'move', :id => '1'},
      {:method => :post, :path => '/issues/1/move'}
    )
  end
 
  # ...
 
  def test_destroy_routing
    assert_recognizes( #TODO: use DELETE on issue URI (need to change forms)
      {:controller => 'issues', :action => 'destroy', :id => '1'},
      {:method => :post, :path => '/issues/1/destroy'}
    )
  end
 
end
 
# test/integration/routing_test.rb
class RoutingTest < ActionController::IntegrationTest
end

After

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48

# test/functional/issues_controller_test.rb
class IssuesControllerTest < ActionController::TestCase
  # ...
end
 
# test/integration/routing_test.rb
class RoutingTest  'issues', :action => 'index'
    should_route :get, "/issues.pdf", :controller => 'issues', :action => 'index', :format => 'pdf'
    should_route :get, "/issues.atom", :controller => 'issues', :action => 'index', :format => 'atom'
    should_route :get, "/issues.xml", :controller => 'issues', :action => 'index', :format => 'xml'
    should_route :get, "/projects/23/issues", :controller => 'issues', :action => 'index', :project_id => '23'
    should_route :get, "/projects/23/issues.pdf", :controller => 'issues', :action => 'index', :project_id => '23', :format => 'pdf'
    should_route :get, "/projects/23/issues.atom", :controller => 'issues', :action => 'index', :project_id => '23', :format => 'atom'
    should_route :get, "/projects/23/issues.xml", :controller => 'issues', :action => 'index', :project_id => '23', :format => 'xml'
    should_route :get, "/issues/64", :controller => 'issues', :action => 'show', :id => '64'
    should_route :get, "/issues/64.pdf", :controller => 'issues', :action => 'show', :id => '64', :format => 'pdf'
    should_route :get, "/issues/64.atom", :controller => 'issues', :action => 'show', :id => '64', :format => 'atom'
    should_route :get, "/issues/64.xml", :controller => 'issues', :action => 'show', :id => '64', :format => 'xml'
 
    should_route :get, "/projects/23/issues/new", :controller => 'issues', :action => 'new', :project_id => '23'
 
    should_route :get, "/issues/64/edit", :controller => 'issues', :action => 'edit', :id => '64'
    # TODO: Should use PUT
    should_route :post, "/issues/64/edit", :controller => 'issues', :action => 'edit', :id => '64'
 
    # TODO: Should use DELETE
    should_route :post, "/issues/64/destroy", :controller => 'issues', :action => 'destroy', :id => '64'
 
    # Extra actions
    should_route :get, "/projects/23/issues/64/copy", :controller => 'issues', :action => 'new', :project_id => '23', :copy_from => '64'
 
    should_route :get, "/issues/1/move", :controller => 'issues', :action => 'move', :id => '1'
    should_route :post, "/issues/1/move", :controller => 'issues', :action => 'move', :id => '1'
 
    should_route :post, "/issues/1/quoted", :controller => 'issues', :action => 'reply', :id => '1'
 
    should_route :get, "/issues/calendar", :controller => 'issues', :action => 'calendar'
    should_route :post, "/issues/calendar", :controller => 'issues', :action => 'calendar'
    should_route :get, "/projects/project-name/issues/calendar", :controller => 'issues', :action => 'calendar', :project_id => 'project-name'
    should_route :post, "/projects/project-name/issues/calendar", :controller => 'issues', :action => 'calendar', :project_id => 'project-name'
 
    should_route :get, "/issues/gantt", :controller => 'issues', :action => 'gantt'
    should_route :post, "/issues/gantt", :controller => 'issues', :action => 'gantt'
    should_route :get, "/projects/project-name/issues/gantt", :controller => 'issues', :action => 'gantt', :project_id => 'project-name'
    should_route :post, "/projects/project-name/issues/gantt", :controller => 'issues', :action => 'gantt', :project_id => 'project-name'
  end
 
end

Review

It’s not really apparent here, but if you look at the diff for this refactoring you can see that all of the routing tests were interspaced in the functional test. This made it extremely difficult to find how the routes should behave for the IssuesController, short of reading the routes.rb. Now with the integration test using shoulda’s should_route it’s really easy to see how the HTTP verbs and urls map to the controllers and actions.

Reference commit