Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-7476

Move the unit test log configuration to a file

    XMLWordPrintable

Details

    • Story
    • Status: Won't Fix
    • Resolution: Done
    • None
    • None
    • None

    Description

      log is configurable at runtime using a log4cxx configuration file. Currently the configuration is hard-coded in utils.tests.init(). Moving it out to a configuration file will give more flexibility.

      Attachments

        Issue Links

          Activity

            DM-7150 was done with my newbie assumption of not changing the default in log, which is now changed in DM-7955. So I think of 3 possibilities with this ticket:
            (1) Revert DM-7150: the default in log is already good after DM-7955; there is no need to reconfigure log in utils.tests.
            (2) Still do this ticket to move utils tests configuration to a file: it's a trivial fix (see branch u/hfc/DM-7476). I thought it could be handy in some use cases.
            (3) Do nothing and close this ticket as Won't Fix.

            In particular, I wonder the opinions of tjenness and salnikov on this.

            hchiang2 Hsin-Fang Chiang added a comment - DM-7150 was done with my newbie assumption of not changing the default in log , which is now changed in DM-7955 . So I think of 3 possibilities with this ticket: (1) Revert DM-7150 : the default in log is already good after DM-7955 ; there is no need to reconfigure log in utils.tests . (2) Still do this ticket to move utils tests configuration to a file: it's a trivial fix (see branch u/hfc/ DM-7476 ). I thought it could be handy in some use cases. (3) Do nothing and close this ticket as Won't Fix. In particular, I wonder the opinions of tjenness and salnikov on this.

            Hsin-Fang, here are my thoughts on this:

            • if you expect to know the format of the logging messages or default logging level (e.g. in the tests which check output) then you probably do not want to depend on the current default configuration in the log package, there is no guarantee that it will not change again. This means that you want to configure log explicitly for the unit tests.
            • in that respect I'm not sure that that configuration file is better than current hard-coded configuration. One issue is the location of the config file, "config/log4cxx.unittest.properties" is probably not sufficient, this seem to imply that unit test runs in a particular directory which may not be always true.
            • if you think that people need to change logging configuration to re-run unit test with more verbose output (e.g. when debugging a failed unit test) then we can provide some mechanism to do that without requiring a configuration file. Simplest is probably to add temporarily log.setLevel('', log.TRACE) to the unit test code but we can do something even fancier.
            • I guess from my point of view this means WontFix but maybe extending documentation with examples of how to do more verbose logging in unit test if needed
            salnikov Andy Salnikov added a comment - Hsin-Fang, here are my thoughts on this: if you expect to know the format of the logging messages or default logging level (e.g. in the tests which check output) then you probably do not want to depend on the current default configuration in the log package, there is no guarantee that it will not change again. This means that you want to configure log explicitly for the unit tests. in that respect I'm not sure that that configuration file is better than current hard-coded configuration. One issue is the location of the config file, "config/log4cxx.unittest.properties" is probably not sufficient, this seem to imply that unit test runs in a particular directory which may not be always true. if you think that people need to change logging configuration to re-run unit test with more verbose output (e.g. when debugging a failed unit test) then we can provide some mechanism to do that without requiring a configuration file. Simplest is probably to add temporarily log.setLevel('', log.TRACE) to the unit test code but we can do something even fancier. I guess from my point of view this means WontFix but maybe extending documentation with examples of how to do more verbose logging in unit test if needed

            Thanks a lot, Andy, for your feedbacks. I agree with what you said. I don't think unit tests within science pipelines rely on log configuration, but know too little about downstream verification scripts. While I converted some packages to use log, I added placeholders/examples of setLevel in some unit tests. I'll update the documentation to make it clearer.

            tjenness, at later time if you see a need, we can revisit this.

            WontFix this ticket for now.

            hchiang2 Hsin-Fang Chiang added a comment - Thanks a lot, Andy, for your feedbacks. I agree with what you said. I don't think unit tests within science pipelines rely on log configuration, but know too little about downstream verification scripts. While I converted some packages to use log , I added placeholders/examples of setLevel in some unit tests. I'll update the documentation to make it clearer. tjenness , at later time if you see a need, we can revisit this. WontFix this ticket for now.

            People

              hchiang2 Hsin-Fang Chiang
              hchiang2 Hsin-Fang Chiang
              Andy Salnikov, Hsin-Fang Chiang
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.