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

pytest 8.2.0 runs unittest.case.TestCase.__init__ during collecting phase in the same process #12289

Closed
jiasli opened this issue May 6, 2024 · 8 comments

Comments

@jiasli
Copy link

jiasli commented May 6, 2024

Original issue: Azure/azure-cli#28848

By injecting

        import os
        print('__init__', os.getpid(), method_name)

at https://github.com/Azure/azure-cli/blob/2750c65ea77638ecf699243555a5f2f7a1314ed4/src/azure-cli-testsdk/azure/cli/testsdk/base.py#L86, we can see different versions of pytest show different output:

With pytest 8.1.1:

$ pip install pytest==8.1.1

$ python -m pytest -x -v --forked --log-level=WARN /home/user2/azure-cli/src/azure-cli/azure/cli/command_modules/containerapp/tests/latest/test_containerappjob_with_secrets.py::ContainerAppJobsSecretsOperationsTest::test_containerapp_manualjob_withsecret_crudoperations_e2e /home/user2/azure-cli/src/azure-cli/azure/cli/command_modules/hdinsight/tests/latest/test_hdinsight_commands.py::HDInsightClusterTests::test_hdinsight_application -rP
============================================== test session starts ===============================================
platform linux -- Python 3.10.12, pytest-8.1.1, pluggy-1.5.0 -- /home/user2/py310/bin/python
cachedir: .pytest_cache
rootdir: /home/user2/azure-cli/src/azure-cli
plugins: xdist-3.5.0, forked-1.6.0
collected 2 items

src/azure-cli/azure/cli/command_modules/containerapp/tests/latest/test_containerappjob_with_secrets.py::ContainerAppJobsSecretsOperationsTest::test_containerapp_manualjob_withsecret_crudoperations_e2e PASSED [ 50%]
src/azure-cli/azure/cli/command_modules/hdinsight/tests/latest/test_hdinsight_commands.py::HDInsightClusterTests::test_hdinsight_application PASSED [100%]

===================================================== PASSES =====================================================
________ ContainerAppJobsSecretsOperationsTest.test_containerapp_manualjob_withsecret_crudoperations_e2e _________
--------------------------------------------- Captured stdout setup ----------------------------------------------
__init__ 1971704 test_containerapp_manualjob_withsecret_crudoperations_e2e
---------------------------------------------- Captured stderr call ----------------------------------------------

----------------------------------------------- Captured log call ------------------------------------------------
WARNING  cli.azure.cli.command_modules.containerapp.custom:custom.py:3233 Containerapp job 'job1000002' executions triggered now will have the added/updated secret.
WARNING  cli.azure.cli.command_modules.containerapp.custom:custom.py:3233 Containerapp job 'job1000002' executions triggered now will have the added/updated secret.
WARNING  cli.azure.cli.command_modules.containerapp.custom:custom.py:3194 Secret(s) successfully removed.
________________________________ HDInsightClusterTests.test_hdinsight_application ________________________________
--------------------------------------------- Captured stdout setup ----------------------------------------------
__init__ 1971712 test_hdinsight_application
----------------------------------------------- Captured log call ------------------------------------------------
WARNING  msrest.serialization:serialization.py:187 Readonly attribute marketplace_identifier will be ignored in class <class 'azure.mgmt.hdinsight.models._models_py3.ApplicationProperties'>
=============================================== 2 passed in 2.88s ================================================

With pytest 8.2.0:

$ pip install pytest==8.2.0

$ python -m pytest -x -v --forked --log-level=WARN /home/user2/azure-cli/src/azure-cli/azure/cli/command_modules/containerapp/tests/latest/test_containerappjob_with_secrets.py::ContainerAppJobsSecretsOperationsTest::test_containerapp_manualjob_withsecret_crudoperations_e2e /home/user2/azure-cli/src/azure-cli/azure/cli/command_modules/hdinsight/tests/latest/test_hdinsight_commands.py::HDInsightClusterTests::test_hdinsight_application -rP
============================================== test session starts ===============================================
platform linux -- Python 3.10.12, pytest-8.2.0, pluggy-1.5.0 -- /home/user2/py310/bin/python
cachedir: .pytest_cache
rootdir: /home/user2/azure-cli/src/azure-cli
plugins: xdist-3.5.0, forked-1.6.0
collecting ... __init__ 1971868 runTest
__init__ 1971868 test_containerapp_manualjob_withsecret_crudoperations_e2e
__init__ 1971868 runTest
__init__ 1971868 test_hdinsight_application
__init__ 1971868 test_hdinsight_autoscale_operation
__init__ 1971868 test_hdinsight_azure_monitor
__init__ 1971868 test_hdinsight_cluster_kafka
__init__ 1971868 test_hdinsight_cluster_kafka_with_optional_disk_args
__init__ 1971868 test_hdinsight_cluster_kafka_with_rest_proxy
__init__ 1971868 test_hdinsight_cluster_min_args
__init__ 1971868 test_hdinsight_cluster_resize
__init__ 1971868 test_hdinsight_cluster_with_availability_zones
__init__ 1971868 test_hdinsight_cluster_with_cluster_config
__init__ 1971868 test_hdinsight_cluster_with_component_version
__init__ 1971868 test_hdinsight_cluster_with_compute_isolation
__init__ 1971868 test_hdinsight_cluster_with_encryption_at_host
__init__ 1971868 test_hdinsight_cluster_with_encryption_in_transit
__init__ 1971868 test_hdinsight_cluster_with_loadbased_autoscale
__init__ 1971868 test_hdinsight_cluster_with_minimal_tls_version
__init__ 1971868 test_hdinsight_cluster_with_private_link_configurations
__init__ 1971868 test_hdinsight_cluster_with_schedulebased_autoscale
__init__ 1971868 test_hdinsight_cluster_with_ssh_creds
__init__ 1971868 test_hdinsight_monitor
__init__ 1971868 test_hdinsight_script_action
__init__ 1971868 test_hdinsight_usage
collected 2 items

