Skip to content

Add Python 3.12 to the test suite #764

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

Merged
merged 14 commits into from
Dec 22, 2022

Conversation

browniebroke
Copy link
Contributor

@browniebroke browniebroke commented Dec 21, 2022

Describe the changes

I know this is quite early 😄 but I was trying to test one of my packages to see if it worked and noticed that pyfakefs was returning some errors. I fixed a few issues to make it work on my machine (macOS) but it looks like it now fails due to pandas not installing yet. Also Windows fails but I don't have such a machine, so I might not be the best person to fix these...

Let me know how I should proceed with this. I understand if you want to close or leave this pull request open until all the jobs pass. I'm also happy to split the fixes and leave the CI updates out for now.

However, I do think that having basic support in this package might help others packages to add Python 3.12 compatibility.

Tasks

  • Unit tests added that reproduce the issue or prove feature is working
  • Fix or feature added
  • Entry to release notes added
  • Pre-commit CI shows no errors
  • Unit tests passing
  • For documentation changes: The Read the Docs preview builds and looks as expected

@@ -383,6 +385,7 @@ class _FakeWindowsFlavour(_FakeFlavour):
| {"COM%d" % i for i in range(1, 10)}
| {"LPT%d" % i for i in range(1, 10)}
)
pathmod = ntpath
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class attribute was present but seemingly unused before python/cpython#95450

@@ -632,6 +632,7 @@ def test_symlink_to(self):
self.assertTrue(path.is_symlink())

@unittest.skipIf(sys.version_info < (3, 8), "link_to new in Python 3.8")
@unittest.skipIf(sys.version_info >= (3, 12), "link_to removed in Python 3.12")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this had been deprecated before (actually renamed to hardlink_to).

@browniebroke
Copy link
Contributor Author

The Windows tests are failing because of this new feature that was added: python/cpython#99548

os.DirEntry now includes an os.DirEntry.is_junction method to check if the entry is a junction.

@browniebroke browniebroke marked this pull request as ready for review December 21, 2022 16:59
Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

Thank you for that, this is helpful! If you will exclude the extra requirement tests for Python 3.12, and add a release note (under Infrastructure), I will happily merge this.

@property
def is_junction(self) -> bool:
# TODO: implement junctions
return False
Copy link
Member

Choose a reason for hiding this comment

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

This should probably return `self.filesystem.is_junction(self.path).

experimental: true
- os: windows-latest
python-version: "3.12-dev"
experimental: true
Copy link
Member

Choose a reason for hiding this comment

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

I would probably just disable the tests with extra requirements for 3.12 (by adding a version check to the respective steps). It is common that larger packages do not support a new Python in early development stages. We can remove the check later.

TypeError: if path is None.
"""
# TODO: implement junction on Windows
return False
Copy link
Member

Choose a reason for hiding this comment

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

I think for the time being it is ok to just return False here - I would adapt the description accordingly, though. Later, we may add some convenience function to make a junction in pyfakefs - as far as I can see, this is not possible in Python yet. We also need to adapt os.stat, os.lstat and possibly other functions that will support junctions.

@@ -632,6 +632,7 @@ def test_symlink_to(self):
self.assertTrue(path.is_symlink())

@unittest.skipIf(sys.version_info < (3, 8), "link_to new in Python 3.8")
@unittest.skipIf(sys.version_info >= (3, 12), "link_to removed in Python 3.12")
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this had been deprecated before (actually renamed to hardlink_to).

@browniebroke
Copy link
Contributor Author

I'll do that tomorrow

@browniebroke browniebroke marked this pull request as draft December 22, 2022 10:10
TypeError: if path is None.
"""
# TODO: implement junction on Windows
"""Junction are never faked."""
Copy link
Member

Choose a reason for hiding this comment

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

Typo: Junction -> Junctions.
You may also add something like "Returns False - junctions are never faked."

@@ -70,17 +58,17 @@ jobs:
fi
shell: bash
- name: Install extra dependencies
if: ${{ ! matrix.experimental }}
if: ${{ python-version == "3.12-dev" }}
Copy link
Member

@mrbean-bremen mrbean-bremen Dec 22, 2022

Choose a reason for hiding this comment

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

Shouldn't that be != ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course 🤦🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The string literal must be written using single quotes: https://docs.github.com/en/actions/learn-github-actions/expressions#literals

@browniebroke browniebroke marked this pull request as ready for review December 22, 2022 10:26
@browniebroke
Copy link
Contributor Author

Do I need to update the documentation anywhere?

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

👍

@mrbean-bremen
Copy link
Member

Do I need to update the documentation anywhere?

No, there is no need. The API documentation is automatically generated, and at this stage I wouldn't announce support for Python 3.12 yet.

@mrbean-bremen mrbean-bremen merged commit 50a2761 into pytest-dev:main Dec 22, 2022
@browniebroke
Copy link
Contributor Author

The API documentation is automatically generated

Yes, I noticed that the new methods I've added aren't showing up because of the conditionals I added here since the docs aren't generated using Python 3.12. It's probably fine for now, but something to keep in mind when official support is added.

@browniebroke browniebroke deleted the upgrade/python-3.12 branch December 22, 2022 10:43
@mrbean-bremen
Copy link
Member

Good point, I wasn't aware of this.

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.

2 participants