Skip to content
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

Suggest Migrating Python Tests to pytest in Favor of Current tests/test_all.py #2944

Open
darwintree opened this issue Nov 6, 2024 · 4 comments

Comments

@darwintree
Copy link
Contributor

Why

There is some disadvantages for current test framework, for example

  • One single TestClass can only be tested sequentially. If multiple cases need to be tested in one class, their order should be carefully arranged.
  • Not convenient to debug single test or run a bunch of tests.

The above disadvantages can be resolved by the pytest framework, and popular ides such as vs code already provided gui so debugging or a group of tests can be run with convinenience.

It would also bring convience as we can make use of pytest fixture or plugins to simplify the development

How this can be done

Create a new folder such as tests_new and the migrated tests will be placed in tests_new folder. With every test migrated, it will be removed from tests folder. During the migration, both tests in tests and tests_all will be executed. In this manner, the changes are easy to be reviewed and won't affect existing tests or pending pull requests.

Implementation details

  • pytest plugin pytest-xdist(parallel) and pytest-order(test order) will be adopted to implement the features done by test_all.py
  • Current ConfluxTestFramework will be refactored, the main() function will be tear down and run_test will be removed. It will not be directly used as parent class of TestClass but still be used to setup the environment.
  • More type hints will be added
@ChenxingLi
Copy link
Contributor

About Motivation

Test Parallelization

For test parallelization, there are several distinct scenarios:

  1. Different Node Setup Parameters
  • Tests with varying configurations
  • Similar test procedures across nodes
  1. Fresh Environment Requirements
  • Identical node setup parameters
  • Each test case requires a clean testing environment
  1. Same-Node Parallelization
  • Identical node setup parameters
  • Test cases can run concurrently on the same node

For the third scenario, implementing thread pools within the current framework could be a simpler alternative to a complete framework migration.

While I've only encountered the first two scenarios once in my experience, we should further discuss specific implementation approaches if these use cases are relevant to your needs.

Current Framework Usability for Debugging Single Tests

I think the current framework is reasonably accessible: since individual tests can be run via python tests/xxx_test.py, this allows for straightforward test class execution

Recommendation

Let's evaluate pytest migration necessity based on specific testing requirements. If needed, we could start with migrating RPC and VM layer tests first.

About Implementation Considerations

Beyond invoking the test scripts, test_all.py handles additional functions like test index assignment and port allocation for independent runs, among other essential features.

So before considering migration, we should thoroughly review the logic flow from the main function of test_all.py to run_single_test and ensure pytest can effectively replicate these capabilities.

@darwintree
Copy link
Contributor Author

About Motivation

Test Parallelization

For test parallelization, there are several distinct scenarios:

  1. Different Node Setup Parameters
  • Tests with varying configurations
  • Similar test procedures across nodes
  1. Fresh Environment Requirements
  • Identical node setup parameters
  • Each test case requires a clean testing environment
  1. Same-Node Parallelization
  • Identical node setup parameters
  • Test cases can run concurrently on the same node

For the third scenario, implementing thread pools within the current framework could be a simpler alternative to a complete framework migration.

While I've only encountered the first two scenarios once in my experience, we should further discuss specific implementation approaches if these use cases are relevant to your needs.

Current Framework Usability for Debugging Single Tests

I think the current framework is reasonably accessible: since individual tests can be run via python tests/xxx_test.py, this allows for straightforward test class execution

Recommendation

Let's evaluate pytest migration necessity based on specific testing requirements. If needed, we could start with migrating RPC and VM layer tests first.

About Implementation Considerations

Beyond invoking the test scripts, test_all.py handles additional functions like test index assignment and port allocation for independent runs, among other essential features.

So before considering migration, we should thoroughly review the logic flow from the main function of test_all.py to run_single_test and ensure pytest can effectively replicate these capabilities.

I would first explain for what pytest can do in the specific scenerios described above, and then discuss the necessity of migration.

Scenarios

Different Node Setup Parameters

  1. Different Node Setup Parameters
  • Tests with varying configurations
  • Similar test procedures across nodes

pytest's @pytest.mark.parametrize wrapper can be used. This is designed for the described scenerio.

Fresh Environment Requirements

  1. Fresh Environment Requirements
  • Identical node setup parameters
  • Each test case requires a clean testing environment

pytest's fixture scope is designed for this scenerio. Fixture in pytest means something used by the test. I intend to refactor current test framework into a fixture. And here is the pseudo code in which case a new env will be created for every test:

@pytest.fixture(scope="function")
def new_env() -> ConfluxTestFramework:
    ......  #  set up env

def test_CIP_123(new_env: ConfluxTestFramework):
    w3 = Web3(new_env.url)
    ......
    
