test: Explain why test logging should be used #18305

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2003-docWhyTestLog changing 2 files +5 −2
  1. MarcoFalke commented at 6:36 PM on March 9, 2020: member

    Background is that some tests don't have any self.log call at all. Thus there are no "anchor points" and those tests are hard to debug because the logs can't easily be parsed by a human.

  2. jonatack commented at 7:08 PM on March 9, 2020: member

    Yes, I have a list of tests I wanted to add logging to, but I was not sure that kind of change was desired. I ended up closing my last PR like that (#17535) due to lack of interest. Maybe one PR that adds all the missing logging would be helpful.

  3. DrahtBot added the label Docs on Mar 9, 2020
  4. DrahtBot added the label Tests on Mar 9, 2020
  5. MarcoFalke commented at 7:12 PM on March 9, 2020: member

    Looks like #17535 changed the release notes, refactored the test, and added logging. It seems a bit too much for one pull.

  6. in test/README.md:148 in fa47bf195f outdated 
     144 | @@ -145,7 +145,7 @@ levels using the logger included in the test_framework, e.g.
 145 |    `test_framework.log` and no logs are output to the console.
 146 |  - when run directly, *all* logs are written to `test_framework.log` and INFO
 147 |    level and above are output to the console.
 148 | -- when run on Travis, no logs are output to the console. However, if a test
 149 | +- when run [on ci](/ci/README.md), no logs are output to the console. However, if a test
    jonatack commented at 7:58 PM on March 9, 2020:

    perhaps: "when run by the CI (Continuous Integration),"

    MarcoFalke commented at 8:25 PM on March 9, 2020:

    Adjusted to "our CI"

  7. in test/functional/README.md:54 in fa47bf195f outdated 
      50 | @@ -51,10 +51,13 @@ don't have test cases for.
  51 |  
  52 |  #### General test-writing advice
  53 |  
  54 | +- Instead of inline comments or no test documentation at all, prefer to log the comments to the test log. E.g.
    jonatack commented at 8:02 PM on March 9, 2020:

    • maybe omit "prefer to"

    • s/. E.g./, e.g./

    MarcoFalke commented at 8:25 PM on March 9, 2020:

    Done both

  8. in test/functional/README.md:55 in fa47bf195f outdated 
      50 | @@ -51,10 +51,13 @@ don't have test cases for.
  51 |  
  52 |  #### General test-writing advice
  53 |  
  54 | +- Instead of inline comments or no test documentation at all, prefer to log the comments to the test log. E.g.
  55 | +  `self.log.info('Creating enough transactions to fill a block'`. Logs make the test code easier to read and the test
    jonatack commented at 8:04 PM on March 9, 2020:

    According to review feedback, it may be better (and shorter) to use the indicative, e.g. "Create..." rather than "Creating..."

    jonatack commented at 8:16 PM on March 9, 2020:

    (That comment proposes some good guidelines for your consideration:)

    • "test x" rather than "testing x"
    • logs can replace docstrings/comment blocks
    • no need for trailing '...' ellipses
    MarcoFalke commented at 8:25 PM on March 9, 2020:

    Thanks, made shorter

  9. jonatack commented at 8:07 PM on March 9, 2020: member

    ACK modulo comments

  10. MarcoFalke force-pushed on Mar 9, 2020
  11. MarcoFalke commented at 8:26 PM on March 9, 2020: member

    Adressed @jonatack feedback

  12. jonatack commented at 8:41 PM on March 9, 2020: member

    ACK. Looked at the changes on the PR branch. Good additions and :+1: to no longer refer to travis.

  13. in test/functional/README.md:55 in eeeedcb77c outdated 
      50 | @@ -51,10 +51,13 @@ don't have test cases for.
  51 |  
  52 |  #### General test-writing advice
  53 |  
  54 | +- Instead of inline comments or no test documentation at all, log the comments to the test log, e.g.
  55 | +  `self.log.info('Create enough transactions to fill a block'`. Logs make the test code easier to read and the test
    fanquake commented at 11:54 AM on March 10, 2020:

    nit: does this need a closing )

    jonatack commented at 12:20 PM on March 10, 2020:

    good eye

    MarcoFalke commented at 12:25 PM on March 10, 2020:

    Added ), and changed commit hash from eeee... to ffff...

  14. fanquake approved
  15. fanquake commented at 11:54 AM on March 10, 2020: member

    ACK eeeedcb77c89c2be4545670bc9f47b86c3a6031c

  16. test: Explain why test logging should be used ffff9dcdc3
  17. MarcoFalke force-pushed on Mar 10, 2020
  18. fanquake approved
  19. fanquake commented at 12:31 PM on March 10, 2020: member

    re-ACK ffff9dcdc3cbe427739cc19cc7a53f032474fa2a

  20. jonatack commented at 12:41 PM on March 10, 2020: member

    ACK ffff9dcdc3cbe427739cc19cc7a53f032474fa2a

  22. MarcoFalke merged this on Mar 10, 2020
  23. MarcoFalke closed this on Mar 10, 2020
  24. MarcoFalke deleted the branch on Mar 10, 2020
  25. Fabcien referenced this in commit d46d359d3e on Aug 24, 2021
  26. DrahtBot locked this on Feb 15, 2022
Contributors
MarcoFalkejonatackfanquakeinstagibbs
Labels
DocsTests
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 06:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me