-
Notifications
You must be signed in to change notification settings - Fork 97
remove Python 3.9 and add support for Python 3.14 #1270
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
Conversation
| with pytest.warns(None) as recorded_warnings: | ||
| with warnings.catch_warnings() as recorded_warnings: | ||
| warnings.simplefilter("error") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was made because Python 3.14 does not allow the type None in pytest.warns() (TypeError: exceptions must be derived from Warning, not <class 'NoneType'>)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, took me a while, but I get what's going on now.
I was a bit confused by warnings.simplefilter("error"), but that's turning each warning into an exception, so you no longer need to check the length of the warnings.
tests/utils/test_table_reflection.py
Outdated
|
|
||
| def test_reflect_all_tables(self): | ||
| run_sync(self.table_storage.reflect()) | ||
| run_sync(self.table_storage.reflect(exclude=["migration"])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason in Python 3.14 (which I don't understand) the migration table is included in the table storage and the tests fail. I exclude the migration table from table storage to prevent this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, interesting. I'll try it locally. I wonder what changed in Python 3.14 to cause this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me locally. Maybe your test database has a stale migration table or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK - I can see it failing now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I just run that test by itself, it's OK. But if I run the entire test suite then it fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be to do with the newer version of Pytest.
In conftest.py we have drop_tables which is meant to clean up these tables. I wonder if that's not running. Worth investigating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked, and drop_tables does get run before the tests run. So the problem is some previous test is creating the migration table but not cleaning it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I just run that test by itself, it's OK. But if I run the entire test suite then it fails.
That's exactly what's happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is a good idea, but we can force the migration table drop in the test setup just to make sure that migration table is dropped. This works in Py3.14 and Py3.12. Something like this
from piccolo.apps.migrations.commands.base import Migration
@engines_only("postgres", "cockroach")
class TestTableStorage(TestCase):
def setUp(self) -> None:
Migration.raw("DROP TABLE IF EXISTS migration").run_sync() # here
self.table_storage = TableStorage()
for table_class in (Manager, Band):
table_class.create_table().run_sync()There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sinisaos Yes, that works.
I managed to figure out which tests were leaving the migration table in the database. I added this auto fixture to conftest.py:
@pytest.fixture(autouse=True)
def migration_table_detector():
yield
response = asyncio.run(
ENGINE._run_in_new_connection(
"SELECT * FROM information_schema.tables WHERE table_name = 'migration'"
)
)
if len(response) > 0:
breakpoint()There were two tests which ran migration commands, and the migration table was being created.
Maybe in the future we can run each test in a transaction to avoid these kinds of problems, but not a priority right now.
I've been able to fix the relevant tests.
|
I've made a fresh virtual env using Python 3.14, but can't install some of our pip dependencies. Will try again tomorrow. |
|
@dantownsend Strange. Now I'm cloned the branch in fresh virtualenv and everything works in my case. Which OS do you use? I use |
|
Looks good, thanks! 👍 |
Related to #1264