def test_CIP_456(new_env: ConfluxTestFramework):
    w3 = Web3(new_env.url)
    ......

Same-Node Parallelization

  1. Same-Node Parallelization
  • Identical node setup parameters
  • Test cases can run concurrently on the same node

This would be something tricky. But I would first know the reason of the the situation setting.

  1. If the concurrency is what need to be tested, then this scenario can be considered as a single test case with multiple sub-tests run concurrently. Using asyncio to collect results of sub-functions in a single test might be more reasonable. I think some pytest plugins can be found to make this easier.
  2. If this is intended to speedup the test, it will be something tricky. pytest-dist sends tests to different workers for parallelization, and different workers won't share same fixture. It means fixture cannot be directly used to setup the environment. It can still be overcome as we can setup the env outside python code.

test index assignment

I think this suggests the slow_tests

    slow_tests = {"full_node_tests/p2p_era_test.py", "pos/retire_param_hard_fork_test.py"}

pytest-order can help for the execution index (https://pytest-order.readthedocs.io/en/latest/configuration.html#order-scope)

port allocation

Within each test, the worker_id can be got from fixture(https://pytest-xdist.readthedocs.io/en/stable/how-to.html#identifying-the-worker-process-during-a-test). And it can be used to allocate port

@pytest.fixture()
def port(worker_id):
    # worker_id is "master" or "gw0", "gw1", ......
    return 12537 if worker_id == "master" else 12537+int(worker_id[2:])

Necessity

From my end, I think the "necessity" in migratioin can be better described as whether the cost of migration is worth the convenience pytest would bring. And there is also possibility that pytest cannot cover current framework's feature.

I think it is hard to tell the answer before actual coding. And several local changes and I found the migration may be somewhat easier than I expected. I think I can first create a PR based on several RPC tests, based on which we can better judge whether it is necessary for migration

@ChenxingLi
Copy link
Contributor

I believe pytest can meet several parallel testing requirements. I'm listing three parallel scenarios because in your initial introduction, the migration to pytest was motivated by parallelization needs.

One single TestClass can only be tested sequentially. If multiple cases need to be tested in one class, their order should be carefully arranged.

I want to discuss what kind of parallel requirements cannot be met by existing frameworks but can be satisfied by pytest.

Furthermore, I'm against code migration without clear motivation. For test code, there are currently several more valuable things worth doing:

  1. The current test framework can only run on Python <=3.9. However, the support for 3.9 will end in less than a year. We can't upgrade to higher versions because some underlying libraries changed their APIs in 3.10. This isn't difficult to fix; it's just a matter of finding time.

  2. Execution layer testing lacks a good code framework that would encourage developers to write more comprehensive and refined tests. Many contract calls require each unit test to implement transaction construction, signing, and receipt handling internally. This has led to a function called call_function being copied everywhere. While Introduce Contract Call Scaffolding for Conflux Test Framework #2717 provided a scaffold for the core space, there isn't one for espace.

@darwintree
Copy link
Contributor Author

I believe pytest can meet several parallel testing requirements. I'm listing three parallel scenarios because in your initial introduction, the migration to pytest was motivated by parallelization needs.

One single TestClass can only be tested sequentially. If multiple cases need to be tested in one class, their order should be carefully arranged.

I want to discuss what kind of parallel requirements cannot be met by existing frameworks but can be satisfied by pytest.

Furthermore, I'm against code migration without clear motivation. For test code, there are currently several more valuable things worth doing:

  1. The current test framework can only run on Python <=3.9. However, the support for 3.9 will end in less than a year. We can't upgrade to higher versions because some underlying libraries changed their APIs in 3.10. This isn't difficult to fix; it's just a matter of finding time.
  2. Execution layer testing lacks a good code framework that would encourage developers to write more comprehensive and refined tests. Many contract calls require each unit test to implement transaction construction, signing, and receipt handling internally. This has led to a function called call_function being copied everywhere. While Introduce Contract Call Scaffolding for Conflux Test Framework #2717 provided a scaffold for the core space, there isn't one for espace.

Sorry for that I didn't express myself clearly. Overall I think there is not such a scenario which pytest can do but current test framework cannot. The aim of the migration is convenience of easier test setup and simpler development.

One single TestClass can only be tested sequentially. If multiple cases need to be tested in one class, their order should be carefully arranged.

I want to convey that, in a single TestClass, there might be many test cases, just like most tests under the rpc folder. And pytest can help to organize the TestClass and sub test cases in a better way, including clear setup, gui support, and each sub test can be tested individually.

I agree the listed other 2 issues are of higher priority, and I can help resolving the mentioned 2 issues. Regarding the test framework, I would not insisit on migrating to pytest if you are agaist this approach. Some other more mild methods can also be adopted to improve the usability of current framework and I think we can discuss it in other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants