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.

Sign up for GitHub

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

Jump to bottom

gh-122666: Tests for ast optimizations #122667

Merged
merged 24 commits into from
Aug 26, 2024
Merged

gh-122666: Tests for ast optimizations #122667

merged 24 commits into from
Aug 26, 2024

Conversation

Eclips4
Copy link
Member

@Eclips4 Eclips4 commented Aug 4, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Aug 4, 2024
Closed
@Eclips4
Copy link
Member Author

Eclips4 commented Aug 4, 2024

Hypothesis failure looks unrelated.

skirpichev
skirpichev reviewed Aug 5, 2024
View reviewed changes
Lib/test/test_ast/test_ast.py Outdated Show resolved Hide resolved
Lib/test/test_ast/test_ast.py Outdated Show resolved Hide resolved
Eclips4 and others added 2 commits August 5, 2024 07:32
@Eclips4 @skirpichev
Update Lib/test/test_ast/test_ast.py
07919e9 
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@Eclips4 @skirpichev
Update Lib/test/test_ast/test_ast.py
ec7062c 
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@Eclips4 Eclips4 requested a review from vstinner August 5, 2024 13:45
@Eclips4 Eclips4 marked this pull request as draft August 5, 2024 14:07
vstinner
vstinner reviewed Aug 5, 2024
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I proposed to write "Lines below are generated by running (...)" into a separated file, not move eval_opt_tests into a separated file. Would it be possible?

@Eclips4 Eclips4 marked this pull request as ready for review August 8, 2024 18:44
Eclips4
Eclips4 commented Aug 8, 2024
View reviewed changes
Lib/test/test_ast/snippets.py
@@ -380,7 +380,6 @@ def main():
print("]")
print("main()")
raise SystemExit
unittest.main()
Copy link
Member Author

Choose a reason for hiding this comment

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

This line is no longer needed since there's no unittest in snippets.py

@wrongnull wrongnull mentioned this pull request Aug 17, 2024
Closed
vstinner
vstinner approved these changes Aug 26, 2024
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@JelleZijlstra: Do you want to review this change?

JelleZijlstra
JelleZijlstra reviewed Aug 26, 2024
View reviewed changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

There are a few more optimizations this doesn't test. We could keep them for another PR if you want to, but they don't seem difficult to test:

  • not (a is b) -> a is not b
  • not (a in b) -> a not in b
  • (1,) * 3 -> (1, 1, 1)
  • It seems that in some cases it changes % formatting into f-strings but I haven't been able to trigger that
  • __debug__ gets turned into True or False

Lib/test/test_ast/test_ast.py Outdated Show resolved Hide resolved
Lib/test/test_ast/test_ast.py Outdated Show resolved Hide resolved
Lib/test/test_ast/test_ast.py Outdated Show resolved Hide resolved
Lib/test/test_ast/test_ast.py Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra mentioned this pull request Aug 26, 2024
Closed
@Eclips4 @JelleZijlstra
Apply suggestions from code review
8bbbc5c 
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@Eclips4
Copy link
Member Author

Eclips4 commented Aug 26, 2024

__debug__ gets turned into True or False

This is already tested by test_optimization_levels__debug__

@JelleZijlstra JelleZijlstra enabled auto-merge (squash) August 26, 2024 19:18
@Eclips4
Copy link
Member Author

Eclips4 commented Aug 26, 2024

@JelleZijlstra do you think it's worth backporting this to the 3.13 branch?
I think so, after reading #123344.

@JelleZijlstra
Copy link
Member

Yes, it's good to backport new tests in general so we can be more confident in bugfixes.

@JelleZijlstra JelleZijlstra added the needs backport to 3.13 bugs and security fixes label Aug 26, 2024
@JelleZijlstra JelleZijlstra merged commit 9f9b00d into python:main Aug 26, 2024
33 checks passed
@miss-islington-app
Copy link

Thanks @Eclips4 for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 26, 2024
@Eclips4 @skirpichev
@vstinner @JelleZijlstra @miss-islington
pythongh-122666: Tests for ast optimizations (pythonGH-122667)
64c6cc0 
(cherry picked from commit 9f9b00d)

Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Aug 26, 2024

GH-123359 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Aug 26, 2024
@Eclips4 Eclips4 deleted the tests_for_ast_opt branch August 26, 2024 19:28
@Eclips4
Copy link
Member Author

Eclips4 commented Aug 26, 2024 •
edited
Loading

Yes, it's good to backport new tests in general so we can be more confident in bugfixes.

Oh, I forgot about 3.12 branch. I think it's also ok to backport it to 3.12. What do you think?

@JelleZijlstra
Copy link
Member

We can't because the optimize=1 argument didn't exist in 3.12. Maybe there's a way to make it work anyway, but not sure it's worth the effort.

@Eclips4
Copy link
Member Author

Eclips4 commented Aug 26, 2024

We can't because the optimize=1 argument didn't exist in 3.12. Maybe there's a way to make it work anyway, but not sure it's worth the effort.

Oh, yeah, you're right. I agree that it's not worth the effort.

JelleZijlstra added a commit that referenced this pull request Aug 27, 2024
@miss-islington @Eclips4
@skirpichev @vstinner @JelleZijlstra
[3.13] gh-122666: Tests for ast optimizations (GH-122667) (#123359)
69b104a 
(cherry picked from commit 9f9b00d)

Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skirpichev skirpichev skirpichev left review comments

@vstinner vstinner vstinner approved these changes

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@iritkatriel iritkatriel Awaiting requested review from iritkatriel

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Eclips4 @JelleZijlstra @vstinner @iritkatriel @skirpichev