All submissions to this site are governed by the Second Life Viewer Contribution Agreement. By submitting patches and other information using this site, you acknowledge that you have read, understood, and agreed to those terms.

Review Board 1.6.11

Welcome to the Second Life Viewer Code Review tool.
See the documentation on our wiki for how to use this site.

Enable legacy viewer C++ tests in indra/test.

Review Request #479 - Created Sept. 19, 2011 and submitted

Log Linden Reviewers
http://hg.lindenlab.com/nat/viewer-more-tests/ viewer
STORM-1634 nat_linden
None viewer-development 
As part of the Linden Lab automation hackathon, I finally had time to re-enable the tests located in the indra/test directory. These tests are currently not being built or run as part of a standard viewer build and haven't at least since we switched over from subversion. I copied over a few tests from the corresponding server test repository and also fixed some tests that had been rendered unbuildable by various code changes to the viewer. These tests can be disabled from running (but not building) by disabling the standard LL_TESTS cmake configuration variable.

llevents_tut.cpp proved to be an interesting challenge to get to pass on our Linux build servers.  With the Debian Lenny version of the gcc build toolchain, if an exception is located in a different dynamic library (.so) file as the code that is causing the exception, the exception will not be caught. My workaround was borrowed from the llmessage unit tests and expanded upon to allow the monitoring of the string from the caught exception.

I built and ran this code on recent versions of all three platforms with no error. When it failed to build in teamcity, I also built it on a lenny system by hand until I had fixed the exception handling issue, then ran all of the tests 100 times in a row on the same lenny machine with no test failures. The tests have run successfully in Teamcity for each revision since the fix for the exception handling code.
Review request changed
Updated (Oct. 4, 2011, 10:23 a.m.)
  • changed from to http://hg.lindenlab.com/nat/viewer-more-tests/ 
Added JIRA and repo containing the changes.
Nat Goodspeed
Posted (Oct. 5, 2011, 10:42 a.m.)

   

  

   

   
    
    
   


  






   







    

     
     

      

       
        

         
          indra/test/llsd_new_tut.cpp
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
Thumbs up for solving this up at the top rather than cluttering code inline. But I think I'd rather see 'using std::fpclassify;' on non-Windows, then put this local fpclassify() in the local namespace, and skip the std:: prefix when calling fpclassify() below.


     

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Thumbs up goes to whomever fixed this on the server. Fixed it on the viewer according to your recommendation, I agree that it's cleaner.
    
    
    
   
    2. 

   


  





    

    

    

     
     

      

       
        

         
          indra/test/llsd_new_tut.cpp
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
Hmm, where did this code come from? Server side?


     

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Yep, that version of this test seemed to be newer and fixed some of the disabled or failing tests (can't remember exactly, it's been almost a month!).
    
    
    
   
    2. 

   


  





    

    

    

     
     

      

       
        

         
          indra/test/llsdmessagebuilder_tut.cpp
          

           (Diff revision 1)

          
         
        

       
       
        
 


        
 


       
      

     

    

    

     
Srsly?? I'd much rather see the definition of _PREHASH_Test0 et al. change to make this work. Are those local?


     

  

   

   
    
    

   
  1. 
    
    
     
    
      
     
    
     
    Nope, they're in indra/llmessage/message_prehash.h. I could change the parameter const-ness in LLMessageBlock* createBlock, but I think I would also have to change  LLMessageBlock::addVariable which is in llmessagetemplate.h.
    
    
    
   
    2. 


   
  2. 
    
    
     
    
      
     
    
     
    Sigh, no, I would not advocate changing stuff in llmessage. I think that could open the door to a world of hurt...

But if _PREHASH_Test[01] are defined the same as the rest of the _PREHASH_Nonsense, how is it that the viewer build in general gets away with passing _PREHASH_Nonsense items to these methods without requiring similar ugly const_cast<char*>() casts?
    
    
    
   
    3. 


   
  3. 
    
    
     
    
      
     
    
     
    (Sorry for the very late answer.)

> But if _PREHASH_Test[01] are defined the same as the rest of the
> _PREHASH_Nonsense, how is it that the viewer build in general gets
> away with passing _PREHASH_Nonsense items to these methods without
> requiring similar ugly const_cast<char*>() casts?

I think that is because we fixed it, see e.g. https://codereview.secondlife.com/r/100/diff/3/#5

Probably createTemplateBlock(...), too, should take a char const* instead of a char*?
    
    
    
   
    4.