Review Board 1.0.8

ResultsController is now tested

Updated 8 months, 1 week ago

Gabriel Roy-Lortie Reviewers
markus_developers
400, 448
None MarkUs Source Code Repository 
Relates to tickets #400 and #448

I'm not implying that it's a thorough test suite, but It's a lot better than it was (empty). It also allowed me to figure out, and remove a couple of bugs.

Regarding #448: The three methods were effectively dead code. They've been removed.

The new fixture files will ship with my first review to get a "ship it". This one, or the FlexibleCriteriaController one.
This review request is all about writing tests.

Unit tests are still top shape.
I have to say, for the record, that, by modifying *many* fixture files I introduced a failure in one of the "functional" test.
Before: 29 failures, 28 errors
After: 30 failures, 24 errors

Unfortunately, I can't figure out where it is. Which one, of the 30 failures, is the new one? I'm sorry that I can't tell.
What I can tell, in revenge, is that: 1) all recently developed tests are still working; and 2) there's no test that features only one failure -- Implying that no test suite went from clean to broken... those that worked, still does, and those that were broken still are.

It's surely not perfect. But I believe that letting this code through to the repository has more advantages than inconvenients.

Let me state once again that I don't like fixtures. I am still looking in a replacement. I had some issues with factory_girl and am on to look for machinist. I'll keep you posted.
Gabriel Roy-Lortie
Posted 8 months, 2 weeks ago (November 12th, 2009, 1:05 a.m.)
Some comments on my modifications.
Was dead code.
Using an array leads to a crash on line (old:50, new:29) when used with a big criterion.id

The array didn't made sense anyway.
Same Array problem here.
Was dead code.
Was dead code.
Rails was complaining about that space.
Same here.
Review request changed
Updated 8 months, 2 weeks ago (November 13th, 2009, 4:52 p.m.)
Added partial files: views/shared/_handle_error.rjs and views/results/marker/_update_mark.rjs

Removed all occurences of:

    partial :update {|page| page...}

replacing them with the partials I created.
Gabriel Roy-Lortie
Posted 8 months, 2 weeks ago (November 16th, 2009, 1:28 p.m.)
Any reason why nobody reviewed yet?
  1. I'm on it right now.  :)
Mike Conley
Posted 8 months, 2 weeks ago (November 16th, 2009, 2:03 p.m.)
Looks great!  I just had one question about the ResultsControllerTest concerning a TA using the download method.  See below.
Ah, much better.  Thinner controllers is good!

Well, you could always take the response_body, and use some regex to see if the error string you're looking for is in there.  It's a bit hacky, but it's an idea.
  1. Yes, I believe it would be decent workaround.
Really?  I'm pretty sure both TA's and Admin's can download files from the ResultsController.
  1. These are the before_filters applied to ResultsController...

  before_filter      :authorize_only_for_admin, :except => [:codeviewer, :edit, :update_mark, :view_marks, :create, :add_extra_mark, :next_grouping, :update_overall_comment, :expand_criteria, :collapse_criteria, :remove_extra_mark, :expand_unmarked_criteria, :update_marking_state]
  before_filter      :authorize_for_ta_and_admin, :only => [:edit, :update_mark, :create, :add_extra_mark, :download, :next_grouping, update_overall_comment, :expand_criteria, :collapse_criteria, :remove_extra_mark, :expand_unmarked_criteria, :update_marking_state]
  before_filter      :authorize_for_user, :only => [:codeviewer]
  before_filter      :authorize_for_student, :only => [:view_marks]

As you can see... there is an :authorize_for_ta_and_admin on :dowload... but I believe it is overridden by the :authorize_only_for_admin that doesn't has an :except clause on dowload.

Furthermore... the test pass... so I believe it really responds with :missing.

Would you like me to change that?
  2. Yes, please!  :D
Review request changed
Updated 8 months, 2 weeks ago (November 16th, 2009, 5:03 p.m.)
Applied Mike's comments:

Using a regex to assert that the response contains the appropriate error messages (when rendering a partial).
Allowed for TAs to send GET download requests.
Severin Gehwolf
Posted 8 months, 2 weeks ago (November 16th, 2009, 5:22 p.m.)

   

  

   
    
    
   


  




   



    

     
     
    

    

     
I think it would be a good idea to put this "sample error msg" in a variable and use it throughout the test (i.e. in the assert_match). 

     

  

   
    
    
   


  



    

    

   


  
 




  

 














 

  




 

   Review request changed
   
Updated 8 months, 1 week ago (November 20th, 2009, midnight)

 

 

  

  
  
Replaced all those 'sample error msg' string with a constant.


 




  

 















 

  




 

 

  
Ship it!

  
Severin Gehwolf

  
Posted 8 months, 1 week ago (November 20th, 2009, 8:07 a.m.)

 

 

   
This is great work! Your well deserved ship it.