test: enable running independent functional test sub-tests

ismaelsadeeq commented at 3:55 pm on September 27, 2024: member

Some test methods in the functional test framework are independent and do not require any prior context or setup in run_test.
This commit adds a new option for running these specific methods within a test file, allowing them to be executed individually without running the entire test suite.
Using this option reduces the time you need to wait before the test you are interested in starts executing.
The functionality added by this PR can be achieved manually by commenting out code, but having a pragmatic option to do this is more convenient.

Note: Running test methods that require arguments or context will fail.

Example Usage:

0build/test/functional/feature_reindex.py --test_methods continue_reindex_after_shutdown

0build/test/functional/feature_config_args.py --test_methods test_log_buffer test_args_log test_connect_with_seednode

DrahtBot commented at 3:55 pm on September 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/30991.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, maflcko, ryanofsky

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

DrahtBot added the label Tests on Sep 27, 2024

DrahtBot added the label CI failed on Sep 29, 2024

DrahtBot removed the label CI failed on Oct 1, 2024

ismaelsadeeq renamed this:
~~test: enable running independent functional test methods~~
test: enable running independent functional test sub-tests
on Oct 1, 2024

in test/functional/test_framework/test_framework.py:163 in cc21876b12 outdated

158@@ -155,6 +159,15 @@ def main(self):
159             exit_code = self.shutdown()
160             sys.exit(exit_code)
161 
162+    def run_custom_test(self):
163+        for method_name in self.options.test_methods:

rkrux commented at 7:16 am on November 4, 2024:

Since the argument is a list named test_methods, let’s call this function run_custom_tests?

ismaelsadeeq commented at 10:49 pm on November 4, 2024:

Done, thanks for reviewing

in test/functional/test_framework/test_framework.py:168 in cc21876b12 outdated

163+        for method_name in self.options.test_methods:
164+            method = getattr(self, method_name, None)
165+            if not method:
166+                raise AttributeError(f"No method with name {method_name}")
167+            if not callable(method):
168+                raise TypeError(f"{method_name} is not callable")

rkrux commented at 7:18 am on November 4, 2024:

Might as well find the searchable & callable methods initially and execute them while throwing the error for those that are filtered out? Otherwise few would be executed and the first incorrect one would throw error.

ismaelsadeeq commented at 10:54 pm on November 4, 2024:

Done, thanks

in test/functional/test_framework/test_framework.py:209 in cc21876b12 outdated

206@@ -194,6 +207,8 @@ def parse_args(self, test_file):
207                             help="use BIP324 v2 connections between all nodes by default")
208         parser.add_argument("--v1transport", dest="v1transport", default=False, action="store_true",
209                             help="Explicitly use v1 transport (can be used to overwrite global --v2transport option)")
210+        parser.add_argument("--test_methods", dest="test_methods", nargs='*',
211+                            help="Run specified test methods sequentially instead of the full test. Use only for methods that do not depend on any context set up in run_test or other methods.")

rkrux commented at 7:21 am on November 4, 2024:

The PR title states that this will enable running functional test sub-tests but any method can be run with this argument at the moment such as one below. Should we restrict only for those that have the test_ prefix?

0+    def sample_calculator(self):
1+        self.log.info("Sample calculator running")
2+        x = 2 + 3
3+        self.log.info(f'Calculated x to be: {x}')
4+

ismaelsadeeq commented at 10:58 pm on November 4, 2024:

Some tests might not have the test prefix, though. I don’t think we can use the test_ prefix to catch all sub-tests; rather, the goal is just to enable running individual methods that can be sub-tests.

rkrux commented at 8:56 am on November 5, 2024:

Ok I didn’t know that all tests will not have the test prefix. Sounds good then.

rkrux commented at 9:05 am on November 5, 2024:

or other methods.

Nit: This can be reworded though because the following snippet works.

0    def calculate(self, val):
1        return val
2
3    def sample_calculator(self):
4        self.log.info("Sample calculator running")
5        x = self.calculate(2 + 3)
6        self.log.info(f'Calculated x to be: {x}')

0➜  bitcoin git:(09-2024-run-ind-functional-test) ✗ build/test/functional/feature_config_args.py --test_methods test_log_buffer test_args_log sample_calculator

ismaelsadeeq commented at 12:59 pm on November 11, 2024:

leaving this as is; but happy to take explicit suggestions :)

rkrux commented at 8:52 am on November 14, 2024:

If I’m not missing something, why not just remove it?

ismaelsadeeq commented at 5:49 pm on November 15, 2024:

It is useful to because running build/test/functional/test_runner.py --help, will describe this new feature for users. If this is removed, it will not be documented, and future users may not learn about it. Therefore, I will be leaving it as is.

rkrux commented at 2:44 am on November 16, 2024:

I meant removing only “or other methods”.

ismaelsadeeq commented at 3:26 pm on December 2, 2024:

Happy to update when retouching.

rkrux approved

rkrux commented at 7:23 am on November 4, 2024: none

Concept ACK cc21876b125930f8320dbb95016f9ee7c1ffec55

Successful make and functional tests. Tried running custom methods with the new --test_methods argument. Left couple suggestions.

ismaelsadeeq force-pushed on Nov 4, 2024

rkrux approved

rkrux commented at 9:07 am on November 5, 2024: none

ACK e96823bdcc2317fbf0bed64e53a1b3e5bb8f00f6

Thanks for quickly addressing the feedback.

test: enable running individual independent functional test methods

- Some test methods in the functional test framework are independent
  and do not require any previous context or setup defined in `run_test`.
- This commit adds a new option for running these specific methods within a test file,
  allowing them to be executed individually without running the entire test suite.

- running test methods that require an argument or context will fail.

409d0d6293

in test/functional/test_framework/test_framework.py:182 in e96823bdcc outdated

177+            error_message = f"Errors encountered:"
178+            if incorrect_methods:
179+                error_message += f"\nIncorrect methods (not found): {incorrect_methods}"
180+            if uncallable_methods:
181+                error_message += f"\nUncallable methods: {uncallable_methods}"
182+            raise RuntimeError(error_message)

maflcko commented at 11:38 am on November 8, 2024:

Is it really useful to map one exception type to another for a test-only dev-only debug-only feature? I’d say it seems fine to just use the python built-in exceptions, which should be self-explanatory. I understand that they won’t print the method name, but it could make sense to log that either way as the first thing in the for loop?

ismaelsadeeq commented at 8:26 pm on November 8, 2024:

The approach here was added to address this comment cc @rkrux .

Which is an overkill for this simple feature honestly.

Is the simplified approach below preferable?

0def run_custom_tests(self):
1    for method_name in self.options.test_methods:
2        self.log.info(f"Attempting to execute method: {method_name}")
3        method = getattr(self, method_name)
4        method()
5        self.log.info(f"Method '{method_name}' executed successfully.")

This version has a few more advantage.

It logs each method’s execution attempt and relies on Python’s built-in exceptions to handle issues.
The code is simpler.
Additionally, it provides helpful feedback if a method name is close to a valid method but has a typo. For instance, omitting the final “n” in the method name would result in: when running the example in the OP will result in

0Traceback (most recent call last):
1  File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
2    self.run_custom_tests()
3  File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/test_framework/test_framework.py", line 165, in run_custom_tests
4    method = getattr(self, method_name)
5             ^^^^^^^^^^^^^^^^^^^^^^^^^^
6AttributeError: 'ReindexTest' object has no attribute 'continue_reindex_after_shutdow'. Did you mean: 'continue_reindex_after_shutdown'?

rkrux commented at 6:30 am on November 11, 2024:

Since this commit was checking for the presence and call-ability of the passed methods already, the main intent of this comment was to add on to it by allowing the execution of all the methods in case of failure of any one of them.

test-only dev-only debug-only feature

I believe keeping this^ in mind, it makes sense to just throw the error in case if any invalid method is passed. I agree the approach shared in the above comment is simpler and good enough for the use case.

Though, I feel sticking to only one term between test_methods or custom_tests would be better from readability POV.

rkrux commented at 6:35 am on November 11, 2024:

With the above approach, I see the following outputs that seem okay.

Error when a non existing attribute is passed.

02024-11-11T06:12:11.578000Z TestFramework (INFO): Attempting to execute method: test_args_l
12024-11-11T06:12:11.579000Z TestFramework (ERROR): Unexpected exception caught during testing
2Traceback (most recent call last):
3  File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
4    self.run_custom_tests()
5    ~~~~~~~~~~~~~~~~~~~~~^^
6  File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 165, in run_custom_tests
7    method = getattr(self, method_name)
8AttributeError: 'ConfArgsTest' object has no attribute 'test_args_l'. Did you mean: 'test_args_log'?

Error when a non callable attribute is passed.

02024-11-11T06:16:15.078000Z TestFramework (INFO): Attempting to execute method: chain
12024-11-11T06:16:15.078000Z TestFramework (ERROR): Unexpected exception caught during testing
2Traceback (most recent call last):
3  File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
4    self.run_custom_tests()
5    ~~~~~~~~~~~~~~~~~~~~~^^
6  File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 166, in run_custom_tests
7    method()
8    ~~~~~~^^
9TypeError: 'str' object is not callable

ismaelsadeeq commented at 12:58 pm on November 11, 2024:

Done in latest push. thanks

ismaelsadeeq force-pushed on Nov 11, 2024

rkrux approved

rkrux commented at 8:53 am on November 14, 2024: none

reACK 409d0d629378c3e23388ed31516376ad1ae536b5

maflcko commented at 9:05 am on November 14, 2024: member

review ACK 409d0d629378c3e23388ed31516376ad1ae536b5

ryanofsky approved

ryanofsky commented at 4:44 pm on December 2, 2024: contributor

Code review ACK 409d0d629378c3e23388ed31516376ad1ae536b5. This seems like a good step towards making it easy to run independent tests quickly. I think ideally there would be some naming convention or @ annotation added to test methods that can run independently, so the test framework could provide more functionality like being able to list test methods, being able to show command lines to quickly reproduce problems when tests fails, and calling test methods automatically instead of requiring individual tests to call them. But these ideas are all compatible with the new --test_methods option

ryanofsky merged this on Dec 2, 2024

ryanofsky closed this on Dec 2, 2024

ismaelsadeeq deleted the branch on Dec 2, 2024

test: enable running independent functional test sub-tests #30991

Code Coverage & Benchmarks

Reviews