src/azure-cli/azure/cli/command_modules/containerapp/tests/latest/test_containerappjob_with_secrets.py::ContainerAppJobsSecretsOperationsTest::test_containerapp_manualjob_withsecret_crudoperations_e2e PASSED [ 50%]
src/azure-cli/azure/cli/command_modules/hdinsight/tests/latest/test_hdinsight_commands.py::HDInsightClusterTests::test_hdinsight_application PASSED [100%]

===================================================== PASSES =====================================================
________ ContainerAppJobsSecretsOperationsTest.test_containerapp_manualjob_withsecret_crudoperations_e2e _________
---------------------------------------------- Captured stderr call ----------------------------------------------

----------------------------------------------- Captured log call ------------------------------------------------
WARNING  cli.azure.cli.command_modules.containerapp.custom:custom.py:3233 Containerapp job 'job1000002' executions triggered now will have the added/updated secret.
WARNING  cli.azure.cli.command_modules.containerapp.custom:custom.py:3233 Containerapp job 'job1000002' executions triggered now will have the added/updated secret.
WARNING  cli.azure.cli.command_modules.containerapp.custom:custom.py:3194 Secret(s) successfully removed.
________________________________ HDInsightClusterTests.test_hdinsight_application ________________________________
----------------------------------------------- Captured log call ------------------------------------------------
WARNING  msrest.serialization:serialization.py:187 Readonly attribute marketplace_identifier will be ignored in class <class 'azure.mgmt.hdinsight.models._models_py3.ApplicationProperties'>
=============================================== 2 passed in 3.63s ================================================

This indicates pytest 8.2.0:

  1. runs extra unittest.case.TestCase.__init__ for runTest and test methods that are not even invoked
    • This slows test down as extra code is executed
  2. runs unittest.case.TestCase.__init__ during collecting phase in the same process, instead of forked processes

System info:

$ cat /etc/os-release
PRETTY_NAME="Ubuntu 22.04.4 LTS"
NAME="Ubuntu"
VERSION_ID="22.04"
VERSION="22.04.4 LTS (Jammy Jellyfish)"
VERSION_CODENAME=jammy
ID=ubuntu
ID_LIKE=debian
HOME_URL="https://www.ubuntu.com/"
SUPPORT_URL="https://help.ubuntu.com/"
BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"
PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"
UBUNTU_CODENAME=jammy

$ pip list | grep pytest
pytest                                  8.2.0
pytest-forked                           1.6.0
pytest-xdist                            3.5.0
@bluetech
Copy link
Member

bluetech commented May 6, 2024

Is there a reason you are doing these things in __init__ rather than in setUp or similar?

@jiasli
Copy link
Author

jiasli commented May 7, 2024

Thanks for the response.

It is true that we can move our logic from __init__ into setUp, and this is exactly what I am experimenting in Azure/azure-cli#28849. However, the code has been there for years and changing it will need some effort.

I just would like to confirm if this is an expected change of pytest 8.2.0 as it somehow seems to be a breaking change. If so, I guess it is worthing mentioning it in the change log https://docs.pytest.org/en/8.2.x/changelog.html. 😊

@RonnyPfannschmidt
Copy link
Member

Its important to note that adding setup code to the constructor is always completely wrong for testcase subclasses

So your code was always incorrect

It simply didn't trigger before by accident

@jiasli
Copy link
Author

jiasli commented May 7, 2024

Thanks a lot @RonnyPfannschmidt for the suggestion. I will make sure to move the logic into setUp.

@jiasli jiasli closed this as completed May 7, 2024
@bluetech
Copy link
Member

bluetech commented May 7, 2024

I just would like to confirm if this is an expected change of pytest 8.2.0 as it somehow seems to be a breaking change. If so, I guess it is worthing mentioning it in the change log https://docs.pytest.org/en/8.2.x/changelog.html. 😊

Yes it was an expected change though I didn't anticipate the issues and didn't consider it to be a breaking change (otherwise I wouldn't have done it in a minor release). I will definitely amend the release notes, I just haven't got to it yet.

@RonnyPfannschmidt
Copy link
Member

As unittest itself collects instances,
Having setup code in the ctor is rightfully unexpected

@DaveyBiggers
Copy link

Its important to note that adding setup code to the constructor is always completely wrong for testcase subclasses

So your code was always incorrect

It simply didn't trigger before by accident

Sorry to comment on a closed issue, but can you clarify this? Why is it wrong to add code to __init__() ? I couldn't see any obvious mention of this in any documentation, but maybe I've been looking in the wrong place. (We're having some strange issues since updating pytest whereby parent class methods are getting called, instead of the overridden subclass's methods, and I'm wondering if it's somehow related.)

@RonnyPfannschmidt
Copy link
Member

Unittest testcase instances are used for collection,also in unittest

So running setup at that point in time is just incorrect

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

4 participants