-
Notifications
You must be signed in to change notification settings - Fork 36.6k
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: write functional test results to csv #30291
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
6737ab7
to
7c8ead9
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. Debug: https://github.com/bitcoin/bitcoin/runs/26256129112 |
7c8ead9
to
ee3b5c7
Compare
lgtm ACK ee3b5c7 |
Concept ACK |
Concept ACK This line initially when I read it made me assume that 28 tests failed when in reality 28 passed and one failed Also I think the durration(seconds) here is equal to number of tests for all which seems to be inaccurate too I would expect
|
test/functional/test_runner.py
Outdated
if args.resultsfile: | ||
results_filepath = pathlib.Path(args.resultsfile) | ||
# Stop early if the parent directory doesn't exist | ||
assert results_filepath.parent.exists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be useful to have a log come from this assert
assert results_filepath.parent.exists() | |
assert results_filepath.parent.exists(), "Results file path parent directory does not exist" |
Before:
test/functional/test_runner.py --resultsfile ./fakedir/functional_test_results.csv feature_blocksdir
Temporary test directory at /tmp/test_runner_₿_🏃_20240616_141849
Traceback (most recent call last):
File "/home/kevkevin/DEVDIR/bitcoin/test/functional/test_runner.py", line 939, in <module>
main()
File "/home/kevkevin/DEVDIR/bitcoin/test/functional/test_runner.py", line 481, in main
assert results_filepath.parent.exists()
AssertionError
After:
test/functional/test_runner.py --resultsfile ./fakedir/functional_test_results.csv feature_blocksdir
Temporary test directory at /tmp/test_runner_₿_🏃_20240616_142329
Traceback (most recent call last):
File "/home/kevkevin/DEVDIR/bitcoin/test/functional/test_runner.py", line 939, in <module>
main()
File "/home/kevkevin/DEVDIR/bitcoin/test/functional/test_runner.py", line 481, in main
assert results_filepath.parent.exists(), "Results file path parent directory does not exist"
AssertionError: Results file path parent directory does not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This is an improvement. Included.
Adds argument --resultsfile to test_runner.py. Writes comma-separated functional test name, status, and duration to the file provided with the argument. Also fixes minor typo in test_runner.py
ee3b5c7
to
ad06e68
Compare
Thank you for the review! The approach is:
stdout:
results file:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK ad06e68
Make is successful so are all the functional tests. Passed the resultsfile
param and generated the results csv few times.
Thanks @tdb3 for this, it's a good addition to the testing suite.
I can imagine using this feature and also sorting the tests in descending order based on time taken. I mentioned couple points but none are blockers.
for test_result in test_results: | ||
all_passed = all_passed and test_result.was_successful | ||
results_writer.writerow([test_result.name, test_result.status, str(test_result.time)]) | ||
results_writer.writerow(['ALL', ("Passed" if all_passed else "Failed"), str(total_runtime)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Having had only few tests failed also display ALL,Failed
, which in the first glance gives the impression that all of them failed. In cases like this, displaying ALL, Few failed
would be more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, at first glance showing ALL,Failed
is not ideal, but in this case the CSV output is matching the approach taken by stdout printing, so it seems appropriate for now.
Since the scope of this PR is to focus on adding the CSV output capability, I'll leave it as-is for now. Perhaps a future discussion for how ALL
is treated in both the stdout printing and the CSV could be had?
TEST | STATUS | DURATION
feature_blocksdir.py | ✓ Passed | 1 s
feature_config_args.py | ✓ Passed | 29 s
wallet_startup.py | ✓ Passed | 3 s
mempool_accept.py | ✖ Failed | 1 s
ALL | ✖ Failed | 34 s (accumulated)
Runtime: 29 s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, makes sense to have that discussion separate from this.
@@ -471,6 +474,13 @@ def main(): | |||
|
|||
logging.debug("Temporary test directory at %s" % tmpdir) | |||
|
|||
results_filepath = None | |||
if args.resultsfile: | |||
results_filepath = pathlib.Path(args.resultsfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The write function below write_results
intentionally creates a csv file, should we add a check here for the csv
file extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As soon as the functional_test_results.csv
file was generated in my system, I thought of adding it in gitignore
. Since this feature is conditional based on the param, we can't add this hardcoded name in gitgnore but how about adding *.csv
in gitgnore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thought. Since the approach is to allow the user to specify the filename/extension as they see fit (rather than force a particular extension), this seems ok as-is. If others feel that forcing the extension .csv
would be more ideal, then will keep this in mind.
re-ACK ad06e68 |
@@ -702,6 +716,17 @@ def print_results(test_results, max_len_name, runtime): | |||
results += "Runtime: %s s\n" % (runtime) | |||
print(results) | |||
|
|||
|
|||
def write_results(test_results, filepath, total_runtime): | |||
with open(filepath, mode="w", encoding="utf8") as results_file: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It creates the file if it doesn't exist, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and overwrites the existing file if it does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK ad06e68
csv output
test,status,duration(seconds) example_test.py,Passed,4 feature_abortnode.py,Passed,2 feature_addrman.py,Passed,5 feature_anchors.py,Passed,4 feature_asmap.py,Passed,8 feature_assumeutxo.py,Passed,45 feature_assumevalid.py,Passed,4 feature_bip68_sequence.py,Passed,7 feature_block.py,Passed,45 feature_blocksdir.py,Passed,1 feature_cltv.py,Passed,2 feature_coinstatsindex.py,Passed,7 feature_config_args.py,Passed,28 feature_csv_activation.py,Passed,18 feature_dersig.py,Passed,1 feature_dirsymlinks.py,Passed,1 feature_discover.py,Passed,3 feature_fastprune.py,Passed,1 feature_fee_estimation.py,Passed,18 feature_filelock.py,Passed,2 feature_framework_miniwallet.py,Passed,5 feature_framework_unit_tests.py,Passed,6 feature_help.py,Passed,1 feature_includeconf.py,Passed,4 feature_init.py,Passed,17 feature_loadblock.py,Passed,3 feature_logging.py,Passed,5 feature_maxtipage.py,Passed,9 feature_maxuploadtarget.py,Passed,64 feature_minchainwork.py,Passed,13 feature_notifications.py,Passed,12 feature_nulldummy.py,Passed,1 feature_posix_fs_permissions.py,Passed,1 feature_presegwit_node_upgrade.py,Passed,1 feature_proxy.py,Passed,4 feature_rbf.py,Passed,5 feature_reindex.py,Passed,6 feature_reindex_readonly.py,Passed,1 feature_remove_pruned_files_on_startup.py,Passed,2 feature_segwit.py --descriptors --v1transport,Passed,20 feature_segwit.py --descriptors --v2transport,Passed,12 feature_segwit.py --legacy-wallet,Passed,38 feature_settings.py,Passed,3 feature_shutdown.py,Passed,1 feature_signet.py,Passed,3 feature_startupnotify.py,Passed,1 feature_taproot.py,Passed,31 feature_uacomment.py,Passed,3 feature_utxo_set_hash.py,Passed,1 feature_versionbits_warning.py,Passed,3 interface_bitcoin_cli.py --descriptors,Passed,16 interface_bitcoin_cli.py --legacy-wallet,Passed,18 interface_http.py,Passed,1 interface_rest.py,Passed,4 interface_rpc.py,Passed,2 mempool_accept.py,Passed,2 mempool_accept_v3.py,Passed,8 mempool_accept_wtxid.py,Passed,3 mempool_datacarrier.py,Passed,1 mempool_dust.py,Passed,5 mempool_expiry.py,Passed,1 mempool_limit.py,Passed,16 mempool_package_limits.py,Passed,2 mempool_package_onemore.py,Passed,1 mempool_packages.py,Passed,4 mempool_persist.py --descriptors,Passed,17 mempool_reorg.py,Passed,5 mempool_resurrect.py,Passed,2 mempool_sigoplimit.py,Passed,4 mempool_spend_coinbase.py,Passed,1 mempool_unbroadcast.py,Passed,9 mempool_updatefromblock.py,Passed,7 mining_basic.py,Passed,15 mining_getblocktemplate_longpoll.py,Passed,68 mining_prioritisetransaction.py,Passed,2 p2p_1p1c_network.py,Passed,14 p2p_add_connections.py,Passed,9 p2p_addr_relay.py,Passed,16 p2p_addrfetch.py,Passed,1 p2p_addrv2_relay.py,Passed,2 p2p_block_sync.py --v1transport,Passed,2 p2p_block_sync.py --v2transport,Passed,2 p2p_blockfilters.py,Passed,9 p2p_blocksonly.py,Passed,10 p2p_compactblocks.py,Passed,13 p2p_compactblocks_blocksonly.py,Passed,3 p2p_compactblocks_hb.py --v1transport,Passed,23 p2p_compactblocks_hb.py --v2transport,Passed,23 p2p_disconnect_ban.py --v1transport,Passed,4 p2p_disconnect_ban.py --v2transport,Passed,4 p2p_dns_seeds.py,Passed,28 p2p_dos_header_tree.py,Passed,3 p2p_eviction.py,Passed,5 p2p_feefilter.py,Passed,4 p2p_filter.py,Passed,4 p2p_fingerprint.py,Passed,2 p2p_getaddr_caching.py,Passed,4 p2p_getdata.py,Passed,1 p2p_handshake.py,Passed,4 p2p_handshake.py --v2transport,Passed,4 p2p_headers_sync_with_minchainwork.py,Passed,12 p2p_i2p_ports.py,Passed,1 p2p_i2p_sessions.py,Passed,1 p2p_ibd_stalling.py --v1transport,Passed,4 p2p_ibd_stalling.py --v2transport,Passed,5 p2p_ibd_txrelay.py,Passed,3 p2p_initial_headers_sync.py,Passed,2 p2p_invalid_block.py --v1transport,Passed,2 p2p_invalid_block.py --v2transport,Passed,2 p2p_invalid_locator.py,Passed,2 p2p_invalid_messages.py,Passed,11 p2p_invalid_tx.py --v1transport,Passed,7 p2p_invalid_tx.py --v2transport,Passed,8 p2p_leak.py,Passed,6 p2p_leak_tx.py --v1transport,Passed,9 p2p_leak_tx.py --v2transport,Passed,18 p2p_message_capture.py,Passed,1 p2p_mutated_blocks.py,Passed,2 p2p_net_deadlock.py --v1transport,Passed,2 p2p_net_deadlock.py --v2transport,Passed,2 p2p_nobloomfilter_messages.py,Passed,2 p2p_node_network_limited.py --v1transport,Passed,12 p2p_node_network_limited.py --v2transport,Passed,12 p2p_opportunistic_1p1c.py,Passed,45 p2p_orphan_handling.py,Passed,10 p2p_outbound_eviction.py,Passed,6 p2p_permissions.py,Passed,14 p2p_ping.py,Passed,1 p2p_segwit.py,Passed,62 p2p_sendheaders.py,Passed,18 p2p_sendtxrcncl.py,Passed,6 p2p_timeouts.py --v1transport,Passed,1 p2p_timeouts.py --v2transport,Passed,2 p2p_tx_download.py,Passed,49 p2p_tx_privacy.py,Passed,5 p2p_unrequested_blocks.py,Passed,8 p2p_v2_earlykeyresponse.py,Passed,4 p2p_v2_encrypted.py,Passed,6 p2p_v2_transport.py,Passed,7 rpc_blockchain.py --v1transport,Passed,42 rpc_blockchain.py --v2transport,Passed,44 rpc_createmultisig.py,Passed,3 rpc_decodescript.py,Passed,1 rpc_deprecated.py,Passed,2 rpc_deriveaddresses.py,Passed,1 rpc_deriveaddresses.py --usecli,Passed,1 rpc_dumptxoutset.py,Passed,1 rpc_estimatefee.py,Passed,1 rpc_generate.py,Passed,1 rpc_getblockfilter.py,Passed,1 rpc_getblockfrompeer.py,Passed,5 rpc_getblockstats.py,Passed,1 rpc_getchaintips.py,Passed,5 rpc_getdescriptorinfo.py,Passed,1 rpc_help.py,Passed,1 rpc_invalid_address_message.py,Passed,1 rpc_invalidateblock.py,Passed,1 rpc_mempool_info.py,Passed,1 rpc_misc.py,Passed,3 rpc_named_arguments.py,Passed,1 rpc_net.py --v1transport,Passed,10 rpc_net.py --v2transport,Passed,6 rpc_packages.py,Passed,4 rpc_preciousblock.py,Passed,2 rpc_psbt.py --descriptors,Passed,23 rpc_psbt.py --legacy-wallet,Passed,37 rpc_rawtransaction.py --legacy-wallet,Passed,12 rpc_scanblocks.py,Passed,2 rpc_scantxoutset.py,Passed,3 rpc_setban.py --v1transport,Passed,2 rpc_setban.py --v2transport,Passed,4 rpc_signer.py,Passed,2 rpc_signmessagewithprivkey.py,Passed,1 rpc_signrawtransactionwithkey.py,Passed,2 rpc_txoutproof.py,Passed,3 rpc_uptime.py,Passed,1 rpc_users.py,Passed,7 rpc_validateaddress.py,Passed,1 rpc_whitelist.py,Passed,1 tool_signet_miner.py --descriptors,Passed,3 tool_signet_miner.py --legacy-wallet,Passed,4 tool_wallet.py --descriptors,Passed,78 tool_wallet.py --legacy-wallet,Passed,86 tool_wallet.py --legacy-wallet --bdbro,Passed,92 tool_wallet.py --legacy-wallet --bdbro --swap-bdb-endian,Passed,94 wallet_abandonconflict.py --descriptors,Passed,6 wallet_abandonconflict.py --legacy-wallet,Passed,8 wallet_address_types.py --descriptors,Passed,19 wallet_address_types.py --legacy-wallet,Passed,46 wallet_assumeutxo.py --descriptors,Passed,11 wallet_avoid_mixing_output_types.py --descriptors,Passed,2 wallet_avoidreuse.py --descriptors,Passed,26 wallet_avoidreuse.py --legacy-wallet,Passed,60 wallet_backup.py --descriptors,Passed,34 wallet_backup.py --legacy-wallet,Passed,69 wallet_balance.py --descriptors,Passed,7 wallet_balance.py --legacy-wallet,Passed,7 wallet_basic.py --descriptors,Passed,32 wallet_basic.py --legacy-wallet,Passed,43 wallet_blank.py --descriptors,Passed,1 wallet_blank.py --legacy-wallet,Passed,4 wallet_bumpfee.py --descriptors,Passed,52 wallet_bumpfee.py --legacy-wallet,Passed,60 wallet_change_address.py --descriptors,Passed,15 wallet_change_address.py --legacy-wallet,Passed,29 wallet_coinbase_category.py --descriptors,Passed,3 wallet_coinbase_category.py --legacy-wallet,Passed,3 wallet_conflicts.py --descriptors,Passed,50 wallet_conflicts.py --legacy-wallet,Passed,44 wallet_create_tx.py --descriptors,Passed,14 wallet_create_tx.py --legacy-wallet,Passed,48 wallet_createwallet.py --descriptors,Passed,3 wallet_createwallet.py --legacy-wallet,Passed,6 wallet_createwallet.py --usecli,Passed,5 wallet_createwalletdescriptor.py --descriptors,Passed,2 wallet_crosschain.py,Passed,1 wallet_descriptor.py --descriptors,Passed,3 wallet_disable.py,Passed,1 wallet_dump.py --legacy-wallet,Passed,14 wallet_encryption.py --descriptors,Passed,6 wallet_encryption.py --legacy-wallet,Passed,6 wallet_fallbackfee.py --descriptors,Passed,2 wallet_fallbackfee.py --legacy-wallet,Passed,2 wallet_fast_rescan.py --descriptors,Passed,11 wallet_fundrawtransaction.py --descriptors,Passed,51 wallet_fundrawtransaction.py --legacy-wallet,Passed,132 wallet_gethdkeys.py --descriptors,Passed,2 wallet_groups.py --descriptors,Passed,14 wallet_groups.py --legacy-wallet,Passed,15 wallet_hd.py --descriptors,Passed,10 wallet_hd.py --legacy-wallet,Passed,18 wallet_implicitsegwit.py --legacy-wallet,Passed,7 wallet_import_rescan.py --legacy-wallet,Passed,128 wallet_import_with_label.py --legacy-wallet,Passed,2 wallet_importdescriptors.py --descriptors,Passed,15 wallet_importmulti.py --legacy-wallet,Passed,15 wallet_importprunedfunds.py --descriptors,Passed,2 wallet_importprunedfunds.py --legacy-wallet,Passed,3 wallet_keypool.py --descriptors,Passed,3 wallet_keypool.py --legacy-wallet,Passed,4 wallet_keypool_topup.py --descriptors,Passed,15 wallet_keypool_topup.py --legacy-wallet,Passed,47 wallet_labels.py --descriptors,Passed,7 wallet_labels.py --legacy-wallet,Passed,10 wallet_listdescriptors.py --descriptors,Passed,1 wallet_listreceivedby.py --descriptors,Passed,8 wallet_listreceivedby.py --legacy-wallet,Passed,9 wallet_listsinceblock.py --descriptors,Passed,12 wallet_listsinceblock.py --legacy-wallet,Passed,10 wallet_listtransactions.py --descriptors,Passed,13 wallet_listtransactions.py --legacy-wallet,Passed,16 wallet_migration.py,Passed,21 wallet_miniscript.py --descriptors,Passed,12 wallet_multisig_descriptor_psbt.py --descriptors,Passed,3 wallet_multiwallet.py --descriptors,Passed,14 wallet_multiwallet.py --legacy-wallet,Passed,27 wallet_multiwallet.py --usecli,Passed,16 wallet_orphanedreward.py,Passed,11 wallet_reindex.py --descriptors,Passed,2 wallet_reindex.py --legacy-wallet,Passed,4 wallet_reorgsrestore.py,Passed,5 wallet_rescan_unconfirmed.py --descriptors,Passed,2 wallet_resendwallettransactions.py --descriptors,Passed,10 wallet_resendwallettransactions.py --legacy-wallet,Passed,5 wallet_send.py --descriptors,Passed,30 wallet_send.py --legacy-wallet,Passed,38 wallet_sendall.py --descriptors,Passed,33 wallet_sendall.py --legacy-wallet,Passed,101 wallet_sendmany.py --descriptors,Passed,4 wallet_sendmany.py --legacy-wallet,Passed,6 wallet_signer.py --descriptors,Passed,7 wallet_signmessagewithaddress.py,Passed,1 wallet_signrawtransactionwithwallet.py --descriptors,Passed,3 wallet_signrawtransactionwithwallet.py --legacy-wallet,Passed,4 wallet_simulaterawtx.py --descriptors,Passed,1 wallet_simulaterawtx.py --legacy-wallet,Passed,2 wallet_spend_unconfirmed.py,Passed,9 wallet_startup.py,Passed,2 wallet_taproot.py --descriptors,Passed,14 wallet_timelock.py,Passed,1 wallet_transactiontime_rescan.py --descriptors,Passed,4 wallet_txn_clone.py,Passed,5 wallet_txn_clone.py --mineblock,Passed,4 wallet_txn_clone.py --segwit,Passed,2 wallet_txn_doublespend.py --descriptors,Passed,2 wallet_txn_doublespend.py --legacy-wallet,Passed,4 wallet_txn_doublespend.py --mineblock,Passed,7 wallet_watchonly.py --legacy-wallet,Passed,3 wallet_watchonly.py --usecli --legacy-wallet,Passed,3 feature_bind_extra.py,Skipped,0 feature_bind_port_discover.py,Skipped,0 feature_bind_port_externalip.py,Skipped,1 feature_unsupported_utxo_db.py,Skipped,0 interface_usdt_coinselection.py,Skipped,0 interface_usdt_mempool.py,Skipped,0 interface_usdt_net.py,Skipped,0 interface_usdt_utxocache.py,Skipped,0 interface_usdt_validation.py,Skipped,0 interface_zmq.py,Skipped,0 mempool_compatibility.py,Skipped,0 rpc_bind.py --ipv4,Skipped,0 rpc_bind.py --ipv6,Skipped,1 rpc_bind.py --nonloopback,Skipped,1 wallet_backwards_compatibility.py --descriptors,Skipped,1 wallet_backwards_compatibility.py --legacy-wallet,Skipped,1 wallet_inactive_hdchains.py --legacy-wallet,Skipped,0 wallet_upgradewallet.py --legacy-wallet,Skipped,0 wallet_transactiontime_rescan.py --legacy-wallet,Failed,131 ALL,Failed,971
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, tested ACK ad06e68
Thanks for reviewing/testing @marcofleon! |
tACK ad06e68 Looks good to me! |
ACK ad06e68 |
Adds argument
--resultsfile
to test_runner.py.Enables functional test results to be written to a (csv) file for processing by other applications (or for historical archiving).
Test name, status, and duration are written to the file provided with the argument.
Since
test_runner.py
is being touched, also fixes a misspelling (linter warning). Can split into its own commit if desired.Notes
doc/benchmarking.md
, existing PRs, and Issues, but didn't immediately see this type of capability or alternate solutions (please chime in if you know of one!). Thought it would be beneficial to add this capability totest_runner
to facilitate this type of data analysis (and potentially other use cases)print_results()
(i.e. take advantage of the same loop overtest_results
) or implement in a separatewrite_results()
function. Decided on the latter for now, but interested in reviewers' thoughts.Example 1: all tests pass
Example 2: one test failure