test: Move `script_assets_tests` into its own suite #31576

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:241227-test-assets changing 3 files +181 −141

hebasto commented at 7:28 PM on December 27, 2024: member

This PR ensures that the script_assets_tests test case is explicitly reported as "Skipped" when it is not run, making it clearer when running the test suite with ctest:

on the master branch @ 9355578a77978a0c2f189bd7315a2883142d8119:

$ env -u DIR_UNIT_TEST_DATA ctest --test-dir build -j 16 -R "^script_"
Internal ctest changing into directory: /home/hebasto/git/bitcoin/build
Test project /home/hebasto/git/bitcoin/build
    Start 87: script_tests
    Start 83: script_p2sh_tests
    Start 85: script_segwit_tests
    Start 86: script_standard_tests
    Start 84: script_parse_tests
1/5 Test [#84](/bitcoin-bitcoin/84/): script_parse_tests ...............   Passed    0.11 sec
2/5 Test [#86](/bitcoin-bitcoin/86/): script_standard_tests ............   Passed    0.11 sec
3/5 Test [#85](/bitcoin-bitcoin/85/): script_segwit_tests ..............   Passed    0.12 sec
4/5 Test [#83](/bitcoin-bitcoin/83/): script_p2sh_tests ................   Passed    0.12 sec
5/5 Test [#87](/bitcoin-bitcoin/87/): script_tests .....................   Passed    0.36 sec

100% tests passed, 0 tests failed out of 5

Total Test time (real) =   0.37 sec

with this PR:

$ env -u DIR_UNIT_TEST_DATA ctest --test-dir build -j 16 -R "^script_"
Internal ctest changing into directory: /home/hebasto/git/bitcoin/build
Test project /home/hebasto/git/bitcoin/build
    Start 83: script_assets_tests
    Start 88: script_tests
    Start 84: script_p2sh_tests
    Start 86: script_segwit_tests
    Start 87: script_standard_tests
    Start 85: script_parse_tests
1/6 Test [#85](/bitcoin-bitcoin/85/): script_parse_tests ...............   Passed    0.11 sec
2/6 Test [#83](/bitcoin-bitcoin/83/): script_assets_tests ..............***Skipped   0.12 sec
3/6 Test [#86](/bitcoin-bitcoin/86/): script_segwit_tests ..............   Passed    0.11 sec
4/6 Test [#87](/bitcoin-bitcoin/87/): script_standard_tests ............   Passed    0.11 sec
5/6 Test [#84](/bitcoin-bitcoin/84/): script_p2sh_tests ................   Passed    0.12 sec
6/6 Test [#88](/bitcoin-bitcoin/88/): script_tests .....................   Passed    0.36 sec

100% tests passed, 0 tests failed out of 6

Total Test time (real) =   0.37 sec

The following tests did not run:
	 83 - script_assets_tests (Skipped)
$ env DIR_UNIT_TEST_DATA=/home/hebasto/git/bitcoin/qa-assets/unit_test_data ctest --test-dir build -j 16 -R "^script_"
Internal ctest changing into directory: /home/hebasto/git/bitcoin/build
Test project /home/hebasto/git/bitcoin/build
    Start 83: script_assets_tests
    Start 88: script_tests
    Start 84: script_p2sh_tests
    Start 86: script_segwit_tests
    Start 87: script_standard_tests
    Start 85: script_parse_tests
1/6 Test [#85](/bitcoin-bitcoin/85/): script_parse_tests ...............   Passed    0.11 sec
2/6 Test [#87](/bitcoin-bitcoin/87/): script_standard_tests ............   Passed    0.11 sec
3/6 Test [#86](/bitcoin-bitcoin/86/): script_segwit_tests ..............   Passed    0.11 sec
4/6 Test [#84](/bitcoin-bitcoin/84/): script_p2sh_tests ................   Passed    0.12 sec
5/6 Test [#88](/bitcoin-bitcoin/88/): script_tests .....................   Passed    0.35 sec
6/6 Test [#83](/bitcoin-bitcoin/83/): script_assets_tests ..............   Passed    1.58 sec

100% tests passed, 0 tests failed out of 6

Total Test time (real) =   1.58 sec

hebasto added the label Tests on Dec 27, 2024
DrahtBot commented at 7:28 PM on December 27, 2024: contributor


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



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31576.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK maflcko, ajtowns, achow101

Stale ACK yuvicc

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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #33012 (script: return verification flag responsible for error upon validation failure by darosior)
- #32998 (Bump SCRIPT_VERIFY flags to 64 bit by ajtowns)
- #32473 (Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts by sipa)
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.
hebasto marked this as a draft on Dec 27, 2024
hebasto force-pushed on Dec 27, 2024
hebasto marked this as ready for review on Dec 27, 2024
hebasto commented at 3:01 PM on January 7, 2025: member

See the use case in https://github.com/hebasto/bitcoin-core-nightly/pull/27.
hebasto commented at 9:54 AM on February 10, 2025: member

Friendly ping @maflcko ;)
maflcko commented at 5:17 PM on February 25, 2025: member

Seems fine. No strong opinion.

Type	Reviewers
ACK	maflcko, ajtowns, achow101
Stale ACK	yuvicc

yuvicc commented at 7:27 PM on May 17, 2025: contributor

tACK 0859fc22f0baf00259cf964f9d39d36fba6cd291

making skipped tests explicitly appear as 'Skipped' rather than silently omitting

    Start 89: script_tests
    Start 85: script_p2sh_tests
    Start 87: script_segwit_tests
    Start 88: script_standard_tests
    Start 86: script_parse_tests
    Start 84: script_assets_tests
1/6 Test [#86](/bitcoin-bitcoin/86/): script_parse_tests ...............   Passed    0.16 sec
2/6 Test [#84](/bitcoin-bitcoin/84/): script_assets_tests ..............***Skipped   0.16 sec
3/6 Test [#88](/bitcoin-bitcoin/88/): script_standard_tests ............   Passed    0.17 sec
4/6 Test [#87](/bitcoin-bitcoin/87/): script_segwit_tests ..............   Passed    0.17 sec
5/6 Test [#85](/bitcoin-bitcoin/85/): script_p2sh_tests ................   Passed    0.17 sec
6/6 Test [#89](/bitcoin-bitcoin/89/): script_tests .....................   Passed    0.37 sec

100% tests passed, 0 tests failed out of 6

Total Test time (real) =   0.38 sec

The following tests did not run:
	 84 - script_assets_tests (Skipped)

in src/test/script_assets_tests.cpp:33 in 0859fc22f0 outdated

  28 | +unsigned int ParseScriptFlags(std::string strFlags);
  29 | +
  30 | +BOOST_AUTO_TEST_SUITE(script_assets_tests)
  31 | +
  32 | +/* Wrapper around ProduceSignature to combine two scriptsigs */
  33 | +SignatureData CombineSignatures(const CTxOut& txout, const CMutableTransaction& tx, const SignatureData& scriptSig1, const SignatureData& scriptSig2)

maflcko commented at 8:01 AM on June 20, 2025:

what is this for?

hebasto commented at 9:10 AM on June 20, 2025:

Thanks! Dropped.

maflcko approved

maflcko commented at 8:04 AM on June 20, 2025: member

makes sense to be explicit here to avoid confusion

lgtm ACK 0859fc22f0baf00259cf964f9d39d36fba6cd291 🐽

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 0859fc22f0baf00259cf964f9d39d36fba6cd291 🐽
oguvM7vjti8dmtabMmgzCKuxp+/7OOHqZVEMvjj+RjTVxfTJLoC+HJhdnAQ11Y5a2tsJI5smtPebrAKwdUAKBA==

</details>

test: Move `script_assets_tests` into its own suite

This change ensures that the `script_assets_tests` test case is
explicitly reported as "Skipped" when it is not run, making it clearer
when running the test suite with `ctest`.

c40dbbbf77

hebasto force-pushed on Jun 20, 2025
hebasto commented at 9:11 AM on June 20, 2025: member

Addressed feedback from @maflcko.

maflcko commented at 9:42 AM on June 20, 2025: member

only change is dropping dead code

re-ACK c40dbbbf77078bb86a652ca151b472e7bef61ae0 👈

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK c40dbbbf77078bb86a652ca151b472e7bef61ae0 👈
NetYDFd/QmbGErA2ugrrDGOZgiUEvhYtaPbG9oaznL6YlEGo5+XntdHOPsxEkTl4G1PYEoc+dEs9jOccdWLJDA==

</details>

DrahtBot requested review from yuvicc on Jun 20, 2025
ajtowns commented at 1:12 AM on July 23, 2025: contributor

ACK c40dbbbf77078bb86a652ca151b472e7bef61ae0

You could consider rebasing it on top of #32945 in order to not move the code to a different file, but that still involves moving code about and only avoids duplicating the ToScript function across both files.
achow101 commented at 9:33 PM on July 25, 2025: member

ACK c40dbbbf77078bb86a652ca151b472e7bef61ae0
achow101 merged this on Jul 25, 2025
achow101 closed this on Jul 25, 2025

l0rinc commented at 12:42 AM on July 26, 2025: contributor

test-only post-merge ACK

It's reporting script_assets_tests as skipped on Mac

<details> <summary>Details</summary>

129/144 Test  [#86](/bitcoin-bitcoin/86/): script_assets_tests ..................***Skipped   0.36 sec
...
The following tests did not run:
         86 - script_assets_tests (Skipped)

</details>

and on Linux

<details> <summary>Details</summary>

139/144 Test  [#86](/bitcoin-bitcoin/86/): script_assets_tests ..................***Skipped   0.20 sec
...
The following tests did not run:
         86 - script_assets_tests (Skipped)

</details>

While CI ran it successfully

<details> <summary>Details</summary>

131/143 Test  [#87](/bitcoin-bitcoin/87/): script_assets_tests ..................   Passed  184.12 sec

</details>

hebasto deleted the branch on Jul 26, 2025

Contributors

Review Requested
yuvicc

Labels

Tests

Linked (view graph)

#83 .gitignore #84 Transaction-less transfer of value #85 avoid sub-cent change (lost in fees) whenever possible #86 CreateTransaction: suggest/enforce fee for big low-priority transactions to avoid backlog #87 Ability to move/share addresses from one client to another #88 Smalltxfix

test: Move `script_assets_tests` into its own suite #31576

Code Coverage & Benchmarks

Reviews

Conflicts