tests: Fail if RPC has been added without tests by maflcko · Pull Request #15943 · bitcoin/bitcoin

maflcko · 2019-05-02T20:31:58Z

Need to be run with --coverage

jnewbery

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

test/functional/test_runner.py

DrahtBot · 2019-05-02T21:29:50Z

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

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.

maflcko · 2019-05-02T22:01:03Z

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

practicalswift · 2019-05-03T07:56:01Z

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

gmaxwell · 2019-05-03T08:05:10Z

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").

maflcko · 2019-05-03T12:13:36Z

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.

practicalswift · 2019-05-03T12:38:14Z

Concept ACK

Agree with @MarcoFalke.

jnewbery · 2019-05-03T13:34:21Z

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.

promag

Concept ACK.

Currently failing with

Uncovered RPC commands:
  - pruneblockchain

.travis.yml

maflcko · 2019-05-15T00:13:39Z

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

practicalswift · 2019-05-15T05:05:19Z

utACK fa7ea62

jonatack

Concept ACK

test/functional/test_runner.py

jonatack · 2019-05-15T11:39:24Z

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

maflcko · 2019-05-15T11:42:50Z

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

jonatack · 2019-05-15T11:46:25Z

Gotcha. Doing more testing.

ryanofsky

utACK fa7ea62

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.

.travis.yml

ryanofsky

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

fad0ce5 tests: Fail if RPC has been added without tests (MarcoFalke) Pull request description: Need to be run with --coverage ACKs for commit fad0ce: ryanofsky: utACK fad0ce5. New comment in travis.yml is the only change since last review. Tree-SHA512: b53632dfe9865ec06991bfcba2fd67238bebbb866b355f09624eaf233257b2bca902caac6c24abb358b2f4c1c43f28ca75e30982765911e1a117102df65276d9

Summary: fad0ce59e9 tests: Fail if RPC has been added without tests (MarcoFalke) Pull request description: Need to be run with --coverage ACKs for commit fad0ce: ryanofsky: utACK fad0ce59e9154f9b7e61907a71c740a942c60282. New comment in travis.yml is the only change since last review. Tree-SHA512: b53632dfe9865ec06991bfcba2fd67238bebbb866b355f09624eaf233257b2bca902caac6c24abb358b2f4c1c43f28ca75e30982765911e1a117102df65276d9 Backport of Core [[bitcoin/bitcoin#15943 | PR15943]] Test Plan: test_runner.py --coverage --extended Verify `All RPC commands covered.` is printed to the console. Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5743

fad0ce59e9 tests: Fail if RPC has been added without tests (MarcoFalke) Need to be run with --coverage Backport of Core [PR15943](bitcoin/bitcoin#15943) Test Plan: ``` test_runner.py --coverage --extended ``` Verify `All RPC commands covered.` is printed to the console. Differential Revision: https://reviews.bitcoinabc.org/D5743

fad0ce5 tests: Fail if RPC has been added without tests (MarcoFalke) Pull request description: Need to be run with --coverage ACKs for commit fad0ce: ryanofsky: utACK fad0ce5. New comment in travis.yml is the only change since last review. Tree-SHA512: b53632dfe9865ec06991bfcba2fd67238bebbb866b355f09624eaf233257b2bca902caac6c24abb358b2f4c1c43f28ca75e30982765911e1a117102df65276d9

Merge bitcoin PRs: bitcoin#14683, bitcoin#15841, bitcoin#14984 and bitcoin#15943

fad0ce5 tests: Fail if RPC has been added without tests (MarcoFalke) Pull request description: Need to be run with --coverage ACKs for commit fad0ce: ryanofsky: utACK fad0ce5. New comment in travis.yml is the only change since last review. Tree-SHA512: b53632dfe9865ec06991bfcba2fd67238bebbb866b355f09624eaf233257b2bca902caac6c24abb358b2f4c1c43f28ca75e30982765911e1a117102df65276d9

DrahtBot added the Tests label May 2, 2019

jnewbery reviewed May 2, 2019

View reviewed changes

test/functional/test_runner.py Outdated Show resolved Hide resolved

maflcko force-pushed the 1905-testFailNoRpcCov branch from fa20517 to fa054ba Compare May 3, 2019 17:45

promag reviewed May 6, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

maflcko force-pushed the 1905-testFailNoRpcCov branch 3 times, most recently from faffa16 to fa7ea62 Compare May 14, 2019 21:36

jonatack reviewed May 15, 2019

View reviewed changes

test/functional/test_runner.py Show resolved Hide resolved

ryanofsky approved these changes May 15, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

tests: Fail if RPC has been added without tests

fad0ce5

maflcko force-pushed the 1905-testFailNoRpcCov branch from fa7ea62 to fad0ce5 Compare May 15, 2019 18:29

ryanofsky approved these changes May 15, 2019

View reviewed changes

maflcko merged commit fad0ce5 into bitcoin:master May 15, 2019

maflcko deleted the 1905-testFailNoRpcCov branch May 15, 2019 19:53

dzutto mentioned this pull request Oct 12, 2021

Merge bitcoin PRs: #14683, #15841, #14984 and #15943 dashpay/dash#4514

Merged

UdjinM6 added a commit to dashpay/dash that referenced this pull request Oct 13, 2021

Merge pull request #4514 from dzutte-cpp/merge_14683_15841_14984_15943

4ac54be

Merge bitcoin PRs: bitcoin#14683, bitcoin#15841, bitcoin#14984 and bitcoin#15943

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

maflcko commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented May 2, 2019

Uh oh!

practicalswift commented May 3, 2019

Uh oh!

gmaxwell commented May 3, 2019

Uh oh!

maflcko commented May 3, 2019

Uh oh!

practicalswift commented May 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented May 3, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented May 15, 2019

Uh oh!

practicalswift commented May 15, 2019

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented May 15, 2019

Uh oh!

maflcko commented May 15, 2019

Uh oh!

jonatack commented May 15, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

maflcko commented May 2, 2019 •

edited

Loading

DrahtBot commented May 2, 2019 •

edited

Loading

practicalswift commented May 3, 2019 •

edited

Loading