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

Make Tests Python 3.11 Compatible #34506

Merged
merged 6 commits into from
Apr 19, 2024
Merged

Conversation

feanil
Copy link
Contributor

@feanil feanil commented Apr 11, 2024

Multiple edx-platform Python tests fail when running on 3.11 Update those tests so that they will work correctly when we start testing on 3.11.

This PR does not introduce 3.11 testing because there are other costs and complexities to doing that so it is just making modifications to the tests so that they will be compatible with both versions of the code.

The goal is to land this ahead of the other parts of #34374 so that we can reduce the size of the breaking change PR.

  • fix: Be able to clear the process_cache manually in Python 3.11
  • fix: Provide a sequence to random.sample
  • fix: Create a bad unicode file differently.
  • fix: Fix function mocking.
  • fix: Don't use the deprecated location for Hashable
  • fix: Remove deprecated getargspec call.

@feanil feanil linked an issue Apr 11, 2024 that may be closed by this pull request
4 tasks
@feanil feanil requested a review from kdmccormick April 12, 2024 14:24
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and thanks for the helpful commit messages.

Copy link
Member

@UsamaSadiq UsamaSadiq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple suggestion, otherwise looking good to be merged.

xmodule/tests/xml/factories.py Outdated Show resolved Hide resolved
Given code like the following

```
class Foo:
    @process_cached
    def bar(self):
        pass
```

In Python 3.8 referencing `bar` would not call its `__get__` method.

```
x = Foo().bar
```

However in Python 3.11, making the same call would call the `__get__`
method, permanently replacing the underlying `process_cached` object
with the partial function that references it.

This meant that code to clear the cache would work in Python 3.8 but
would break in 3.11

```
Foo().bar.cache.clear()  # Works in 3.8 but not in 3.11
```

In 3.11 this results in the following error:
```
E       AttributeError: 'functools.partial' object has no attribute 'cache'
```

To make this compatible in both version, we just add the cache as an
accessible attribute on the partial we generate for our wrapped
function.
The sample function used to automatically convert sets to sequences but
that is no longer supported starting in 3.11 so we have to do it
manually.

Reference: https://docs.python.org/3/library/random.html#random.sample
In Python 3.11 CSV files are allowed to have null characters so the test
data is no longer a valid. We update it to not have a valid unicode
character to still test this code path correctly.

I'm not concerned about the fact that the files with null will get past
this test beacause there are other checks on the content of the file
that catch if it doesn't have enough or the right fields so this should
be a safe change to make to the tests.

Relevant Change in Python: python/cpython#71767
The way the patch decorator was being used is not supported in python
3.11.  Use the patch decorator to auto generate the correct mock and
make the test a bit more readabale.  The new change is both 3.8 and
3.11 compatible.
The Hashable object was moved in python 3.3 and support for the old
location is dropped in python 3.10 the new location is available in
python 3.8 so we can just update this and it should work with both
python 3.8 and 3.11

https://docs.python.org/3.8/library/collections.html
This function was removed by python 3.11 so update to the alternate
call that is the current recommended replacement.

https://docs.python.org/3.11/library/inspect.html#inspect.getfullargspec
@feanil feanil force-pushed the feanil/fix_tests_for_python_3.11 branch from 995036f to 6fb5963 Compare April 18, 2024 17:28
@feanil feanil merged commit a08b7de into master Apr 19, 2024
66 checks passed
@feanil feanil deleted the feanil/fix_tests_for_python_3.11 branch April 19, 2024 14:07
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

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

Successfully merging this pull request may close these issues.

[edx-platform] Test Python 3.11
4 participants