CSC/ECE 517 Spring 2014/oss E1406 st: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
 
(57 intermediate revisions by 3 users not shown)
Line 1: Line 1:
=E1406: Improve tests & investigate regex warnings for team functionality=
=E1406: Improve tests & investigate regex warnings for team functionality=
 
''Please be advised that we do not have deployment link, since our work are all back-end tests''
__TOC__
__TOC__
==Classes==
==Classes==
=== app/controllers/teams_controller.rb ===
=== app/controllers/teams_controller.rb ===
Line 11: Line 10:
* create new teams
* create new teams
* import/export teams
* import/export teams
* delete teams
* delete all teams/delete specified team
* copy existing teams from a course down to an assignment(inherit)
* update team's information


=== app/controllers/student_team_controller.rb ===
=== app/controllers/student_team_controller.rb ===
Line 21: Line 22:
* remove the advertise for their team
* remove the advertise for their team
* leave the team
* leave the team
* <del>review members</del> (removed)


==What are we supposed to do?==
== Motivation ==
* This [http://guides.rubyonrails.org/action_controller_overview.html| controller] contained 28 methods, which is way too many.  Some of them did not follow the convention for method names (snake case).  
There were no functional tests written on the two controllers to detect bugs, as well as confirming they are functional by itself and after integration.


* This controller had many extraneous methods, including those dealing with team membership which were to be moved, and setting due dates for staggered deadline assignments which were a part of different project (E810).
Our goal is to add comprehensive functional tests and integration tests to the two controllers to provide a preferable test suite for the future developers.  


* One of the major functionalities of this class is dealing with deadlines for different topics. These functionalities should be moved to other classes, namely DueDate and DueDate controller.
Also, as the classes are influenced by the changes from other, we are also responsible for fixing some of the bugs, refactoring the method names to a better ones, as well as reporting the bugs found in the testing process.


* Code that deals with the behind-the-scenes working of signup sheets had to be moved to signup_topic model as project E803 is going to deal with it.
== Tasks ==
Overall: contribute to the code coverage, ideally having overall coverage increase 2%
===student_team_controller===
* write tests
* def advertise_for_partners  # change to “advertise”
* def remove # change to “remove_advertisment”
* def review # remove this method


* Also, methods of this class refer to the now-removed resubmission & re-review deadlines (“resubmission” deadlines are now submission deadlines; “re-review” deadlines have been turned into review deadlines).
===teams_controller===
* write tests


* This controller should be callable from the Topics tab on a New or Edit Assignment screen, not (just) from the popup menu for an assignment. This was dealt with, in another project and was skipped in view of avoiding redundancy.  
== Deployment ==
Please note that all of our work will be under the /test folder, which means they will not show any change on the view.  
To see the result of tests, run
rake test TEST=test/functional/student_team_controller.rb
and  
rake test TEST=test/functional/teams_controller.rb


* [http://6brand.com/rails-integration-testing-how-to-learn.html| Integration tests] need to be written.  
running the whole test suite with <code>rake test</code> is not recommended, as the schema change overtime has broke some of the previous tests.


==Testing==


Hyperlink: http://152.46.20.88:5800/
;Framework : [http://guides.rubyonrails.org/testing.html Rails default test framework]
;Sample data : Fixtures (under /test/fixtures)
;Coverage tool: [https://rubygems.org/gems/simplecov SimpleCov]
;Total Number of Tests : 37
;Previous Coverage : 22.32%
;Current Coverage : 24.08%


Git repository: https://github.com/mdjayaprakash/expertiza/tree/sign_up_sheet_controller
===Testing Scenarios===
====student_team_controller====
Most of the test case are straightforward. There are some of the test cases was commented out due to bug in the controller
* View student team (GET #view)
* Edit team (GET #edit)
* Create team with valid name (POST #create)
* Create team with name in use (POST #create)
* Update valid team name (POST #update)
* Update team name in use (POST #update)
* Update with current team name (POST #update)
* Advertise (GET #advertise)
''This is currently commented out in the committed code. It seems like the method is never called. It raises missing template error upon every call. ''
The <code>advertise</code> method was previously named advertise_for_partners, but renamed due to ambiguity as there is another class with the same name.  


==Steps to set up Expertiza==
* Remove advertisement (GET #remove_advertisement )
The <code>remove_advertisement</code> method was previously named remove, but renamed to avoid ambiguity
This method is never called as well, but it is functional as it do not require any template.
* Leave student team (GET #leave)
''This is currently commented out in the committed code, because it will raise error every time when called.''
It seems like problem happens on
  line 96: other_members = TeamsUser.where( ['team_id = ?', params[:team_id]]).first
  line 97: if other_members.length == 0
This will raise error due to the calling length of other_members, which is a single object but not an array.
Also, the related record seems to be remain undeleted in the database.


* Extract source code for Expertiza into RubyMine from the following URL: https://github.com/mdjayaprakash/expertiza/tree/sign_up_sheet_controller
====teams_controller====
Current test cases are as following.
* Create team should increase the number of teams by 1(GET #create)
* Create team should increase the number of team nodes by 1(GET #create)
* Create team with existing name (POST #create)
* Create team should redirect to list assignments (POST #create)
* Delete all teams (GET #delete_all)
* Delete all teams should redirect to list (GET #delete_all)
* List should receive assignment (GET #list)
* List should receive course (GET #list)
* New should assign parent (GET #new)
* Update team should redirect (POST #update)
* Update team should have validate name (POST #update)
* Edit team should have time (POST #edit)
* Delete team should redirect (POST #delete)
* Delete team should decrease the number of teams by 1(GET #delete)
* Delete team should decrease the number of team nodes by 1(GET #delete)
* Inherit team should redirect (POST #inherit)
* Bequeath team should redirect (POST #bequeath)
The methods in the following are ignored in testing since they are never used in current codes.
* create_teams_view
* create_teams


* Confirm the SDK for RubyMine to Ruby1.9.3 using File -> Settings.
==Sample Test Code==
We show two pieces of test code for two controllers to explain how our test works


* Run - bundle install: This should install all the gems required for Expertiza in Ruby1.9.3.
    test "create_student team with name in use" do
        sessionVars = session_for(users(:student8))
        post(:create, {'team' => { 'name' => 'IntelligentTeam2'}, 'id' => participants(:par21).id, "commit" => "Create Team"}, sessionVars, nil)
        assert_equal 'Team name is already in use.', flash[:notice]
        assert_redirected_to :controller => 'student_team', :action => 'view', :id => participants(:par21).id
    end


* Run - rake db:create:all
    test "create_should_increase_number_of_teams_course" do
        sessionVars = session_for(users(:instructor1))
        sessionVars[:team_type] = "Course"
        assert_difference 'Team.count' do
          get :create, {'id' => @testCourse,'team' => {'name' => "Random"}},sessionVars
        end
    end


* Run - rake [http://guides.rubyonrails.org/migrations.html| db:migrate ]
The first piece of code is from student_team_controller.rb. It tests if there is already a same team name when student8 want to create a team. The second piece of code is from teams_controller_test.rb. It tests if the team number decreases when instructor1 creates a new team for a course.


* From the Expertiza folder(i.e. project root), execute the command “rails server”
As we can see from the code, when we want to use <code>post</code> or <code>get</code>, several parameters are needed. Usually we use four parameters: First, the method we are requesting. Second, an hash of request parameters for the method. Third, an optional hash of session variables. Forth, an optional hash of flash values. If we do not want to pass in the optional hash, we can set it to nil as the first piece of code shows, or we can just leave it blank as the second piece of code shows.


* This should start the Expertiza server at localhost:3000 in your browser.
When we run the command <code>rake test TEST=test/functional/your_test_file.rb</code>, you will see if the test is passed or not.  


==Categories of code smells identified==
==Result==


* '''Obvious Conditionals:''' Branches that check the conditions that are pretty obvious from the context hence increase the length of the overall code and leads to inefficient code.
In this section, we will show the result form SimpleCov. The coverage is 24.08%.


* '''Meaningless Identifiers:''' The identifiers used to address some data whose meaning can’t be understood without going through the flow of code.
[[File:Result.png]]


* '''Excessive Parameters:''' Making a method call with too many parameters makes the code pretty much error prone and reduces the code readability, understandability and simplicity.
==Future Works==
 
===student_team_controller===
* '''Lengthy Definitions:''' These are the methods that have too many responsibilities and collaborators. As a result of which, their overall length has grown too long reducing the maintainability and readability.
''Please note that this bug was reported fixed in the newest version of master branch of expertiza, but the fix came too late''
 
* '''Duplicated Code:''' These are those pieces of code that does almost the same functionality of some other piece of code leading to a bad design style. They are usually too error prone since not everyone identifies all the code responsible for the same task and does the changes whenever the need arises.
 
* '''Bad method names:''' A good method name is expected to adhere to the Ruby style and design style guidelines. It is expected to convey a reasonable amount of responsibilities handled. A good method name in combination with good practices should resemble a user story.
 
* '''Used and unnecessary methods:''' These are those methods that were useful when the code was first developed. But, they lost their functionality as the code got updated and new functionalities were added or existing ones were modified.
 
* '''Bad Debug Statements:''' These statements are highly useful in development environment, but they spoil the software completeness if they’re not logged in the conventional way.
 
 
==Steps followed to eliminate code smells==
 
 
1. '''Definition renaming:'''
 
* confirmTopic() was refactored to confirm_topic()
 
This method follows the [https://en.wikipedia.org/wiki/CamelCase| camel case] which is a bad method naming style was renamed to a snake case style and its functionality was moved to waitlist controller since its functionality and association with waitlist controller is more prominent.
 
 
  [[ File:1_Screenshot.png ]]
 
 
* signup() was refactored to sign_up()
 
Even this method follows the camel case which is a bad method naming style was renamed to a snake case style just like the previous definition. The changes were done to the method heading along with all the code associated with this method like views/sign_up_sheet/_reserve_topic.html.erb and test/functional/SignUpControllerTest
 
 
  [[ File:2_Screenshot.png ]]
 
 
* teamdetails() was refactored to show_team()
 
This definition does the job of fetching and showing a team. Hence, we changed its name to a word which describes the definition’s responsibility the closest.
This change had effects in views/sign_up_sheet/_table_line.html.erb
 
 
  [[ File:3_Screenshot.png ]]
 
 
* signup_topics() was refactored to list()
 
This definition’s responsibility can be best described by the word list since it lists all the topics for an assignment. The changes were reflected in signup_sheet_controller, views/student_task/view.html.erb, config/routes.rb and signupControllerTest in test/functional.
 
 
  [[ File:4_Screenshot.png ]]
 
 
* create_default_for_microtask() was refactored to add_default_microtask()
This definition adds the default microtask as suggested by the new definition name.
 
 
  [[ File:Screenshot 5.png ]]
 
 
* create_topic_deadline() was refactored to assign_topic_deadline()
The responsibility of this definition is better described as assign_topic_deadline rather than create_topic_deadline since this method does both creating and assigning task.
 
 
  [[ File:Screenshot 6.png ]]
 
 
 
2. Several debug statements were present in various parts of the code which reflects bad programming style. Testing should have been used instead to identify the errors in code. For example, in sign_up:
 
 
  [[ File:Screenshot 7.png ]]
 
 
3. Several methods and parts of code that were associated with the signup_sheet_controller should be a part of other controller or model class. Such fragments of code were moved to the respective classes. The methods touched were:
 
 
 
* The method build_dependency_graph() was moved to sign_up_topic model
 
* The code in confirm_topic() was moved to waitlist_teams method in waitlist model.
 
* The code in create_topic_deadline was moved to DueDate.assign_topic_deadline
 
* The method set_start_due_date was moved to DueDate model
 
* The functionalities of delete_signup_for_topic was moved from sign_up_sheet_controller to reassign_topic method in sign_up_topic model
 
* The functionality of signup method from sign_up_sheet controller to signup_teams method in signup_sheet model.


There are several error raised from ''leave'' method need to be fixed, as mentioned above. Future teams may need to add tests on it as soon as it fixed, since it is a crucial functionality of the student team.


Also, methods that seems like not being used, such as <code>advertise</code> and <code>remove_advertisement</code> may need to be double checked and see if they are redundant.
===teams_controller===
The method <code>create_teams_view</code> does not be used at all. So it is safe to delete it.


4. Redundancy is not a desirable feature in [https://en.wikipedia.org/wiki/Object-oriented_programming| O-O style programming]. The code in “save_topic_deadlines” had some duplication in the conditional if statement. This was carefully removed to adhere to good programming style and to increase readability.
The method <code>create_teams</code> calls the method <code>randomize_all_by_parent</code>, which is not correct. The developer may want to fix these bugs.  


It's possible that we can write more testing cases if we improve the fixture.


  [[ File:Screenshot9.png ]]
5. Wherever possible, the conditional if statement was replaced by the ternary operator ( ? : ) and if !(condition) was replaced with unless to enhance readability. These changes were made in almost all the definition. Like the one showed below:
  [[ File:Screenshot 8.png ]]
6. Long definitions make it difficult to follow the flow of code and therefore, concise methods containing fewer lines of code are desired. In order to achieve the same, a new method “set_values_for_new_topic” was defined.
  [[ File:Screenshot 10.png ]]
7. All the references to resubmission and re-review deadlines were removed and refactored. Since this functionality has already been removed, the references are redundant and useless.
8. The following redundant methods were removed since they’re redundant and were having duplicate code: find_team_members(team_id) and has_user(user, team_id)
==Testing==
  Framework Used: Capybara<ref>http://rubygems.org/gems/capybara </ref>
  Total Number of Tests: 7
===Testing Scenarios:===
# To login successfully.
# To successfully login and create a private assignment with Topics and Available to Students options selected.
# To successfully login and create a Topic for the assignment created in the above test case.
# To login as a student who is enrolled for the same course as that of assignment created in scenario 2.
# To login as the same student in scenario 4 and reach the sign up sheet for the created assignment.
# To login as a student and successfully select the created topic under the created sample test_assignment.
# To login as a student and fail to choose another topic once a topic has already been selected.
==Future Works==
==References==
==References==
<references/>
<references/>
* http://guides.rubyonrails.org/testing.html
* https://github.com/colszowka/simplecov

Latest revision as of 19:22, 9 April 2014

E1406: Improve tests & investigate regex warnings for team functionality

Please be advised that we do not have deployment link, since our work are all back-end tests

Classes

app/controllers/teams_controller.rb

teams_controller contains functionalities for team management for instructor/TA accounts. Instructor/TA can:

  • view current teams
  • create new teams
  • import/export teams
  • delete all teams/delete specified team
  • copy existing teams from a course down to an assignment(inherit)
  • update team's information

app/controllers/student_team_controller.rb

The student_team_controller support team management for student account. student can

  • create team
  • view current team (including team invitations received and sent)
  • advertise for their team
  • remove the advertise for their team
  • leave the team

Motivation

There were no functional tests written on the two controllers to detect bugs, as well as confirming they are functional by itself and after integration.

Our goal is to add comprehensive functional tests and integration tests to the two controllers to provide a preferable test suite for the future developers.

Also, as the classes are influenced by the changes from other, we are also responsible for fixing some of the bugs, refactoring the method names to a better ones, as well as reporting the bugs found in the testing process.

Tasks

Overall: contribute to the code coverage, ideally having overall coverage increase 2%

student_team_controller

  • write tests
  • def advertise_for_partners # change to “advertise”
  • def remove # change to “remove_advertisment”
  • def review # remove this method

teams_controller

  • write tests

Deployment

Please note that all of our work will be under the /test folder, which means they will not show any change on the view. To see the result of tests, run

rake test TEST=test/functional/student_team_controller.rb

and

rake test TEST=test/functional/teams_controller.rb

running the whole test suite with rake test is not recommended, as the schema change overtime has broke some of the previous tests.

Testing

Framework
Rails default test framework
Sample data
Fixtures (under /test/fixtures)
Coverage tool
SimpleCov
Total Number of Tests
37
Previous Coverage
22.32%
Current Coverage
24.08%

Testing Scenarios

student_team_controller

Most of the test case are straightforward. There are some of the test cases was commented out due to bug in the controller

  • View student team (GET #view)
  • Edit team (GET #edit)
  • Create team with valid name (POST #create)
  • Create team with name in use (POST #create)
  • Update valid team name (POST #update)
  • Update team name in use (POST #update)
  • Update with current team name (POST #update)
  • Advertise (GET #advertise)

This is currently commented out in the committed code. It seems like the method is never called. It raises missing template error upon every call. The advertise method was previously named advertise_for_partners, but renamed due to ambiguity as there is another class with the same name.

  • Remove advertisement (GET #remove_advertisement )

The remove_advertisement method was previously named remove, but renamed to avoid ambiguity This method is never called as well, but it is functional as it do not require any template.

  • Leave student team (GET #leave)

This is currently commented out in the committed code, because it will raise error every time when called. It seems like problem happens on

 line 96: other_members = TeamsUser.where( ['team_id = ?', params[:team_id]]).first
 line 97: if other_members.length == 0

This will raise error due to the calling length of other_members, which is a single object but not an array. Also, the related record seems to be remain undeleted in the database.

teams_controller

Current test cases are as following.

  • Create team should increase the number of teams by 1(GET #create)
  • Create team should increase the number of team nodes by 1(GET #create)
  • Create team with existing name (POST #create)
  • Create team should redirect to list assignments (POST #create)
  • Delete all teams (GET #delete_all)
  • Delete all teams should redirect to list (GET #delete_all)
  • List should receive assignment (GET #list)
  • List should receive course (GET #list)
  • New should assign parent (GET #new)
  • Update team should redirect (POST #update)
  • Update team should have validate name (POST #update)
  • Edit team should have time (POST #edit)
  • Delete team should redirect (POST #delete)
  • Delete team should decrease the number of teams by 1(GET #delete)
  • Delete team should decrease the number of team nodes by 1(GET #delete)
  • Inherit team should redirect (POST #inherit)
  • Bequeath team should redirect (POST #bequeath)

The methods in the following are ignored in testing since they are never used in current codes.

  • create_teams_view
  • create_teams

Sample Test Code

We show two pieces of test code for two controllers to explain how our test works

   test "create_student team with name in use" do
       sessionVars = session_for(users(:student8))
       post(:create, {'team' => { 'name' => 'IntelligentTeam2'}, 'id' => participants(:par21).id, "commit" => "Create Team"}, sessionVars, nil)
       assert_equal 'Team name is already in use.', flash[:notice]
       assert_redirected_to :controller => 'student_team', :action => 'view', :id => participants(:par21).id
   end
   test "create_should_increase_number_of_teams_course" do
       sessionVars = session_for(users(:instructor1))
       sessionVars[:team_type] = "Course"
       assert_difference 'Team.count' do
         get :create, {'id' => @testCourse,'team' => {'name' => "Random"}},sessionVars
       end
   end

The first piece of code is from student_team_controller.rb. It tests if there is already a same team name when student8 want to create a team. The second piece of code is from teams_controller_test.rb. It tests if the team number decreases when instructor1 creates a new team for a course.

As we can see from the code, when we want to use post or get, several parameters are needed. Usually we use four parameters: First, the method we are requesting. Second, an hash of request parameters for the method. Third, an optional hash of session variables. Forth, an optional hash of flash values. If we do not want to pass in the optional hash, we can set it to nil as the first piece of code shows, or we can just leave it blank as the second piece of code shows.

When we run the command rake test TEST=test/functional/your_test_file.rb, you will see if the test is passed or not.

Result

In this section, we will show the result form SimpleCov. The coverage is 24.08%.

Future Works

student_team_controller

Please note that this bug was reported fixed in the newest version of master branch of expertiza, but the fix came too late

There are several error raised from leave method need to be fixed, as mentioned above. Future teams may need to add tests on it as soon as it fixed, since it is a crucial functionality of the student team.

Also, methods that seems like not being used, such as advertise and remove_advertisement may need to be double checked and see if they are redundant.

teams_controller

The method create_teams_view does not be used at all. So it is safe to delete it.

The method create_teams calls the method randomize_all_by_parent, which is not correct. The developer may want to fix these bugs.

It's possible that we can write more testing cases if we improve the fixture.

References

<references/>