test: Add option to skip python unit tests #29470

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:202402_skip_python_unit changing 1 files +12 −9

mzumsande commented at 7:21 PM on February 23, 2024: contributor

In the python test_runner, it's possible to disable specific functional tests (or just enable a few specific ones), but the unit tests for the python test framework cannot be skipped. Add this option (--skipunit or -u), it would save some time for devs not interested in running those every time.
DrahtBot commented at 7:21 PM on February 23, 2024: contributor


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



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK fjahr, stratospher, tdb3

Concept ACK 1440000bytes

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:
- #29387 (test: fix RPC coverage check by BrandonOdiwuor)
- #29335 (test: Handle functional test disk-full error by BrandonOdiwuor)
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.
DrahtBot added the label Tests on Feb 23, 2024
1440000bytes commented at 7:56 PM on February 23, 2024: none

it would save some time for devs not interested in running those every time

Makes no sense. In fact, it could waste more time for everyone in the process.

However, since this is an option.

Concept ACK
DrahtBot added the label CI failed on Feb 23, 2024
in test/functional/test_runner.py:576 in c84d0f202b outdated
```
 579 | -        test_framework_tests.addTest(unittest.TestLoader().loadTestsFromName("test_framework.{}".format(module)))
 580 | -    result = unittest.TextTestRunner(verbosity=1, failfast=True).run(test_framework_tests)
 581 | -    if not result.wasSuccessful():
 582 | -        sys.exit("Early exiting after failure in TestFramework unit tests")
 583 | +    if not skipunit:
 584 | +        # Test Framework Tests
```
fjahr commented at 9:49 PM on February 23, 2024:

nit: That comment isn't very helpful IMO and seems unnecessary since the print below documents what's going on here much better, so it could be removed. Maybe this was meant more as a section divider (?) but then it should stay on the same line where it was before.

mzumsande commented at 10:13 PM on February 23, 2024:

I agree - removed it!
fjahr commented at 9:52 PM on February 23, 2024: contributor

tACK c84d0f202b0fae3cbc85d5b6fe0ebe93ee84eacb
test: Add option to skip unit tests for the test runner 5f240ab2e8
mzumsande force-pushed on Feb 23, 2024
mzumsande commented at 10:16 PM on February 23, 2024: contributor

Makes no sense. In fact, it could waste more time for everyone in the process.

An example: I often run commands like python3 test_runner.py p2p* on local branches to quickly find out if a p2p change I just made broke something. In this case, it's annoying having to wait for half a minute (or any amount of time really) for some test framework unit tests that cannot possibly be affected by my changes to bitcoind.
fjahr commented at 10:26 PM on February 23, 2024: contributor

re-ACK 5f240ab2e89fb20286fbaf9a1f00346bb1cad5a1
tdb3 commented at 10:28 PM on February 23, 2024: contributor

Code review and tested ACK 5f240ab2e89fb20286fbaf9a1f00346bb1cad5a1

Useful feature. LGTM.
DrahtBot removed the label CI failed on Feb 24, 2024
stratospher commented at 4:34 AM on February 24, 2024: contributor

tested ACK 5f240ab.
maflcko commented at 9:49 AM on February 24, 2024: member
lgtm

Idea for a follow-up:

Would it be possible to move this code blob into a separate python file, to be run in parallel with all other tests?

This should solve all issues:
- You can run test_runner.py p2p* as-is to skip them
- Anyone running all tests, will run them in parallel, without blocking a single CPU for half a minute
- Anyone running just the unit tests, could do so
mzumsande commented at 3:26 PM on February 24, 2024: contributor

Idea for a follow-up: (...)

If that works, it sounds like the best approach! I'm not going to work on this anytime soon, perhaps @tdb3 you might be interested?
1440000bytes commented at 5:59 PM on February 24, 2024: none

Makes no sense. In fact, it could waste more time for everyone in the process.

An example: I often run commands like python3 test_runner.py p2p* on local branches to quickly find out if a p2p change I just made broke something. In this case, it's annoying having to wait for half a minute (or any amount of time really) for some test framework unit tests that cannot possibly be affected by my changes to bitcoind.

All tests are annoying so I skip all and never run them. It really helps them when i see red ci.
tdb3 commented at 7:38 PM on February 24, 2024: contributor
Idea for a follow-up: (...)

If that works, it sounds like the best approach! I'm not going to work on this anytime soon, perhaps @tdb3 you might be interested?

Sounds interesting to me. @maflcko, does this sound aligned with the desire/preference?
- Do not run the framework tests before all other tests in https://github.com/bitcoin/bitcoin/blob/1ac627c485a43e50a9a49baddce186ee3ad4daad/test/functional/test_runner.py#L577
- Migrate the execution of the framework tests into another file, such that the framework tests are treated just like any other tests (finishing after some non-zero number of tests have completed), (e.g. perhaps in test_list, inserted into an ideal runtime section https://github.com/bitcoin/bitcoin/blob/1ac627c485a43e50a9a49baddce186ee3ad4daad/test/functional/test_runner.py#L476)
I imagine there could be some who prefer framework unit tests to be finished before other tests run. Do you think it's worth opening an Issue for this to discuss first, to see if sufficient concept ack can be reached? If so, I can open one.
maflcko commented at 9:27 AM on February 26, 2024: member
I imagine there could be some who prefer framework unit tests to be finished before other tests run. Do you think it's worth opening an Issue for this to discuss first, to see if sufficient concept ack can be reached? If so, I can open one.

I think it should be fine to call the unit tests manually, in this (rare?) case?
```
./test/functional/feature_unit_self_tests.py # (or whatever it is called)
```
fanquake merged this on Feb 26, 2024
fanquake closed this on Feb 26, 2024
mzumsande deleted the branch on Feb 27, 2024
achow101 referenced this in commit 1ffbd96349 on Apr 26, 2024
bitcoin locked this on Feb 26, 2025