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

Fix tests so they work with Python 3.12/3.13 #227

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

juliangilbey
Copy link

Hi!

There are quite a number of tests that fail with Python 3.12 and some also with Python 3.13. Most of them are due to the error messages produced by Python 3.12 and/or Python 3.13 being different from those in earlier versions of Python, and this (very ugly) patch fixes the tests to handle this.

More complicated is that there are some strings that now compile fine in Python 3.13 but fail on evaluation, specifically 'from .__future__ import whatever' and "f'{1:{5:{3}}}'". For now, this patch runs an eval() on the compiled string if it successfully compiles to catch these two cases, but it may be better for parso to accept these strings with Python 3.13.

Finally, f-strings can include backslashes in Python 3.12+, so the string r'''f"{'\n'}"''' is syntactically valid in these Python versions. I've removed this case from the list in failing_examples.py; parso should probably start accepting this when parsing Python 3.12+ (in parso/python/errors.py), but I have not attempted to do so - I cannot see how the functions in this file would know the version of Python being parsed.

@juliangilbey
Copy link
Author

juliangilbey commented Nov 21, 2024

I just refactored the patch slightly to make flake8 happy. It also removes loads of the elif conditionals, so reads better too, which is a bonus. I assume the slight drop in coverage is because I have removed the test for backslashes in f-strings, as noted above.

@davidhalter
Copy link
Owner

Thanks a lot @juliangilbey

I agree with your analysis. Have you not added the 3.12/3.13 builds to the build pipeline for some reason or did you just not think about it? I'm happy to do that eventually myself, but I thought that it might make sense in this pull request. Happy to help out if there are issues.

I like your changes a lot. The test code is really smelly and I don't mind deleting certain test cases. These tests worked really well for Python <3.10, but a lot has changed since then and they don't match the Python implementation very well anymore. So feel free to change functionality as well and allow certain things that are allowed in newer Python versions. IMO being correct about older versions is not as being correct in newer versions of Python.

@davidhalter
Copy link
Owner

Also: Feel free to do further changes, if you think they make sense. I trust you. (Maybe in a separate commit to be able to have a proper diff from what you did here, because I like what you have done.

@juliangilbey
Copy link
Author

Hi Dave,

I just didn't think to add the newer versions of Python to the test matrix, but I certainly could! But I'm not sure about these cases where Python 3.13 (and 3.12 in one case) compile correctly but fail at evaluation time: how should parso handle these?

I haven't looked into the internals of parso, but I'm sadly not going to have time to do much more in the near future :(

On a quick look, I did notice that the default version in utils.py is 3.6; that could perhaps be bumped a bit?

Best wishes, Julian

@juliangilbey
Copy link
Author

Just one more thought for now: it seems that the reason that these tests are becoming so complex is that we're fixing the problem in the wrong place. What I think should happen (though this is certainly more work) is that parso/python/errors.py is modified to give the correct error message for the Python version being parsed.

@juliangilbey
Copy link
Author

I just had a better idea for how to handle the cases that compile correctly in later versions of Python. I'll try to implement it.

@juliangilbey
Copy link
Author

I notice that the build.yml script is using the quite old actions/checkout@v2 and actions/setup-python@v2; these are now at v4 and v5 respectively. But I won't change that in this PR, as it is unrelated.

@davidhalter
Copy link
Owner

davidhalter commented Nov 22, 2024

Thanks a lot for the changes! Really appreciated.

Feel free to change actions/checkout@v2, etc. as well. I generally don't mind if a PR is not perfect, as long as I like the changes.

I hope I didn't merge too early.

@davidhalter davidhalter merged commit 9ddffca into davidhalter:master Nov 22, 2024
10 checks passed
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