tests: Fail if RPC has been added without tests #15943

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:1905-testFailNoRpcCov changing 4 files +16 −3
  1. MarcoFalke commented at 8:31 PM on May 2, 2019: member

    Need to be run with --coverage

  2. DrahtBot added the label Tests on May 2, 2019
  3. in test/functional/test_runner.py:226 in fa205179f0 outdated 
     222 | @@ -223,6 +223,7 @@ def main():
 223 |                                       formatter_class=argparse.RawTextHelpFormatter)
 224 |      parser.add_argument('--combinedlogslen', '-c', type=int, default=0, metavar='n', help='On failure, print a log (of length n lines) to the console, combined from the test framework and all test nodes.')
 225 |      parser.add_argument('--coverage', action='store_true', help='generate a basic coverage report for the RPC interface')
 226 | +    parser.add_argument('--allow_uncovered_rpcs', action='store_true', help='Uncovered RPCs do not fail the test run')
    jnewbery commented at 8:54 PM on May 2, 2019:

    sort please :)

    MarcoFalke commented at 10:00 PM on May 2, 2019:

    It is sorted logically, comes right after --coverage

  4. jnewbery commented at 8:55 PM on May 2, 2019: member

    Concept ACK, although I'd prefer to reverse the defaults (ie only fail if a -failifuncoveredrpc or similar flag is passed)

  5. DrahtBot commented at 9:29 PM on May 2, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15812 (Do not generate coverage report on test failures by crackercracked)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  6. MarcoFalke commented at 10:01 PM on May 2, 2019: member

    Concept ACK, although I'd prefer to reverse the defaults (ie only fail if a -failifuncoveredrpc or similar flag is passed)

    this is only active when --coverage is passed

  7. practicalswift commented at 7:56 AM on May 3, 2019: contributor

    @MarcoFalke Will Travis fail if RPC has been added without tests?

  8. gmaxwell commented at 8:05 AM on May 3, 2019: contributor

    A concern that comes to mind is that if travis is just arbitrarily failing on new rpc's "without tests" it's going to encourage adding more pre-textual tests that don't really test anything and just run the rpc, or which inappropriately hardcode some output ("software changed detection").

  9. MarcoFalke commented at 12:13 PM on May 3, 2019: member

    This is already an issue with the existing coverage report, which motivated people to test for exact verbose responses. Though, I have a slight hope that review might catch those before merge. If it doesn't we have at least a test as opposed to no test and get notified of the issue the next time the response changes.

  10. practicalswift commented at 12:38 PM on May 3, 2019: contributor

    Concept ACK

    Agree with @MarcoFalke.

  11. jnewbery commented at 1:34 PM on May 3, 2019: member

    This is already an issue with the existing coverage report, which motivated people to test for exact verbose responses. Though, I have a slight hope that review might catch those before merge. If it doesn't we have at least a test as opposed to no test and get notified of the issue the next time the response changes.

    Totally agree with this. The way to prevent bad test code being merged is to catch it at review.

  12. MarcoFalke force-pushed on May 3, 2019
  13. in .travis.yml:155 in fa054ba3bd outdated 
     151 | @@ -152,6 +152,7 @@ jobs:
 152 |          DOCKER_NAME_TAG=ubuntu:16.04
 153 |          PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libssl-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-chrono-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libprotobuf-dev protobuf-compiler libqrencode-dev"
 154 |          NO_DEPENDS=1
 155 | +        ALLOW_UNCOVERED_RPCS="--allow_uncovered_rpcs"
    promag commented at 1:41 PM on May 6, 2019:

    Use a generic variable name instead? Like TEST_RUNNER_EXTRA="--allow-uncovered_rpcs"?

  14. promag commented at 1:44 PM on May 6, 2019: member

    Concept ACK.

    Currently failing with

    Uncovered RPC commands:
  - pruneblockchain
  15. MarcoFalke force-pushed on May 14, 2019
  16. MarcoFalke force-pushed on May 14, 2019
  17. MarcoFalke force-pushed on May 14, 2019
  18. MarcoFalke commented at 12:13 AM on May 15, 2019: member

    Addressed @jnewbery and @promag feedback. Example output is here: https://travis-ci.org/bitcoin/bitcoin/jobs/532518417#L3769

  19. practicalswift commented at 5:05 AM on May 15, 2019: contributor

    utACK fa7ea62b140c76951488c54bac3dcc7d7568d736

  20. in test/functional/test_runner.py:415 in fa7ea62b14 outdated 
     412 |      # Clear up the temp directory if all subdirectories are gone
 413 |      if not os.listdir(tmpdir):
 414 |          os.rmdir(tmpdir)
 415 |  
 416 | -    all_passed = all(map(lambda test_result: test_result.was_successful, test_results))
 417 | +    all_passed = all(map(lambda test_result: test_result.was_successful, test_results)) and coverage_passed
    jonatack commented at 11:04 AM on May 15, 2019:

    Mixed use of coverage_passed

    might be more explicit to omit lines 408-409 and replace coverage_passed with (coverage and coverage_passed)

    MarcoFalke commented at 11:33 AM on May 15, 2019:

    I don't understand (coverage and coverage_passed) would always be false in a default run

    jonatack commented at 11:45 AM on May 15, 2019:

    something like (coverage is None or coverage_passed)

  21. jonatack commented at 11:07 AM on May 15, 2019: member

    Concept ACK

  22. jonatack commented at 11:39 AM on May 15, 2019: member

    Note that testing a single file through the runner with --coverage outputs something like this: https://gist.github.com/jonatack/f6fd73d25ebfc8222ab3594cc322735c

  23. MarcoFalke commented at 11:42 AM on May 15, 2019: member

    That is expected and I don't think I changed that in this pull request?

  24. jonatack commented at 11:46 AM on May 15, 2019: member

    Gotcha. Doing more testing.

  25. in .travis.yml:124 in fa7ea62b14 outdated 
     120 | @@ -121,6 +121,7 @@ jobs:
 121 |          HOST=x86_64-unknown-linux-gnu
 122 |          PACKAGES="python3-zmq qtbase5-dev qttools5-dev-tools protobuf-compiler libdbus-1-dev libharfbuzz-dev libprotobuf-dev"
 123 |          DEP_OPTS="NO_QT=1 NO_UPNP=1 DEBUG=1 ALLOW_HOST_PACKAGES=1"
 124 | +        TEST_RUNNER_EXTRA="--coverage --extended --exclude feature_dbcrash"
    ryanofsky commented at 6:12 PM on May 15, 2019:

    What's purpose of excluding feature_dbcrash? Can you add a comment in the code saying why it's needed?

    It would also be nice to know about why the extended flag is added, if adding it is intentional.

    MarcoFalke commented at 6:29 PM on May 15, 2019:

    Done

  26. ryanofsky approved
  27. ryanofsky commented at 6:20 PM on May 15, 2019: member

    utACK fa7ea62b140c76951488c54bac3dcc7d7568d736

    going to encourage adding more pre-textual tests that don't really test anything and just run the rpc

    IMO, while this is a legitimate concern, if the ultimate goal is to have meaningful tests for all RPCs, it's often a lot easier to start from a minimal test and extend it to be more interesting, than to have to start from scratch and have to figure out how to create a context where the RPC can be called at all.

  28. tests: Fail if RPC has been added without tests fad0ce59e9
  29. MarcoFalke force-pushed on May 15, 2019
  30. ryanofsky approved
  31. ryanofsky commented at 6:43 PM on May 15, 2019: member

    utACK fad0ce59e9154f9b7e61907a71c740a942c60282. New comment in travis.yml is the only change since last review.

  32. MarcoFalke referenced this in commit 9d266dbdec on May 15, 2019
  33. MarcoFalke merged this on May 15, 2019
  34. MarcoFalke closed this on May 15, 2019
  35. MarcoFalke deleted the branch on May 15, 2019
  36. deadalnix referenced this in commit c7f7fe7721 on Apr 16, 2020
  37. ftrader referenced this in commit f0d275e75a on Aug 17, 2020
  38. dzutto referenced this in commit 6141ede4d9 on Oct 12, 2021
  39. dzutto referenced this in commit 1940ecc725 on Oct 12, 2021
  40. dzutto referenced this in commit 652a36b0f2 on Oct 12, 2021
  41. UdjinM6 referenced this in commit 4ac54be71b on Oct 13, 2021
  42. pravblockc referenced this in commit 6c9a0bcb99 on Nov 18, 2021
  43. DrahtBot locked this on Dec 16, 2021
Contributors
MarcoFalkejnewberyDrahtBotpracticalswiftgmaxwellpromagjonatackryanofsky
Labels
Tests
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