Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add coverage to rpc_scantxoutset.py #27422

Merged

Conversation

ismaelsadeeq
Copy link
Member

@ismaelsadeeq ismaelsadeeq commented Apr 5, 2023

Include a test that checks whether the first argument of scantxoutset RPC call "start" is required.
The rpc call should fail if the "start" argument is not provided.

Include a test that checks whether the first argument of
scantxoutset RPC call is required. The rpc call should fail if
the "start" argument is not provided.
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 5, 2023

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke
Concept ACK kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot changed the title test: add coverage to rpc_scantxoutset.py test: add coverage to rpc_scantxoutset.py Apr 5, 2023
@DrahtBot DrahtBot added the Tests label Apr 5, 2023
@kevkevinpal
Copy link
Contributor

utACK by visually looking at the code I can see 3 different options
status abort and start which requires 1 argument

but if it doesn't get any of the above options it calls JSONRPCError link below
Link to blockchain.cpp Line #2243

@ismaelsadeeq
Copy link
Member Author

Thank you @kevkevinpal Yes if it gets an argument that is not status abort or start
blockchain.cpp#L2243 is called.

But
scantxoutsetaction argument is specified as
RPCArg::Optional::NO blockchain.cpp#L2062.
If there are no arguments scantxoutset won't be called at all.
It returns the entire method help string.

That is what 7e3d4f8 is checking for rpc_scantxoutset.py#L124
But with the abbreviation of the method help string
"scantxoutset \"action\" ( [scanobjects,...] )"

@kevkevinpal
Copy link
Contributor

ok I see so this is a scenario where no command is entered. Should there also be a test for when something other than start abort or status is entered?

@ismaelsadeeq
Copy link
Member Author

Yes can be something like
assert_raises_rpc_error(-8, "Invalid action 'word'", self.nodes[0].scantxoutset, "word")

@kevkevinpal
Copy link
Contributor

Yes can be something like assert_raises_rpc_error(-8, "Invalid action 'word'", self.nodes[0].scantxoutset, "word")

opened PR for this #27453
let me know if you have any comments

@fanquake fanquake requested a review from josibake April 17, 2023 09:10
fanquake added a commit to bitcoin-core/gui that referenced this pull request May 2, 2023
24d55fb test: added coverage to rpc_scantxoutset.py (kevkevin)

Pull request description:

  Included a test that checks if an invalid first argument is entered we receive a rpc error. The rpc should fail if "start", "status" or "abort" is not the first command.

  Relavant: mentioned in bitcoin/bitcoin#27422

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 24d55fb
  theStack:
    ACK 24d55fb

Tree-SHA512: 4b804235d3fa17c7bf492068ab47c1f87cb6cfc1a428c51e273ec059d3c41f581bcc467bb5d6d8bbf2fab14c60cd1c52a30c50009efe1c9b5adee70c88897ad9
@fanquake
Copy link
Member

fanquake commented May 4, 2023

cc @theStack @MarcoFalke

@maflcko
Copy link
Member

maflcko commented May 4, 2023

lgtm ACK 7e3d4f8

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 4, 2023
24d55fb test: added coverage to rpc_scantxoutset.py (kevkevin)

Pull request description:

  Included a test that checks if an invalid first argument is entered we receive a rpc error. The rpc should fail if "start", "status" or "abort" is not the first command.

  Relavant: mentioned in bitcoin#27422

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 24d55fb
  theStack:
    ACK 24d55fb

Tree-SHA512: 4b804235d3fa17c7bf492068ab47c1f87cb6cfc1a428c51e273ec059d3c41f581bcc467bb5d6d8bbf2fab14c60cd1c52a30c50009efe1c9b5adee70c88897ad9
@fanquake fanquake merged commit 6c7ebcc into bitcoin:master May 4, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 5, 2023
7e3d4f8 test: add coverage to ensure the first arg of scantxoutset is needed (ismaelsadeeq)

Pull request description:

  Include a test that checks whether the first argument of scantxoutset RPC call "start" is required.
  The rpc call should fail if the "start" argument is not provided.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 7e3d4f8

