test: create assert_not_equal util #10
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevkevinpal
force-pushed
the
testFrameworkUtilsOne
branch
29 times, most recently
from
6c6f7ea to
c430e9d
Compare
kevkevinpal
force-pushed
the
testFrameworkUtilsOne
branch
from
c430e9d to
1c34f15
Compare
The only coverage of combinerawtransaction is in a legacy wallet only test. So also use it in rpc_createmultisig so that this RPC remains tested after the legacy wallet is removed.
Wastes less resources and notifies the developer faster when a failure occurs.
47f50c7 doc: add bitcoin-qt man description (willcl-ark) 40b82e3 doc: add bitcoin-util man description (willcl-ark) a7bf80f doc: add bitcoin-tx man description (willcl-ark) 3f9a516 doc: add bitcoin-wallet man description (willcl-ark) d8c0bb2 doc: add bitcoin-cli man description (willcl-ark) 09abccf doc: add bitcoind man description (willcl-ark) Pull request description: Closes bitcoin#29552 Add better descriptions to help string for all binaries. Use format which is correctly detected by help2man when generating manpages. Examples: Before:  After:  Demonstration using `bitcoin-cli` also highlights removal of inline usage explanations which were being incorrectly formatted by `help2man`. This results in the following changed format to `bitcoin-cli --help`:  ACKs for top commit: achow101: ACK 47f50c7 tdb3: re ACK 47f50c7 rkrux: tACK 47f50c7 maflcko: ACK 47f50c7 📠 Tree-SHA512: 124a8877077b7d47758ea970949d472b2444e3ba65d2bfeb47ebbdb1f5f8d3bf0abe2c88714bb6c92ba0e36583f0b36aa6f016ea88b65f011c610096ea872182
c189eec doc: release note for mempoolrullrbf removal (Greg Sanders) d47297c rpc: Mark fullrbf and bip125-replaceable as deprecated (Greg Sanders) 04a5dce docs: remove requirement to signal bip125 (Greg Sanders) 111a23d Remove -mempoolfullrbf option (Greg Sanders) Pull request description: Given bitcoin#30493 and the related discussion on network uptake it's probably not helpful to have an option for a feature that will not be respected by the network in any meaningful way. Wallet changes can be done in another PR on its own cadence to account for possible fingerprinting, waiting for fullrbf logic to permeate the network, etc. ACKs for top commit: stickies-v: re-ACK c189eec achow101: ACK c189eec murchandamus: ACK c189eec rkrux: reACK c189eec Tree-SHA512: 9447f88f8f291c56c5bde70af0a91b0a4f5163aaaf173370fbfdaa3c3fd0b44120b14d3a1977f7ee10e27ffe9453f8a70dd38aad0ffb8c39cf145049d2550730
This pull request addresses a minor issues in the REST-interface.md documentation: Missing Comma in JSON Example: In the "Query UTXO set" section, a missing comma after the "desc" field in the JSON example has been added to ensure valid JSON syntax.
…actually tracing 0de3e96 tracing: use bitcoind pid in bcc tracing examples (0xb10c) 411c6cf tracing: only prepare tracepoint args if attached (0xb10c) d524c1e tracing: dedup TRACE macros & rename to TRACEPOINT (0xb10c) Pull request description: Currently, if the tracepoints are compiled (e.g. in depends and release builds), we always prepare the tracepoint arguments regardless of the tracepoints being used or not. We made sure that the argument preparation is as cheap as possible, but we can almost completely eliminate any overhead for users not interested in the tracepoints (the vast majority), by gating the tracepoint argument preparation with an `if(something is attached to this tracepoint)`. To achieve this, we use the optional semaphore feature provided by SystemTap. The first commit simplifies and deduplicates our tracepoint macros from 13 TRACEx macros to a single TRACEPOINT macro. This makes them easier to use and also avoids more duplicate macro definitions in the second commit. The Linux tracing tools I'm aware of (bcc, bpftrace, libbpf, and systemtap) all support the semaphore gating feature. Thus, all existing tracepoints and their argument preparation is gated in the second commit. For details, please refer to the commit messages and the updated documentation in `doc/tracing.md`. Also adding unit tests that include all tracepoint macros to make sure there are no compiler problems with them (e.g. some varadiac extension not supported). Reviewers might want to check: - Do the tracepoints still work for you? Do the examples in `contrib/tracing/` run on your system (as bpftrace frequently breaks on every new version, please test master too if it should't work for you)? Do the CI interface tests still pass? - Is the new documentation clear? - The `TRACEPOINT_SEMAPHORE(event, context)` macros places global variables in our global namespace. Is this something we strictly want to avoid or maybe move to all `TRACEPOINT_SEMAPHORE`s to a separate .cpp file or even namespace? I like having the `TRACEPOINT_SEMAPHORE()` in same file as the `TRACEPOINT()`, but open for suggestion on alternative approaches. - Are newly added tracepoints in the unit tests visible when using `readelf -n build/src/test/test_bitcoin`? You can run the new unit tests with `./build/src/test/test_bitcoin --run_test=util_trace_tests* --log_level=all`. <details><summary>Two of the added unit tests demonstrate that we are only processing the tracepoint arguments when attached by having a test-failure condition in the tracepoint argument preparation. The following bpftrace script can be used to demonstrate that the tests do indeed fail when attached to the tracepoints.</summary> `fail_tests.bt`: ```c #!/usr/bin/env bpftrace usdt:./build/src/test/test_bitcoin:test:check_if_attached { printf("the 'check_if_attached' test should have failed\n"); } usdt:./build/src/test/test_bitcoin:test:expensive_section { printf("the 'expensive_section' test should have failed\n"); } ``` Run the unit tests with `./build/src/test/test_bitcoin` and start `bpftrace fail_tests.bt -p $(pidof test_bitcoin)` in a separate terminal. The unit tests should fail with: ``` Running 594 test cases... test/util_trace_tests.cpp(31): error: in "util_trace_tests/test_tracepoint_check_if_attached": check false has failed test/util_trace_tests.cpp(51): error: in "util_trace_tests/test_tracepoint_manual_tracepoint_active_check": check false has failed *** 2 failures are detected in the test module "Bitcoin Core Test Suite" ``` </details> These links might provide more contextual information for reviewers: - [How SystemTap Userspace Probes Work by eklitzke](https://eklitzke.org/how-sytemtap-userspace-probes-work) (actually an example on Bitcoin Core; mentions that with semaphores "the overhead for an untraced process is effectively zero.") - [libbpf comment on USDT semaphore handling](https://github.com/libbpf/libbpf/blob/1596a09b5de2a50ab8d44218fc29b6d42f886305/src/usdt.c#L83-L92) (can recommend the whole comment for background on how the tracepoints and tracing tools work together) - https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation#Semaphore_Handling ACKs for top commit: willcl-ark: utACK 0de3e96 laanwj: re-ACK 0de3e96 jb55: utACK 0de3e96 vasild: ACK 0de3e96 Tree-SHA512: 0e5e0dc5e0353beaf5c446e4be03d447e64228b1be71ee9972fde1d6fac3fac71a9d73c6ce4fa68975f87db2b2bf6eee2009921a2a145e24d83a475d007a559b
36a22e5 ci: make ctest stop on failure (furszy) Pull request description: Make `ctest` stops when the first failure happens. Wasting less resources and notifying the developer faster when a failure occurs. ACKs for top commit: maflcko: lgtm ACK 36a22e5 tdb3: code review and test ACK 36a22e5 Tree-SHA512: 3abdb330e76aa312f7a5432e3d447a654e6689fc56e067b8e4d07ed8d677fc92f836e603aab0b2f175a6c039a5d50e5fd1160d503164321c1af44ad902f09605
…rminology in protocol.{h,cpp}
4120c75 scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp} (Sebastian Falbesoner)
Pull request description:
The confusing "command" terminology for the 12-byte field in the (v1) p2p message header was replaced with the more proper term "message type" in other modules already years ago, see eg bitcoin#18533, bitcoin#18937, bitcoin#24078, bitcoin#24141. This PR does the same for the protocol.{h,cpp} module to complete the replacements. Note that "GetCommand" is a method name also used in the `ArgsManager` (there it makes much more sense), so the scripted-diff lists for this replacement the files explicitly, rather than using `$(git grep -l ...)`.
ACKs for top commit:
maflcko:
review ACK 4120c75 🛒
fjahr:
Code review ACK 4120c75
rkrux:
tACK 4120c75
Tree-SHA512: 7b4dd30136392a145da95d2f3ba181c18c155ba6f3158e49e622d76811c6a45ef9b5c7539a979a04d8404faf18bb27f11457aa436d4e2998ece3deb2c9e59748
…n args fa729ab doc: Fixup bitcoin-wallet manpage chain selection args (MarcoFalke) Pull request description: The sentence is missing `-testnet4` and `-chain`. Instead of duplicating the full list (and having to keep it in sync), just refer to them as `(test)chain selection arguments`. ACKs for top commit: willcl-ark: utACK fa729ab tdb3: Code Review ACK fa729ab rkrux: crACK fa729ab Tree-SHA512: e2cb6e2dd778a34e6c7e8ccde9794ab601e68bad68fe110f41cd73ac12ac3c5d0632fb59a48355f03ef0909f77ec5afd7ea50f301a998cb3ec76e115969f3e7e
…nterface.md 5e3b444 doc: Fix missing comma in JSON example in REST-interface.md (secp512k2) Pull request description: This pull request addresses a minor issues in the REST-interface.md documentation: Missing Comma in JSON Example: In the "Query UTXO set" section, a missing comma after the "desc" field in the JSON example has been added to ensure valid JSON syntax. ACKs for top commit: maflcko: lgtm ACK 5e3b444 Abdulkbk: ACK 5e3b444 Tree-SHA512: d2d479c8a991d3380d16b7b140a375a90dca0fce0a024a4b8ccf842d703398fde14ae972349f5fbd2e0ce26aa6cd6d07c0262d9c09ddc4c6c466527cfbe0e1f1
…proxy connections 99d9a09 test: clarify log messages when handling SOCKS5 proxy connections (Vasil Dimov) Pull request description: Clarify log messages when handling SOCKS5 proxy connections. Suggested in bitcoin#29420 (comment) ACKs for top commit: mzumsande: Code Review ACK 99d9a09 tdb3: code review ACK 99d9a09 Tree-SHA512: 06bc0e63fbc9fdd8144a161d65d02e6c99565960064e65782b9b4b2fdfdf18539a1cd9513e17a911eef1506525e411e8422b7b805ce4c2392fcca6620112e172
…signing-tutorial.md ec375de doc: Add missing 'blank=true' option in offline-signing-tutorial.md (secp512k2) Pull request description: Issue: The text mentions that the `createwallet` command should use the options `disable_private_keys=true, blank=true`, but the provided command only includes `disable_private_keys=true`, missing the `blank=true` option. Correction: Added `blank=true` to the command to match the options described in the text. Explanation: The `blank=true` option is necessary to create a blank wallet. Including this option ensures the command matches the options specified in the text. ACKs for top commit: fanquake: ACK ec375de Tree-SHA512: 8c145e3ef1598c5e13f2aa89e496f76bfe2fc6f47d5e740963acad18dd1f782655a822dc234862af8e5a08060ab7fe1039a3dcfa68455a9143fe2d849975927c
Fix typos in miniscript.h
faf2162 refactor: Drop deprecated space in operator""_mst (MarcoFalke) Pull request description: The space is deprecated according to https://en.cppreference.com/w/cpp/language/user_literal#Literal_operators and compilers will start to warn about this. For example, GCC-15 should warn when compiling under C++23, according to https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html#index-Wdeprecated-literal-operator and Clang-20 will do so by default, according to https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated-literal-operator. Fix it by removing the unused and deprecated space. Also, fix the iwyu include list, while touching the module. ACKs for top commit: TheCharlatan: ACK faf2162 l0rinc: ACK faf2162 Tree-SHA512: 888a7b57c91114e1f71b6278fa13783bde16a9b51f4df10ae4b6c7d62bf016d6c022128250e6962b459f66743ddeab774b4109064281654892ecdb5bc11b6be6
5a96767 depends, libevent: Do not install *.pc files and remove patches for them (Hennadii Stepanov) ffda355 cmake, refactor: Move `HAVE_EVHTTP_...` to `libevent` interface (Hennadii Stepanov) b619bdc cmake: Revamp `FindLibevent` module (Hennadii Stepanov) Pull request description: This PR generalizes the use of `find_package` / `pkg_check_modules`, prioritizing the former. Addresses bitcoin#30903 (comment): > We should also follow up with refactoring the libevent module, to more generically use CMake/pkg-config, rather than restricting the CMake usage to `vcpkg`. At that point, we'd likely be able to dump pkg-config for the depends path entirely. Similar to bitcoin#30903. ACKs for top commit: fanquake: ACK 5a96767 Tree-SHA512: 181020c16ccd2821e718c73f264badcdc5e62980c4a8d9691e759efe2ea00da2326e26308d1dcfdeac01e9e27930428ecace9f36941deee951b751b138d7266c
726cbee doc: correct typos (fanquake) 9fdfb73 doc: fix typos (Afanti) Pull request description: Includes bitcoin#31253. Includes bitcoin#31239 (review). Fixes remaining lint output. ACKs for top commit: l0rinc: ACK 726cbee rkrux: crACK 726cbee tdb3: ACK 726cbee Tree-SHA512: 51978343f11fb5f0c6b824d92dbfc9999952373a9f790ab79ef8750f920f1c020c092ca874c9e39f478d12d85cdadcfd8c63dda0cbb02745bc55fda28d371e4c
…temultisig 83fab32 test: Add combinerawtransaction test to rpc_createmultisig (Ava Chow) Pull request description: The only coverage of combinerawtransaction is in a legacy wallet only test. So also use it in rpc_createmultisig so that this RPC remains tested after the legacy wallet is removed. Split from bitcoin#28710 ACKs for top commit: maflcko: re-ACK 83fab32 BrandonOdiwuor: Re-ACK 83fab32 Abdulkbk: ACK 83fab32 brunoerg: code review ACK 83fab32 rkrux: tACK 83fab32 Tree-SHA512: 383d88ff6c9b54337ed81c714026e527b0fed41d976959fd5c6863b49d0defa4ea13fdc3d984885c86a2b6380825cd66c17842cc31f20fbec4bc42d86aecbbfa
9de9c85 test: enhance p2p_orphan_handling (tdb3) 33af14b test: reduce assert_debug_log reliance (tdb3) Pull request description: Previously, `p2p_orphan_handling` relied on checking the debug log for orphanage changes. This updates the tests to reduce debug log checking and add checks using `tx_in_orphanage()` and `getorphantxs` introduced in bitcoin#30793. ACKs for top commit: glozow: light code review ACK 9de9c85 rkrux: tACK 9de9c85 danielabrozzoni: ACK 9de9c85 Tree-SHA512: b53bf0d66d727c79eab972b736a074bd04ca652afd89d2a50830247f42734c61c4c2fa883fde179560e39469c81d0e7be478e1faa0992d3688d5e04d75c067d7
kevkevinpal
force-pushed
the
testFrameworkUtilsOne
branch
5 times, most recently
from
37eaab0 to
4756288
Compare
In the functional tests there are lots of cases where we assert != which this new util will replace, we also are adding the imports and the following commit will add the new assertion
kevkevinpal
force-pushed
the
testFrameworkUtilsOne
branch
from
4756288 to
6b3260e
Compare
… assert_not_equal -BEGIN VERIFY SCRIPT- sed -i "s/assert sha256sum_file(dump_output\['path'\]) != sha256sum_file(dump_output4\['path'\])/assert_not_equal(sha256sum_file(dump_output['path']), sha256sum_file(dump_output4['path']))/g" ./test/functional/feature_assumeutxo.py git grep -l "assert .*!=" -- ':(exclude)*script.py' ./test/functional | xargs sed -i -e "/assert (.*!=.*)/ s/assert (/assert_not_equal(/g" -e "/assert (.*) !=/ s/assert /assert_not_equal(/g" -e "/assert [^(].*!=/ s/assert /assert_not_equal(/g" -e "/assert_not_equal(/ s/ !=/,/g" -e "/assert_not_equal(.*#/ s/ #/) #/g" -e "/assert_not_equal([^#]*.[^)]$/ s/$/)/g" -e "/assert_not_equal([^#]*.[(][)][)]$/ s/$/)/g" -e "/assert_not_equal([^#]*.[(][)]$/ s/$/)/g" -e "/assert_not_equal([^#]*([a-zA-Z=_]*)$/ s/$/)/g" -e "s/assert_not_equal(cblks/assert_not_equal((cblks/g" -e "/assert_not_equal((cblks/ s/)$/))/g" -e "s/assert_not_equal(amount/assert_not_equal((amount/g" -e "/assert_not_equal((amount/ s/None)$/None))/g" -END VERIFY SCRIPT-
kevkevinpal
force-pushed
the
testFrameworkUtilsOne
branch
from
6b3260e to
dcf9a45
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the functional tests there are lots of cases where we assert != which we now swap with assert_not_equal to be more readable