-
Notifications
You must be signed in to change notification settings - Fork 79
Refact test runner #671
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
base: master
Are you sure you want to change the base?
Refact test runner #671
Conversation
|
The command to run is actually Also, a rebase would be good |
jaoleal
left a comment
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.
Nice!!! I was able to run once but when i ran it again i got a
Failed to start florestad: Failed to bind Electrum server: Address already in use (os error 98)This is a partial review btw, we should focus on organizing nodes to avoid such errors.
d399e3e to
7f1c9d3
Compare
Solved with a cleanup on the temporary dir of the nodes data and logs. |
7f1c9d3 to
cdb4ba3
Compare
c2dce29 to
8859071
Compare
|
Floresta is logging to stdout. I think this is too polluting and unnecessary |
8859071 to
42147e9
Compare
moisesPompilio
left a comment
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.
Mark some lines that exceed 80 characters. In this case, it would be interesting if the changes you made followed a pattern where each line has a limit of 80 characters per line.
|
@Davidson-Souza I've already started writing the florestad tests, but I'm worried this PR is becoming too long. What do you think about just solving the issues @moisesPompilio pointed out (and any following ones), and leaving the florestad and floresta-cli test conversions to pytest for another PR? |
45c62ef to
4581127
Compare
Sure! |
4581127 to
dca5f58
Compare
moisesPompilio
left a comment
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 PR description should include instructions on how to run the tests. If pytest requires specific commands, they need to be listed.
It should also mention whether the migrated tests will be integrated into test_runner or propose a new strategy for running all tests.
| log_to_file: params.log_to_file, | ||
| assume_valid: params.assume_valid, | ||
| log_to_stdout: true, | ||
| log_to_stdout: false, |
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’m not sure if changing this part is appropriate, what do you think @Davidson-Souza ?
I think the logging issue comes from Floresta, Utreexod, and Bitcoind all printing to the terminal, which makes analysis harder. It might be useful to have a flag to enable terminal logs when you want to see what the node is doing. But for tests, ideally only the information relevant to the test should appear, like whether a node connected or disconnected, and that should be handled by the test itself.
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.
think the logging issue comes from Floresta, Utreexod, and Bitcoind all printing to the terminal
+1
| @@ -1,64 +1,11 @@ | |||
| """ | |||
| bitcoin-test.py | |||
| import pytest | |||
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.
IMO, i think we should keep, at least the module docstrings. Class docstrings and method docstrings are welcomed too, since one of the reasons on every file on tests (including the framework).
In case of this test, it have an educational approach: it should explain well what is happening so developers could be guided.
| """ | ||
|
|
||
| from test_framework import FlorestaTestFramework | ||
| import pytest |
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.
Here too
| `run_test, but DOES NOT override `__init__` or `main`. If those standards | ||
| are violated, a `TypeError` is raised. | ||
| """ | ||
| """Metaclass for enforcing test framework contract.""" |
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.
Why remove the bigtext on docstring?
| test/test_framework/{crypto,daemon,rpc,electrum}/*.py to see | ||
| how the test framework was structured. | ||
| """ | ||
| """Base class for Floresta integration tests.""" |
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, I agree that is a very big text. But i still against remove a big block of docstrings on tests. Maybe they could be rewriten, but they should guide the new testers.
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.
This init.py will be removed we can create some documentation work on the node_manager.py, i don't agree too much of write docs for a dead marked 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.
I think we can work more on the documentation when everything is integrated with pytest's ideas and we have a good overview of what will be discarded and what will be merged with what.
| This is used to create a log file for the test. | ||
| """ | ||
| tempdir = str(FlorestaTestFramework.get_integration_test_dir()) | ||
| """Get the path for the test log 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.
IMO it's important to explain that the class name will come in lowercase.
| If the node not exists raise a IndexError. At the time | ||
| the tests will only run nodes configured to run on regtest. | ||
| """Start a node and initialize its RPC connection. |
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.
Here too
|
|
||
| # pylint: disable=redefined-outer-name # Pytest fixtures pattern | ||
|
|
||
| sys.path.insert(0, os.path.dirname(__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.
for windows could be worth to use os.path.normpath
| return node | ||
|
|
||
|
|
||
| @pytest.fixture |
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 think could be nice to add an utreexod_with_tls to test electrum stuffs
| @@ -1,330 +1,272 @@ | |||
| """ | |||
| test_runner.py | |||
| """i disable all pylint here because this is a temporary file, we will replace | |||
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.
Here too, about the comments in the docstrings at module level
| @@ -0,0 +1,116 @@ | |||
| """Utility functions for Floresta integration tests. | |||
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.
Very nice! IMO this docstring at module level could receive more love and a little more verbose explanation of which types of utilities we can expect here.
The new instructions should be on Also, neet to add an |
dca5f58 to
6c2ea05
Compare
| log_to_file: params.log_to_file, | ||
| assume_valid: params.assume_valid, | ||
| log_to_stdout: true, | ||
| log_to_stdout: false, |
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.
think the logging issue comes from Floresta, Utreexod, and Bitcoind all printing to the terminal
+1
tests/test_runner.py
Outdated
| results_queue = Queue() | ||
| workers = [] | ||
| print("=" * 60) | ||
| print(f"TEST SUMMARY") |
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.
| print(f"TEST SUMMARY") | |
| print("TEST SUMMARY") |
tests/test_runner.py
Outdated
|
|
||
| # Argument parser | ||
| parser = argparse.ArgumentParser( | ||
| prog="multi_test_runner", description="Run Floresta integration tests" |
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.
| prog="multi_test_runner", description="Run Floresta integration tests" | |
| prog="test_runner", description="Run Floresta integration tests" |
| ("example", "utreexod"), | ||
| ] | ||
|
|
||
| # Before running the tests, we check if the number of tests |
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.
Why remove these check? When someone add a test and the test isn't registered, it will not run the test in CI and give some false sense of success.
tests/test_framework/__init__.py
Outdated
| import contextlib | ||
| from datetime import datetime, timezone | ||
| from typing import Any, Dict, List, Literal, Pattern, TextIO | ||
| from typing import Any, Dict, List, Pattern |
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.
Unused Dict:
| from typing import Any, Dict, List, Pattern | |
| from typing import Any, List, Pattern |
| Check if an option is set in extra_args | ||
| return any(arg.startswith(option) for arg in extra_args) | ||
|
|
||
| def extract_port_from_args(self, extra_args: list[str], option: str) -> int: |
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.
IMO we can rename this method since it's not extracting anymore, but instead, giving a random available one:
| def extract_port_from_args(self, extra_args: list[str], option: str) -> int: | |
| def get_available_random_port(self, extra_args: list[str], option: str) -> int: |
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.
| ports["rpc"] = 18443 + port_index | ||
| default_args.append(f"--rpc-address=127.0.0.1:{ports['rpc']}") | ||
| else: | ||
| ports["rpc"] = self.extract_port_from_args(extra_args, "--rpc-address") |
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.
Replace the name with what you renamed
| """ | ||
| Pytest configuration and fixtures for node testing. | ||
| This module provides fixtures for creating and managing test nodes | ||
| (florestad, bitcoind, utreexod) in various configurations. |
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 think it's worth to teach rustceans how to use created marks with examples in docstring at module level.
| @pytest.fixture | ||
| def electrum_setup(node_manager) -> Dict[str, Node]: | ||
| """Setup for electrum tests""" | ||
| florestad = node_manager.create_node( | ||
| variant="florestad", | ||
| extra_args=["--electrum-address=127.0.0.1:50001"], | ||
| testname="pytest_electrum", | ||
| ) | ||
| node_manager.start_node(florestad) | ||
| return {"florestad": florestad} |
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.
IMO we can change the name in favor to add another implementation in a follow-up (electrs, esplora, etc...):
| @pytest.fixture | |
| def electrum_setup(node_manager) -> Dict[str, Node]: | |
| """Setup for electrum tests""" | |
| florestad = node_manager.create_node( | |
| variant="florestad", | |
| extra_args=["--electrum-address=127.0.0.1:50001"], | |
| testname="pytest_electrum", | |
| ) | |
| node_manager.start_node(florestad) | |
| return {"florestad": florestad} | |
| @pytest.fixture | |
| def floresta_electrum_setup(node_manager) -> Dict[str, Node]: | |
| """Setup for electrum tests""" | |
| florestad = node_manager.create_node( | |
| variant="florestad", | |
| extra_args=["--electrum-address=127.0.0.1:50001"], | |
| testname="pytest_electrum", | |
| ) | |
| node_manager.start_node(florestad) | |
| return {"florestad": florestad} |
| # I disable the following because i literally will delete this file too later | ||
| # its a middle way to the __init__.py from test framework for the pytest |
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.
IMO this could be in tests/test_framework/node/{__init__.py,node.py,node_manager.py}.
| # Scripts that are run by default. | ||
| # Longest test should go first, | ||
| # to favor running tests in parallel. | ||
| # We use this like those in the | ||
| # Bitcoin Core tests/functional/test_runner.py:89 | ||
|
|
||
| # Tests that are ran by default. The longest running tests should be ran first, | ||
| # so parallelization is used most effectively. This structure is copied from | ||
| # Bitcoin Core's functional tests: | ||
| # https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_runner.py#L89 |
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.
IMO, this explanation is important to be above if TEST_REGISTRY declaration on previous commented review request.
| TEST_REGISTRY = { | ||
| "addnodev2": import_test_class("floresta-cli.addnode-v2", "AddnodeTestV2"), | ||
| "addnodev1": import_test_class("floresta-cli.addnode-v1", "AddnodeTestV1"), | ||
| "reorg_chain": import_test_class("florestad.reorg-chain", "ChainReorgTest"), | ||
| "getbestblockhash": import_test_class( | ||
| "floresta-cli.getbestblockhash", "GetBestblockhashTest" | ||
| ), | ||
| "getblockcount": import_test_class( | ||
| "floresta-cli.getblockcount", "GetBlockCountTest" | ||
| ), | ||
| "uptime": import_test_class("floresta-cli.uptime", "UptimeTest"), | ||
| "restart": import_test_class("florestad.restart", "TestRestart"), | ||
| "connect": import_test_class("florestad.connect", "CliConnectTest"), | ||
| "stop": import_test_class("floresta-cli.stop", "StopTest"), | ||
| "ping": import_test_class("floresta-cli.ping", "PingTest"), | ||
| "getrpcinfo": import_test_class("floresta-cli.getrpcinfo", "GetRpcInfoTest"), | ||
| "getblockhash": import_test_class("floresta-cli.getblockhash", "GetBlockhashTest"), | ||
| "tls": import_test_class("florestad.tls", "TestSslInitialization"), | ||
| "getroots": import_test_class("floresta-cli.getroots", "GetRootsIDBLenZeroTest"), | ||
| "getblock": import_test_class("floresta-cli.getblock", "GetBlockTest"), | ||
| "getmemoryinfo": import_test_class( | ||
| "floresta-cli.getmemoryinfo", "GetMemoryInfoTest" | ||
| ), | ||
| "getblockheader": import_test_class( | ||
| "floresta-cli.getblockheader", "GetBlockheaderHeightZeroTest" | ||
| ), | ||
| "getpeerinfo": import_test_class("floresta-cli.getpeerinfo", "GetPeerInfoTest"), | ||
| "tls_fail": import_test_class("florestad.tls-fail", "TestSslFailInitialization"), | ||
| "getblockchaininfo": import_test_class( | ||
| "floresta-cli.getblockchaininfo", "GetBlockchaininfoTest" | ||
| ), | ||
| } |
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 are some forgotten tests here as well IMO this could be cleaner:
| TEST_REGISTRY = { | |
| "addnodev2": import_test_class("floresta-cli.addnode-v2", "AddnodeTestV2"), | |
| "addnodev1": import_test_class("floresta-cli.addnode-v1", "AddnodeTestV1"), | |
| "reorg_chain": import_test_class("florestad.reorg-chain", "ChainReorgTest"), | |
| "getbestblockhash": import_test_class( | |
| "floresta-cli.getbestblockhash", "GetBestblockhashTest" | |
| ), | |
| "getblockcount": import_test_class( | |
| "floresta-cli.getblockcount", "GetBlockCountTest" | |
| ), | |
| "uptime": import_test_class("floresta-cli.uptime", "UptimeTest"), | |
| "restart": import_test_class("florestad.restart", "TestRestart"), | |
| "connect": import_test_class("florestad.connect", "CliConnectTest"), | |
| "stop": import_test_class("floresta-cli.stop", "StopTest"), | |
| "ping": import_test_class("floresta-cli.ping", "PingTest"), | |
| "getrpcinfo": import_test_class("floresta-cli.getrpcinfo", "GetRpcInfoTest"), | |
| "getblockhash": import_test_class("floresta-cli.getblockhash", "GetBlockhashTest"), | |
| "tls": import_test_class("florestad.tls", "TestSslInitialization"), | |
| "getroots": import_test_class("floresta-cli.getroots", "GetRootsIDBLenZeroTest"), | |
| "getblock": import_test_class("floresta-cli.getblock", "GetBlockTest"), | |
| "getmemoryinfo": import_test_class( | |
| "floresta-cli.getmemoryinfo", "GetMemoryInfoTest" | |
| ), | |
| "getblockheader": import_test_class( | |
| "floresta-cli.getblockheader", "GetBlockheaderHeightZeroTest" | |
| ), | |
| "getpeerinfo": import_test_class("floresta-cli.getpeerinfo", "GetPeerInfoTest"), | |
| "tls_fail": import_test_class("florestad.tls-fail", "TestSslFailInitialization"), | |
| "getblockchaininfo": import_test_class( | |
| "floresta-cli.getblockchaininfo", "GetBlockchaininfoTest" | |
| ), | |
| } | |
| # Scripts that are run by default. | |
| # Longest test should go first, | |
| # to favor running tests in parallel. | |
| # We use this like those in the | |
| # Bitcoin Core tests/functional/test_runner.py:89 | |
| # Tests that are ran by default. The longest running tests should be ran first, | |
| # so parallelization is used most effectively. This structure is copied from | |
| # Bitcoin Core's functional tests: | |
| # https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_runner.py#L89 | |
| TEST_REGISTRY = {} | |
| REGISTRY_LIST = [ | |
| ("floresta-cli.addnode-v2", "AddnodeTestV2"), | |
| ("floresta-cli.addnode-v1", "AddnodeTestV1"), | |
| ("florestad.reorg-chain", "ChainReorgTest"), | |
| ("floresta-cli.getbestblockhash", "GetBestblockhashTest"), | |
| ("floresta-cli.getblockcount", "GetBlockCountTest"), | |
| ("floresta-cli.uptime", "UptimeTest"), | |
| ("florestad.restart", "TestRestart"), | |
| ("florestad.connect", "CliConnectTest"), | |
| ("floresta-cli.stop", "StopTest"), | |
| ("floresta-cli.ping", "PingTest"), | |
| ("floresta-cli.getrpcinfo", "GetRpcInfoTest"), | |
| ("floresta-cli.getblockhash", "GetBlockhashTest"), | |
| ("florestad.tls", "TestSslInitialization"), | |
| ("floresta-cli.getroots", "GetRootsIDBLenZeroTest"), | |
| ("floresta-cli.getblock", "GetBlockTest"), | |
| ("floresta-cli.getmemoryinfo", "GetMemoryInfoTest"), | |
| ("floresta-cli.getblockheader", "GetBlockheaderHeightZeroTest"), | |
| ("floresta-cli.getpeerinfo", "GetPeerInfoTest"), | |
| ("florestad.tls-fail", "TestSslFailInitialization"), | |
| ( "floresta-cli.getblockchaininfo", "GetBlockchaininfoTest"), | |
| ] | |
| for (module_name, class_name) in REGISTRY_LIST: | |
| key_template = module_name.split(".")[1] | |
| key = key_template.replace("-", "") | |
| TEST_REGISTRY[key] = import_test_class(module_name, class_name) | |
tests/test_runner.py
Outdated
|
|
||
| # Get integration test directory | ||
| try: | ||
| temp_dir = FlorestaTestFramework.get_integration_test_dir() |
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.
No need to declare the variable temp_dir:
| temp_dir = FlorestaTestFramework.get_integration_test_dir() | |
| FlorestaTestFramework.get_integration_test_dir() |
| default=[], | ||
| help="Test name to be tested in a suite. May be used more than once.", | ||
| "-v", "--verbose", action="store_true", help="Show verbose output on failure" | ||
| ) |
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.
This remotion will cause issues to test individually while developing
| # Handle --list-tests | ||
| if args.list_tests: | ||
| print(f"{INFO_EMOJI} Available tests:") | ||
| for test_name in sorted(TEST_REGISTRY.keys()): | ||
| print(f" • {test_name}") | ||
| print(f"\nTotal: {len(TEST_REGISTRY)} tests") | ||
| return | ||
|
|
||
| test_dir = os.path.abspath(os.path.dirname(__file__)) | ||
| # Determine which tests to run | ||
| if args.test == "all": | ||
| tests_to_run = TEST_REGISTRY | ||
| print(f"{INFO_EMOJI} Running ALL tests ({len(tests_to_run)} total)") | ||
| else: | ||
| tests_to_run = {args.test: TEST_REGISTRY[args.test]} | ||
| print(f"{INFO_EMOJI} Running single test: {args.test}") | ||
|
|
||
| if args.list_suites: | ||
| list_test_suites(test_dir) | ||
| return | ||
| # Run the tests | ||
| overall_start_time = time.time() | ||
|
|
||
| task_queue = setup_test_suite(args, test_dir) | ||
| results = run_test_workers(task_queue, args) | ||
| if len(tests_to_run) == 1: | ||
| # Single test mode - use original behavior | ||
| test_name = list(tests_to_run.keys())[0] | ||
| test_class = list(tests_to_run.values())[0] | ||
|
|
||
| passed = [(name, log, start, end) for (name, ok, log, start, end) in results if ok] | ||
| failed = [ | ||
| (name, log, start, end) for (name, ok, log, start, end) in results if not ok | ||
| ] | ||
| result = run_test_direct(test_name, test_class, args.verbose) | ||
|
|
||
| print("\nTest Summary:") | ||
| print(f"\n{len(passed)} test(s) passed:") | ||
| for name, log, start, end in passed: | ||
| print(f"\n {SUCCESS_EMOJI} {name}: {log} (took {end - start:.2f}s)") | ||
| if result.success: | ||
| print(f"{SUCCESS_EMOJI} {test_name} PASSED in {result.duration:.2f}s") | ||
| print(f"{ALLDONE_EMOJI} Test completed successfully!") | ||
| else: | ||
| print(f"{FAILURE_EMOJI} {test_name} FAILED in {result.duration:.2f}s") | ||
| if args.verbose and result.error_msg: | ||
| print(f"Error: {result.error_msg}") | ||
|
|
||
| if failed: | ||
| print(f"\n{len(failed)} test(s) failed:") | ||
| for name, log, start, end in failed: | ||
| print(f"\n {FAILURE_EMOJI} {name} failed: {log} (took {end - start:.2f}s)") | ||
| raise SystemExit( | ||
| f"\n{FAILURE_EMOJI} Some tests failed. Check the logs in {args.log_dir}." | ||
| if not result.success: | ||
| sys.exit(1) | ||
| else: | ||
| # Multi-test mode | ||
| results, overall_duration = run_all_tests( | ||
| tests_to_run, | ||
| args.verbose, | ||
| continue_on_failure=not args.stop_on_failure, | ||
| ) | ||
|
|
||
| # Print summary | ||
| all_passed = print_summary(results, overall_duration, args.verbose) | ||
|
|
||
| if not all_passed: | ||
| sys.exit(1) | ||
|
|
||
| overall_end_time = time.time() | ||
| print(f"Total runtime: {overall_end_time - overall_start_time:.2f}s") | ||
|
|
||
| # Run pytest after all tests complete | ||
| print("\n" + "=" * 60) | ||
| print(f"{RUNNING_EMOJI} Running pytest tests...") | ||
| print("=" * 60) | ||
|
|
||
| try: | ||
| result = subprocess.run( | ||
| ["uv", "run", "pytest", "tests/", "-n=4"], capture_output=False, text=True | ||
| ) | ||
|
|
||
| if result.returncode == 0: | ||
| print(f"\n{SUCCESS_EMOJI} Pytest tests completed successfully!") | ||
| else: | ||
| print( | ||
| f"\n{FAILURE_EMOJI} Pytest tests failed with exit code: {result.returncode}" | ||
| ) | ||
| sys.exit(result.returncode) | ||
| except FileNotFoundError: | ||
| print( | ||
| f"\n{FAILURE_EMOJI} Error: 'uv' command not found. Make sure uv is installed." | ||
| ) | ||
| print(f"\n{ALLDONE_EMOJI} ALL TESTS PASSED! GOOD JOB!") | ||
| sys.exit(1) | ||
| except Exception as e: | ||
| print(f"\n{FAILURE_EMOJI} Error running pytest: {e}") | ||
| sys.exit(1) |
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 filtering feature of individual tests shorten the development phase time. I know this will be resolved with pytest but while the old test_runner isn't fully removed i think could be good to maintain it.
ac759f8 to
19dcdf1
Compare
jaoleal
left a comment
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.
This is some old review, feel free to just resolve them if you think they are outdated.
| log_to_file: params.log_to_file, | ||
| assume_valid: params.assume_valid, | ||
| log_to_stdout: true, | ||
| log_to_stdout: false, |
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.
This is to avoid logging during test execution, correct ? It could be a parameter, but it seems out-of-scope for this PR.
| This metaclass ensures that any subclass of `FlorestaTestFramework` | ||
| adheres to a standard whereby the subclass overrides `set_test_params` and | ||
| `run_test, but DOES NOT override `__init__` or `main`. If those standards | ||
| are violated, a `TypeError` is raised. |
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.
This part of the doc appears to be important, no ?
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.
This is outdated and this file will be removed, new tests should be in pytest format that don't need this form and old ones will be refactored to pytest too so i don't think this should be mantained.
|
|
||
| # pylint: disable=too-many-public-methods | ||
| class FlorestaTestFramework(metaclass=FlorestaTestMetaClass): | ||
| """ |
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.
This whole doc explains how we expect FlorestaTestFramework to be consumed, it should be kept and apply changes where needed
7f1d391 to
fb8a6ec
Compare
|
Hi @Davidson-Souza @qlrd @moisesPompilio @jaoleal — this PR currently mixes several independent changes, which makes review and merging slower and riskier. I propose splitting it into smaller, focused PRs in this order:
Because some files will be removed/replaced (for example test_framework/init.py → node_manager.py), adding documentation to files that are about to be replaced may not be the best use of time. If the purpose of those docs is to help new testers, I recommend encouraging new testers to implement tests directly in pytest. |
Happens!!! I really got lost here. |
|
I think it can be done here, but with this commit structure you've mentioned. Right now this git history is kinda messed, full of "this file will be removed", for people going commit-by-commit is weird to review something and it be removed next commit. |
Creates the direct run, a way to test running directly like the old test_runner.py, soon pytest will replace it, so its a temporary file.
Refact from the framework avoiding the python calling python paralelism and read the ports from log ideas, and made the nescessary configurations and fixtures for pytest in the conftest and node_manager file.
This commit add a customizable test framework (`--test-runner` for old test-framework and `--pytest` for new test-framework).
Refact the integrations tests, add example dir example/ for pytest.
feat: add arguments to tests/run.sh
Update tests/test_framework/__init__.py Co-authored-by: Shigeru Myamoto <[email protected]> Update tests/test_framework/__init__.py Co-authored-by: Shigeru Myamoto <[email protected]> Update tests/conftest.py Co-authored-by: qlrd <[email protected]> Update tests/test_framework/__init__.py Co-authored-by: Shigeru Myamoto <[email protected]>
fb8a6ec to
2a2e5b5
Compare
|

What is the purpose of this pull request?
Which crates are being modified?
Description and Notes
Summary
This PR replaces our homegrown functional testing harness with pytest and removes the need to scrape runtime data (like ports) from log files. The goal is to make tests reliable, faster, and easier to maintain without reinventing what mature tooling already provides.
Motivation
The custom test framework started to feel like we were rebuilding an existing wheel:
Approach
Why this helps
Goals
Scope and collaboration
This work was done with input and decision-making support from @jaoleal, and from @qlrd to understand the test framework. We intentionally did not refactor all tests at once; feedback from the team will help guide the remaining migration and any follow-up improvements.
How to run
Open questions and next steps
Feedback welcome—especially on migration strategy, fixture design, and CI defaults.