Tree-SHA512: 6a456af9f3ccd5437be2edcd61936eb9f9c21ab926a6056c2c11b6b5121d1caca4e1f2ffd09015f9414af152c635a20e1da041eefdef980afbe8a0e8ccce07bd
@ismaelsadeeq ismaelsadeeq deleted the 2023-04-test-coverage-rpc_scantxoutset branch June 27, 2024 11:12
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
7e3d4f8 test: add coverage to ensure the first arg of scantxoutset is needed (ismaelsadeeq)

Pull request description:

  Include a test that checks whether the first argument of scantxoutset RPC call "start" is required.
  The rpc call should fail if the "start" argument is not provided.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 7e3d4f8

Tree-SHA512: 6a456af9f3ccd5437be2edcd61936eb9f9c21ab926a6056c2c11b6b5121d1caca4e1f2ffd09015f9414af152c635a20e1da041eefdef980afbe8a0e8ccce07bd
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 25, 2024
7e3d4f8 test: add coverage to ensure the first arg of scantxoutset is needed (ismaelsadeeq)

Pull request description:

  Include a test that checks whether the first argument of scantxoutset RPC call "start" is required.
  The rpc call should fail if the "start" argument is not provided.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 7e3d4f8

Tree-SHA512: 6a456af9f3ccd5437be2edcd61936eb9f9c21ab926a6056c2c11b6b5121d1caca4e1f2ffd09015f9414af152c635a20e1da041eefdef980afbe8a0e8ccce07bd
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 25, 2024
37389c7 Merge bitcoin#28781: depends: latest config.guess & config.sub (fanquake)
3239f15 Merge bitcoin#28479: build: use _LIBCPP_ENABLE_DEBUG_MODE over ENABLE_ASSERTIONS (fanquake)
45cc44b Merge bitcoin#27628: build: Fix shared lib linking for darwin with lld (fanquake)
b8ddcd9 Merge bitcoin#27575: Introduce platform-agnostic `ALWAYS_INLINE` macro (fanquake)
71c6d7f Merge bitcoin#26653: test, init: perturb file to ensure failure instead of only deleting them (fanquake)
417f71a Merge bitcoin#27422: test: add coverage to rpc_scantxoutset.py (fanquake)
898dcbd Merge bitcoin#27559: doc: clarify processing of mempool-msgs when NODE_BLOOM (glozow)
a4e429c Merge bitcoin#26953: contrib: add ELF OS ABI check to symbol-check.py (fanquake)
deb7de2 Merge bitcoin#26604: test: add coverage for `-bantime` (fanquake)
f725ed5 Merge bitcoin#26314: test: perturb anchors.dat to test error during initialization (fanquake)
3306f96 Merge bitcoin#25937: test: add coverage for rpc error when trying to rescan beyond pruned data (fanquake)
712dcaf Merge bitcoin#27516: test: simplify uint256 (de)serialization routines (fanquake)
90d65f2 Merge bitcoin#27508: build: use latest config.{guess,sub} in depends (fanquake)
df7be02 Merge bitcoin#27506: test: prevent intermittent failures (fanquake)
9b58b2d Merge bitcoin#27447: depends: Remove `_LIBCPP_DEBUG` from depends DEBUG mode (fanquake)
07770b7 Merge bitcoin#26741: doc: FreeBSD DataDirectoryGroupReadable Setting (fanquake)
5645362 Merge bitcoin#27362: test: remove `GetRNGState` lsan suppression (fanquake)
671e8e6 Merge bitcoin#27368: refactor: Drop no longer used `CNetMsgMaker` instances (fanquake)
a11690b Merge bitcoin#27301: depends: make fontconfig build under clang-16 (fanquake)
0a94b3f Merge bitcoin#27328: depends: fix osx build with clang 16 (fanquake)

Pull request description:

  ## Issue being fixed or feature implemented
  Batch of trivial backports

  ## What was done?
  See commits

  ## How Has This Been Tested?
  built locally; large combined merge passed tests locally

  ## Breaking Changes
  Should be none

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 37389c7
  kwvg:
    utACK 37389c7

Tree-SHA512: 1ea075a58361f57e037febcf003d380ab845b6c8e1c62d9fcf8954d46cd006046d1951f15a41a5deb9ab7af734df9dafcf89f33d115d78246752f7e2cd13f4ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants