oracle / oci-python-sdk

Oracle Cloud Infrastructure SDK for Python
https://cloud.oracle.com/cloud-infrastructure
Other
386 stars 277 forks source link

Fixture order not guaranteed (pytest < 3.5.0) #196

Open smarlowucf opened 4 years ago

smarlowucf commented 4 years ago

I realize there's a min pytest version specified in requirements but it seems to be incorrect. The tests seem to run fine at anything matching pytest >= 3.5.0. In 3.5.0 the order of fixture instantiation based on scope was added and this breaks the tests for anything older. However, there is an easy fix which I think is worthwhile and could allow the min pytest version to be lowered even further. Below is the patch diff of the changes that would be required:

Index: oci-python-sdk-2.6.2/tests/unit/test_basic_api_calls.py
===================================================================
--- oci-python-sdk-2.6.2.orig/tests/unit/test_basic_api_calls.py
+++ oci-python-sdk-2.6.2/tests/unit/test_basic_api_calls.py
@@ -5,7 +5,7 @@ import oci
 import pytest

-def test_identity_list_users(identity, config):
+def test_identity_list_users(config, identity):
     response = identity.list_users(config["tenancy"])

     assert response is not None
@@ -31,7 +31,7 @@ def test_vcn_list_instances(compute, con
     assert response.request_id is not None

-def test_limit(identity, config):
+def test_limit(config, identity):
     response = identity.list_users(config["tenancy"], limit=1)

     assert response is not None
Index: oci-python-sdk-2.6.2/tests/unit/test_waiters.py
===================================================================
--- oci-python-sdk-2.6.2.orig/tests/unit/test_waiters.py
+++ oci-python-sdk-2.6.2/tests/unit/test_waiters.py
@@ -108,7 +108,7 @@ def test_wait_multiple_states(virtual_ne
     assert total_time < 60 * 5

-def test_invalid_operation(identity, config):
+def test_invalid_operation(config, identity):
     # Create User
     request = oci.identity.models.CreateUserDetails()
     request.compartment_id = config["tenancy"]
@@ -131,7 +131,7 @@ def test_invalid_operation(identity, con
         oci.wait_until(identity, response, 'not a real property', 'test')

-def test_already_in_state(identity, config):
+def test_already_in_state(config, identity):
     description = 'test user'
     request = oci.identity.models.CreateUserDetails()
     request.compartment_id = config["tenancy"]
@@ -151,7 +151,7 @@ def test_already_in_state(identity, conf
     identity.delete_user(user_id)

-def test_wait_time_exceeded(identity, config):
+def test_wait_time_exceeded(config, identity):
     description = 'test user'
     request = oci.identity.models.CreateUserDetails()
     request.compartment_id = config["tenancy"]
@@ -182,7 +182,7 @@ def test_property_and_eval_function_prov
     assert str(ve.value) == 'If an evaluate_response function is provided, then the property argument cannot also be provided'

-def test_eval_function_lambda(identity, config):
+def test_eval_function_lambda(config, identity):
     user_id = None
     try:
         description = 'test user'
@@ -206,7 +206,7 @@ def test_eval_function_lambda(identity,
             identity.delete_user(user_id)

-def test_eval_function_func_ref(identity, config):
+def test_eval_function_func_ref(config, identity):
     user_id = None
     try:
         description = 'test user'

The fix is just changing the order of fixture imports to ensure the config fixture is always prior to identity. The config fixture skips the test if there's no config file available instead of failing the test.

pelliu commented 4 years ago

Hi @smarlowucf, thanks for letting us know the issue you are facing and how to work around the issue for it, the pytest version we are using is 4.1.0(listed in requirement.txt), and we haven't upgraded it for a while, in order to make sure all the features of oci-python-sdk work fine, we suggest you upgrade your python venv to the versions that requirement.txt listed, that would help resolve some potential issues, thanks.

smarlowucf commented 4 years ago

The issue here is related to RPM packaging. The pytest version cannot be upgraded in this case. Therefore the suggested patch has to be used. It seems simple enough that it wouldn't cause issues to make the minor changes to the tests? The advantage is supporting more environments.

pelliu commented 4 years ago

@smarlowucf , I am trying to understand is there any reason that we can't upgrade pytest version? are you saying it's your system other dev tools' dependency caused? Thanks!

glaubitz commented 4 years ago

@pelliu We are shipping the OCI SDK and CLI as part of our enterprise distributions.

We can only provide support to paying customers if they are using the software we have packaged. It's not practical to provide support for software that users install from third party sources and hence we don't do that.

smarlowucf commented 4 years ago

Hi @pelliu , Our use case or goal is to package the oci-sdk for a specific enterprise Linux distribution. This is to provide a package that can be installed via a system package manager as opposed to PyPI. These distributions have set packages which can only be upgraded for CVE's or security updates. For example the distribution this patch is for is set to pytest == 3.3.2. This distribution will be supported for many years and pytest will never be updated >= 3.4.0. This is why we required the patch I mentioned above.

In packaging oci-sdk we are required to run any test suite that is available to ensure that the package installs and runs inside the given distribution. This means the test suite has to run against the distributions packages and not in a virtual environment. The tests are run using pytest == 3.3.2 because that is the package that was delivered and is supported for the lifecyle of the distribution.

I hope this helps clarify our use case? We can maintain a patch downstream but ideally it is helpful if possible to get the patch implemented into the latest code base. This would not only help us but also any other Linux distributions that want to provide a supported oci-sdk system package in older releases.

jasonyin commented 4 years ago

hi @smarlowucf @glaubitz, thanks for reporting the issue.

In general, we are not able to support all released versions of pytest, they might have breaking changes, security updates etc. One common practice is only for two major versions. PyTest 3.3.2 was released on 2017-12-25 which is pretty old one. And it's now on 5.x version.

I understand your use cases. From the log you posted, looks like it's all related with the order of tests inputs which happens to be integration tests (test call the real services). @pelliu , I think we should remove these tests publicly and only release out unit tests.

@smarlowucf , @garthy is the log you posted are complete changes which breaks for you? Please let me would that suggestion work for you or not? BTW, run full unit tests should be good in terms of validating the installation.

This might be able to fix the issue you mentioned, but you might see other issues in the future in terms of mis-matched third party dependencies. So I am wondering:

  1. are you going to vendor the 2.6.2 into distribution once or you are updating the python sdk version on a regular basis;
  2. how do you update the python sdk version and it's dependencies? Install locally and push to a repository?
smarlowucf commented 4 years ago

Hi,

is the log you posted are complete changes which breaks for you?

Yes, the diff I provided fixes the issues we are seeing. I've tested using latest Pytest versions with that diff and don't see any issues so it's a non-breaking change.

1. are you going to vendor the 2.6.2 into distribution once or you are updating the python sdk version on a regular basis;

Not sure I fully understand the question but in packaging for a distribution we try to avoid vendored dependencies. For example if a version of requests is vendored inside another package we want to avoid this because it's not testable on it's own and it is a security concern.

Using OCI SDK as an example which has a vendored dependency on requests. We have to remove the vendored requests code during packaging and make changes for the SDK to use the version of requests that is distributed.

This is required by SLA to ensure that both the oci-sdk package and the requests package are properly reviewed, tested and can be updated independent of each other.

2\. how do you update the python sdk version and it's dependencies? Install locally and push to a repository?

When a package has a new version we pull down a new tarball of the code from either GitHub or PyPI and make any necessary changes to the RPM specification. Normally the change is just a version bump but it might require an update to dependency lists, new patches or other settings. The package is built and tested against each distribution using a build service. Once the new package is confirmed to be in working condition it's submitted for review. When review passes the package is accepted and can be installed/updated for that distribution. E.g. in openSUSE to install the oci-sdk the command would look like:

zypper install python3-oci-sdk

Hope this helps.

jasonyin commented 4 years ago

hi @smarlowucf thanks for the updates.

@pelliu would the change proposed by @smarlowucf work for our version of pytest? If yes for both, we can consider to make that change to unblock our customer and at same time we still keep existing pytest version, thoughts?