Loading ...
Sorry, an error occurred while loading the content.

CI Upgrade and Code Checks

Expand Messages
  • James Dempsey
    Hi, I ve updated our continuous integration server to the latest version of Jenkins (which used to be called Hudson). This seems
    Message 1 of 12 , May 1, 2011
    • 0 Attachment
      Hi,

      I've updated our continuous integration server to the latest version of Jenkins (which used to be called Hudson). This seems to be where all the big players and the plugin writers are going, so seemed the right time to move.

      In the process I have added the Cobertura and Violations plugins and configured them to track the PCGen Nightly Build (Trunk). This means we now have a history of the unit test coverage and can see the checkstyle violations.

      Unit test coverage is looking good thanks to Tom's thorough efforts, with many of the plugin packages having 100% coverage! A lot of the other areas of code are lower, but when you exclude the UI code we still have almost half of the classes and a third of the branches tested. Not bad at all given the code's complexity and our late start to unit testing.

      The checkstyle violations looks a bit dismal though - over 35000 things flagged :) Some are simple - I fixed the worst offender with a reformat for instance, but some will require a bit more effort. The plugin also supports PMD, but that takes a while to run on our huge code base - I'm considering running that weekly though as it will highlight perhaps higher impact issues, including copied code.

      The good news is that this list is something anyone can contribute to. If you have commit access to the code, have a look at the report and spend a few minutes to fix a couple of issues or even a whole class. The report guides you on what needs to be done. The changes are generally easy, low risk and can be done without having a huge block of time to focus on the task. If you don't have commit access and would like to - grab the source, make a small group of fixes, send me the patch and I'll review it and give you access to commit it.

      PS: Are there any checkstyle rule changes anyone thinks would be useful?

      Cheers,
      James Dempsey
      PCGen Code SB
    • Martijn Verburg
      Hi James, ... Fair call, I don t think Sonatype and Oracle will catch up for some time (although it s likely they will end up with a higher quality core
      Message 2 of 12 , May 2, 2011
      • 0 Attachment
        Hi James,

        Hi,

        I've updated our continuous integration server to the latest version of Jenkins (which used to be called Hudson). This seems to be where all the big players and the plugin writers are going, so seemed the right time to move.

        Fair call, I don't think Sonatype and Oracle will catch up for some time (although it's likely they will end up with a higher quality core codebase).

        In the process I have added the Cobertura and Violations plugins and configured them to track the PCGen Nightly Build (Trunk). This means we now have a history of the unit test coverage and can see the checkstyle violations.

        Awesome. 

        Unit test coverage is looking good thanks to Tom's thorough efforts, with many of the plugin packages having 100% coverage! A lot of the other areas of code are lower, but when you exclude the UI code we still have almost half of the classes and a third of the branches tested. Not bad at all given the code's complexity and our late start to unit testing.

        I think this is a fantastic effort and I _really_ applaud everyone who's contributed a test.  It means we can continue to change PCGen at a fairly rapid pace without fear, because we know what we're breaking!  This was impossible in the past. 

        The checkstyle violations looks a bit dismal though - over 35000 things flagged :) Some are simple - I fixed the worst offender with a reformat for instance, but some will require a bit more effort. The plugin also supports PMD, but that takes a while to run on our huge code base - I'm considering running that weekly though as it will highlight perhaps higher impact issues, including copied code.

        Heh, my local copy said 400,000+.  PMD and FindBugs are both fantastic tools for discovering code smells. 

        The good news is that this list is something anyone can contribute to. If you have commit access to the code, have a look at the report and spend a few minutes to fix a couple of issues or even a whole class. The report guides you on what needs to be done. The changes are generally easy, low risk and can be done without having a huge block of time to focus on the task. If you don't have commit access and would like to - grab the source, make a small group of fixes, send me the patch and I'll review it and give you access to commit it.

        Should we create an overriding JIRA for this?  Something like "Aim to get Checkstyle violations down to 25,000" or "Aim to get PMD violations down to 1000" as blanket JIRAs that people can contribute to?

        PS: Are there any checkstyle rule changes anyone thinks would be useful?

        We had the list we voted on in the Wiki.  Is this what you based the rules off?

        If we wan to go a step further in the future we can consider deploying a Sonar instance which takes away some of the heavy lifting of code analysis from Jenkins and provides a dedicated historical repository + prettier reporting.

        K
      • Andrew Wilson
        ... Hi James There are some things it s throwing up that I don t think are useful. It has an irrational fear of inline conditionals, we should probably disable
        Message 3 of 12 , May 4, 2011
        • 0 Attachment
          On 1 May 2011 23:15, James Dempsey <jdempsey@...> wrote:
          PS: Are there any checkstyle rule changes anyone thinks would be useful?
           
          Hi James
           
          There are some things it's throwing up that I don't think are useful.
           
          It has an irrational fear of inline conditionals, we should probably
          disable that.
           
          It's flagging up names that don't match some standard that we don't
          seem to adhere to at all.  It wants constants to be all uppercase, with
          words separated by underscores.  It also complains when a symbol
          that isn't a constant uses an underscore.  Are we sure we want to
          adopt these?
           
          andrew
        • Martijn Verburg
          Hi All, I firmly believe we should vote on each of these issues (by simple Wiki polling as it were). Also, do we need to be careful about applying formatting
          Message 4 of 12 , May 5, 2011
          • 0 Attachment
            Hi All,

            I firmly believe we should vote on each of these issues (by simple Wiki polling as it were).

            Also, do we need to be careful about applying formatting changes whilst we still have a major branch to merge in?

            K

            On 5 May 2011 00:24, Andrew Wilson <andrew@...> wrote:
             

            On 1 May 2011 23:15, James Dempsey <jdempsey@...> wrote:
            PS: Are there any checkstyle rule changes anyone thinks would be useful?
             
            Hi James
             
            There are some things it's throwing up that I don't think are useful.
             
            It has an irrational fear of inline conditionals, we should probably
            disable that.
             
            It's flagging up names that don't match some standard that we don't
            seem to adhere to at all.  It wants constants to be all uppercase, with
            words separated by underscores.  It also complains when a symbol
            that isn't a constant uses an underscore.  Are we sure we want to
            adopt these?
             
            andrew


          • James Dempsey
            Hi, I think wiki voting is the best option for variable naming, but for things like inline conditionals which we have always used I m happy to drop the rule
            Message 5 of 12 , May 5, 2011
            • 0 Attachment
              Hi,

              I think wiki voting is the best option for variable naming, but for things like inline conditionals which we have always used I'm happy to drop the rule (which I will do shortly). We can also vary the severity of various classes of issues too.

              I'm a fan of the constants in upper case with underscores rule and anything which highlights Hungarian notation for fixing is good IMO. While the 34000 issues might seem a lot, we are talking about quite a lot of code.

              Good point about the reformatting. Localised changes should be ok, but bulk changes, particularly of white-space and other layout will cause us much grief I would expect.

              Cheers,
              James.

              On 5/05/2011 5:08 PM Martijn Verburg wrote
              Hi All,

              I firmly believe we should vote on each of these issues (by simple Wiki polling as it were).

              Also, do we need to be careful about applying formatting changes whilst we still have a major branch to merge in?

              K

              On 5 May 2011 00:24, Andrew Wilson <andrew@...> wrote:
               
              On 1 May 2011 23:15, James Dempsey <jdempsey@...> wrote:
              PS: Are there any checkstyle rule changes anyone thinks would be useful?
               
              Hi James
               
              There are some things it's throwing up that I don't think are useful.
               
              It has an irrational fear of inline conditionals, we should probably
              disable that.
               
              It's flagging up names that don't match some standard that we don't
              seem to adhere to at all.  It wants constants to be all uppercase, with
              words separated by underscores.  It also complains when a symbol
              that isn't a constant uses an underscore.  Are we sure we want to
              adopt these?
               
              andrew
            • Andrew Wilson
              ... I don t mind upper case with underscores for constants. the thing that gave me pause was it complained about the use of underscores in something tht
              Message 6 of 12 , May 5, 2011
              • 0 Attachment
                On 5 May 2011 12:07, James Dempsey <jdempsey@...> wrote:
                I'm a fan of the constants in upper case with underscores rule and    anything which highlights Hungarian notation for fixing is good IMO.    While the 34000 issues might seem a lot, we are talking about quite    a lot of code.
                 
                I don't mind upper case with underscores for constants.  the thing that gave me pause was
                it complained about the use of underscores in something tht wasn't a constant.  it was one
                of a series of symbols where we have multiple of the same thing but for different bits of
                the GUI
                 
                 private static int classTab_AvailableListMode = GuiConstants.INFOCLASS_VIEW_NAME;
                 private static int classTab_SelectedListMode = GuiConstants.INFOCLASS_VIEW_NAME;
                 private static int equipTab_AvailableListMode = GuiConstants.INFOEQUIPPING_VIEW_EQUIPPED;
                 private static int equipTab_SelectedListMode = GuiConstants.INFOEQUIPPING_VIEW_NAME;
                do we want to eliminate the underscores to give us
                 
                ClassTabAvailableListMode
                ClassTabSelectedListMode
                EquipTabAvailableListMode
                EquipTabSelectedListMode
                 
                Good point about the reformatting. Localised changes should be ok, but bulk changes, particularly of white-space and other layout will cause us much grief I would expect.
                Does that mean I should stop doing this 'til after you've merged in?
                 
                andrew
              • James Dempsey
                Hi Andrew, On 5/05/2011 11:48 PM Andrew Wilson wrote ... Well if the first character were lower-case (probably your email editor being helpful) they would
                Message 7 of 12 , May 6, 2011
                • 0 Attachment
                  Hi Andrew,

                  On 5/05/2011 11:48 PM Andrew Wilson wrote
                  On 5 May 2011 12:07, James Dempsey <jdempsey@...> wrote:
                  I'm a fan of the constants in upper case with underscores rule and    anything which highlights Hungarian notation for fixing is good IMO.    While the 34000 issues might seem a lot, we are talking about quite    a lot of code.
                   
                  I don't mind upper case with underscores for constants.  the thing that gave me pause was
                  it complained about the use of underscores in something tht wasn't a constant.  it was one
                  of a series of symbols where we have multiple of the same thing but for different bits of
                  the GUI
                   
                   private static int classTab_AvailableListMode = GuiConstants.INFOCLASS_VIEW_NAME;
                   private static int classTab_SelectedListMode = GuiConstants.INFOCLASS_VIEW_NAME;
                   private static int equipTab_AvailableListMode = GuiConstants.INFOEQUIPPING_VIEW_EQUIPPED;
                   private static int equipTab_SelectedListMode = GuiConstants.INFOEQUIPPING_VIEW_NAME;
                  do we want to eliminate the underscores to give us
                   
                  ClassTabAvailableListMode
                  ClassTabSelectedListMode
                  EquipTabAvailableListMode
                  EquipTabSelectedListMode
                  Well if the first character were lower-case (probably your email editor being helpful) they would match the usual standard yes. I've spent a lot of time in the GUI code and haven't spent the effort to change it, so I'm obviously not too stressed :) I think I have excised most of the m_Foo style entries. They give me bad flashbacks to my early days in the Cobol mines!

                  All that said, voting on these issues is a good thing so we get a consensus view and make the code environment better for all of us to work in.

                   
                  Good point about the reformatting. Localised changes should be ok, but bulk changes, particularly of white-space and other layout will cause us much grief I would expect.
                  Does that mean I should stop doing this 'til after you've merged in?
                   
                  I might do a scan and let you know where is safe and where we should be careful. In general the core and the plugins are fine to work on, the GUI is probably wasted effort at this stage and the loading stuff is being changed so it would be best to avoid.

                  Cheers,
                  James.
                • Andrew Wilson
                  ... Ok, I ve never had the pleasure of doing Cobol, but if we want to eliminate all the single character at the start followed by an underscore I can get
                  Message 8 of 12 , May 7, 2011
                  • 0 Attachment
                    On 7 May 2011 02:45, James Dempsey <jdempsey@...> wrote:
                     private static int classTab_AvailableListMode = GuiConstants.INFOCLASS_VIEW_NAME;
                     private static int classTab_SelectedListMode = GuiConstants.INFOCLASS_VIEW_NAME;
                     private static int equipTab_AvailableListMode = GuiConstants.INFOEQUIPPING_VIEW_EQUIPPED;
                     private static int equipTab_SelectedListMode = GuiConstants.INFOEQUIPPING_VIEW_NAME;
                    do we want to eliminate the underscores to give us
                     
                    ClassTabAvailableListMode
                    ClassTabSelectedListMode
                    EquipTabAvailableListMode
                    EquipTabSelectedListMode
                     
                    Well if the first character were lower-case (probably your email editor being helpful) they would match the usual standard yes. I've spent a lot of time in the GUI code and haven't spent the effort to change it, so I'm obviously not too stressed :) I think I have excised most of the m_Foo style entries. They give me bad flashbacks to my early days in the Cobol mines!

                    All that said, voting on these issues is a good thing so we get a consensus view and make the code environment better for all of us to work in.
                        
                    Ok, I've never had the "pleasure" of doing Cobol, but if we want to eliminate all the single character at the start followed by an underscore I can get onboard with that. 
                    Good point about the reformatting. Localised changes should be ok, but bulk changes, particularly of white-space and other layout will cause us much grief I would expect.
                    Does that mean I should stop doing this 'til after you've merged in?
                     
                    I might do a scan and let you know where is safe and where we should be careful. In general the core and the plugins are fine to work on, the GUI is probably wasted effort at this stage and the loading stuff is being changed so it would be best to avoid.
                      
                    I have some changes in the core.  It took a bit of faffing about, but I've finally got idea to highlight the checkstyle problems as real time checks as I'm working on a file and that makes them a bit easier to deal with.
                     
                    andrew
                  • Martijn Verburg
                    As a reminder to all developers, you can vote for the checkstyle and other coding standards here: http://wiki.pcgen.org/Coding_Standards The voting is only
                    Message 9 of 12 , May 7, 2011
                    • 0 Attachment
                      As a reminder to all developers, you can vote for the checkstyle and other coding standards here:


                      The voting is only open to those who have committed code in the past 6 months

                      Feel free to add new rules etc that we can all vote on

                      K

                      On 7 May 2011 12:51, Andrew Wilson <andrew@...> wrote:
                       

                      On 7 May 2011 02:45, James Dempsey <jdempsey@...> wrote:
                       private static int classTab_AvailableListMode = GuiConstants.INFOCLASS_VIEW_NAME;
                       private static int classTab_SelectedListMode = GuiConstants.INFOCLASS_VIEW_NAME;
                       private static int equipTab_AvailableListMode = GuiConstants.INFOEQUIPPING_VIEW_EQUIPPED;
                       private static int equipTab_SelectedListMode = GuiConstants.INFOEQUIPPING_VIEW_NAME;
                      do we want to eliminate the underscores to give us
                       
                      ClassTabAvailableListMode
                      ClassTabSelectedListMode
                      EquipTabAvailableListMode
                      EquipTabSelectedListMode
                       
                      Well if the first character were lower-case (probably your email editor being helpful) they would match the usual standard yes. I've spent a lot of time in the GUI code and haven't spent the effort to change it, so I'm obviously not too stressed :) I think I have excised most of the m_Foo style entries. They give me bad flashbacks to my early days in the Cobol mines!

                      All that said, voting on these issues is a good thing so we get a consensus view and make the code environment better for all of us to work in.
                          
                      Ok, I've never had the "pleasure" of doing Cobol, but if we want to eliminate all the single character at the start followed by an underscore I can get onboard with that. 
                      Good point about the reformatting. Localised changes should be ok, but bulk changes, particularly of white-space and other layout will cause us much grief I would expect.
                      Does that mean I should stop doing this 'til after you've merged in?
                       
                      I might do a scan and let you know where is safe and where we should be careful. In general the core and the plugins are fine to work on, the GUI is probably wasted effort at this stage and the loading stuff is being changed so it would be best to avoid.
                        
                      I have some changes in the core.  It took a bit of faffing about, but I've finally got idea to highlight the checkstyle problems as real time checks as I'm working on a file and that makes them a bit easier to deal with.
                       
                      andrew


                    • Andrew Wilson
                      Right, now I ve done a few of them I have some other points. I reckon there s 9 man months to a year s worth of effort involved in cleaning these up :-( One
                      Message 10 of 12 , Jun 2, 2011
                      • 0 Attachment
                        Right, now I've done a few of them I have some other points.
                         
                        I reckon there's 9 man months to a year's worth of effort involved in cleaning
                        these up :-( One full day at the weekend every weekend for five years.  Yikes!
                        Just sayin' 's'all.
                         
                        One of the files I'm working on is cmplaining about an empty catch block.  Thing
                        is it's commented, so someone's put a bit of thought into it.  Do we really care
                        about this?  if not how do I get rid of the warning.  do we have a supression
                        mechanism that we're using?
                        andrew
                         
                        On 5 May 2011 12:07, James Dempsey <jdempsey@...> wrote:
                        Hi,

                        I think wiki voting is the best option for variable naming, but for things like inline
                        conditionals which we have always used I'm happy to drop the rule (which I will
                        do shortly). We can also vary the severity of various classes of issues too.
                      • James Dempsey
                        Hi Andrew, On 3/06/2011 4:04 AM Andrew Wilson wrote ... Thanks for that - it has been great to see the clean-up progressing. ... Yes there is little doubt that
                        Message 11 of 12 , Jun 6, 2011
                        • 0 Attachment
                          Hi Andrew,

                          On 3/06/2011 4:04 AM Andrew Wilson wrote
                          Right, now I've done a few of them I have some other points.
                          Thanks for that - it has been great to see the clean-up progressing.

                           
                          I reckon there's 9 man months to a year's worth of effort involved in cleaning
                          these up :-( One full day at the weekend every weekend for five years.  Yikes!
                          Just sayin' 's'all.
                          Yes there is little doubt that it is a big job. One primary benefit of setting up the violations plugin is that we can ensure it doesn't get any worse and instead gradually improve the situation.

                           
                          One of the files I'm working on is cmplaining about an empty catch block.  Thing
                          is it's commented, so someone's put a bit of thought into it.  Do we really care
                          about this?  if not how do I get rid of the warning.  do we have a supression
                          mechanism that we're using?

                          One technique I have used in the past is to add a log entry to the catch block. Generally ignoring exceptions is a bad idea so the PMD/Checkstyle warning is warranted IMO.

                          Cheers,
                          James.
                        • Andrew Wilson
                          ... Generally ignoring exceptions is a bad idea so the PMD/Checkstyle warning ... warranted IMO. ... Fair enough, what severity of thing should I log it as?
                          Message 12 of 12 , Jun 6, 2011
                          • 0 Attachment
                            On 6 June 2011 23:08, James Dempsey <jdempsey@...> wrote:
                            > One of the files I'm working on is complaining about an empty catch block.  Thing 
                            > is it's commented, so someone's put a bit of thought into it.  Do we really care 
                            > about this?  if not how do I get rid of the warning.  do we have a suppression 
                            > mechanism that we're using?

                            One technique I have used in the past is to add a log entry to the catch block. 
                            Generally ignoring exceptions is a bad idea so the PMD/Checkstyle warning is 
                            warranted IMO.

                            Fair enough, what severity of thing should I log it as? 

                            andrew
                          Your message has been successfully submitted and would be delivered to recipients shortly.