10000 test_builtin.py test_compile unit test fix by hydrogen602 · Pull Request #5251 · RustPython/RustPython · GitHub
[go: up one dir, main page]
Skip to content

test_builtin.py test_compile unit test fix #5251

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
Merged
merged 7 commits into from
Apr 23, 2024
Merged

test_builtin.py test_compile unit test fix #5251

merged 7 commits into from
Apr 23, 2024

Conversation

hydrogen602
Copy link
Contributor
@hydrogen602 hydrogen602 commented Apr 21, 2024 •
edited
Loading

Code changes to make the test BuiltinTest.test_compile in test_builtin.py work.

I've made a draft PR as this unit test has multiple different incompatibility issues that all need to be fixed for this to fully work, and so feedback can be given on the first issues while I work on the latter ones.

Issues:
< 8000 !-- -->
youknowone
youknowone reviewed Apr 21, 2024
View reviewed changes
Copy link
Member
@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

If you are going to feel a PR is going too big, you don't need to finish it in a single PR. Keeping the test unchanged and submitting a partial fix is also a preferable way. Don't feel pressured by the unit of test. Thank you!

hydrogen602
hydrogen602 commented Apr 22, 2024
View reviewed changes
@hydrogen602 hydrogen602 marked this pull request as ready for review April 23, 2024 01:09
@hydrogen602
Copy link
Contributor Author

If you are going to feel a PR is going too big, you don't need to finish it in a single PR. Keeping the test unchanged and submitting a partial fix is also a preferable way. Don't feel pressured by the unit of test. Thank you!

Ok I'll aim to get this merged as it is now, and then later look into the last issue with optimize=2 and docstrings.

@youknowone youknowone mentioned this pull request Apr 23, 2024
Merged
youknowone
youknowone reviewed Apr 23, 2024
View reviewed changes
@youknowone youknowone force-pushed the test_builtin_test_compile_fix branch from 9adbe3b to b634a7b Compare April 23, 2024 03:00
@hydrogen602
Copy link
Contributor Author
hydrogen602 commented Apr 23, 2024 •
edited
Loading

Looks like some more compile flags need to be added to the flag validator:

# The CO_xxx symbols are defined here under the same names defined in
# code.h and used by compile.h, so that an editor search will find them here.
# However, they're not exported in __all__, because they don't really belong to
# this module.
CO_NESTED = 0x0010                      # nested_scopes
CO_GENERATOR_ALLOWED = 0                # generators (obsolete, was 0x1000)
CO_FUTURE_DIVISION = 0x20000            # division
CO_FUTURE_ABSOLUTE_IMPORT = 0x40000     # perform absolute imports by default
CO_FUTURE_WITH_STATEMENT = 0x80000      # with statement
CO_FUTURE_PRINT_FUNCTION = 0x100000     # print function
CO_FUTURE_UNICODE_LITERALS = 0x200000   # unicode string literals
CO_FUTURE_BARRY_AS_BDFL = 0x400000
CO_FUTURE_GENERATOR_STOP = 0x800000     # StopIteration becomes RuntimeError in generators
CO_FUTURE_ANNOTATIONS = 0x1000000       # annotations become strings at runtime

I'll look into it

@hydrogen602
Copy link
Contributor Author

Looks like there is an error in macos - pylib/Lib/test/test_socket.py. I'm not sure why though - and the test file runs fine on my own mac. Do you have any insight?

@youknowone
Copy link
Member

That might be just flaky

@hydrogen602
Copy link
Contributor Author

One question I have is how __future__ works. It looks like from compiler/codegen/src/compile.rs compile_future_features() that __future__ settings like annotations are only collected from __future__ import statements, meaning the flags, especially CO_FUTURE_ANNOTATIONS, while allowed, are meaningless. Is there some plan to pass flags into compile.rs?

@youknowone
Copy link
Member
youknowone commented Apr 23, 2024 •
edited
Loading

That's new to me. How about adding a function to collect __future__ from parsed ast and pass the flag value to compile? This way must be easy in current RustPython compile design.
On the other hand, RustPython is premature and we are on the moving target. Ignoring the future and always enabling CO_FUTURE_ANNOTATIONS will be another way to do it.

youknowone
youknowone approved these changes Apr 23, 2024
View reviewed changes
Copy link
Member
@youknowone youknowone 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 so much! The socket test failing looks irrelevant. Let me suppress it later.

@youknowone youknowone merged commit 3313fde into RustPython:main Apr 23, 2024
10 of 11 checks passed
@youknowone youknowone mentioned this pull request Apr 23, 2024
Closed
@hydrogen602 hydrogen602 deleted the test_builtin_test_compile_fix branch April 23, 2024 05:13
@hydrogen602 hydrogen602 mentioned this pull request Apr 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowone youknowone youknowone approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@hydrogen602 @youknowone
0