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

gh-108303: Move test_future into its own subdir #109368

Merged
merged 4 commits into from
Sep 15, 2023

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 13, 2023

Two important notes:

  1. These tests are found by test:
» ./python.exe -m test --list-tests | grep test_future_import
test.test_future_import.test_future
test.test_future_import.test_future3
test.test_future_import.test_future4
test.test_future_import.test_future5
  1. They do run on direct module execution:
» ./python.exe Lib/test/test_future_import/test_future.py    
........................
----------------------------------------------------------------------
Ran 24 tests in 0.087s

OK

@sobolevn
Copy link
Member Author

@brettcannon brettcannon removed their request for review September 13, 2023 22:25
@vstinner
Copy link
Member

I see that the change doesn't remove tests, but adds one test method (29 => 30 tests in total).

Before:

vstinner@mona$ ./python -m test test_future test_future3 test_future4 test_future5 -j0
0:00:00 load avg: 2.56 Run 4 tests in parallel using 4 worker processes
0:00:00 load avg: 2.56 [1/4] test_future5 passed
0:00:00 load avg: 2.56 [2/4] test_future3 passed
0:00:00 load avg: 2.56 [3/4] test_future4 passed
0:00:00 load avg: 2.56 [4/4] test_future passed

== Tests result: SUCCESS ==

All 4 tests OK.

Total duration: 959 ms
Total tests: run=29
Total test files: run=4/4
Result: SUCCESS

After:

vstinner@mona$ ./python -m test test_future_import -j0
0:00:00 load avg: 1.60 Run 4 tests in parallel using 4 worker processes
0:00:00 load avg: 1.60 [1/4] test_future_import.test_future3 passed
0:00:00 load avg: 1.60 [2/4] test_future_import.test_future4 passed
0:00:00 load avg: 1.60 [3/4] test_future_import.test_future5 passed
0:00:00 load avg: 1.60 [4/4] test_future_import.test_future passed

== Tests result: SUCCESS ==

All 4 tests OK.

Total duration: 947 ms
Total tests: run=30
Total test files: run=4/4
Result: SUCCESS

Oh, there is an additional test AFTER:

test.test_future_import.test_future.FutureTest.test_future4

It's this test:

    def test_future4(self):
        with import_helper.CleanImport('test.test_future_import.test_future4'):
            from test.test_future_import import test_future4

I don't understand the design of these tests. test_future.test_future4() just imports test_future4(), but it doesn't execute it? Usually, we don't use "test_xxx.py" files for that, but "xxx.py". It's just surprising.

@vstinner
Copy link
Member

I'm not sure about test_future_import name. I would prefer to stick to just test_future. If you want to keep "import", I would prefer test_import_future :-)

Lib/test/test___future__.py should be included in this directory as well, no?

@sobolevn
Copy link
Member Author

test_future is problematic. I faced several test failures because of Lib/test/test_concurrent_futures/test_future.py and Lib/test/test_future/test_future.py. This might be a bug in libregrtest. I will report it, but I will go with test_import_future for now.

test.test_future_import.test_future.FutureTest.test_future4

Yes, it was missing, I've decided to add it. All other tests were imported like this, except this one. I also a bit confused about the design of these tests, but I don't see any harm in them as well.

test___future__.py

Indeed, thanks for the suggestion!

@vstinner
Copy link
Member

I also a bit confused about the design of these tests, but I don't see any harm in them as well.

Ok. I wasn't directly a remark about this change, but more the test in general. In case of doubt, I prefer to leave it this way.

@vstinner
Copy link
Member

I will go with test_import_future for now.

The documentation calls it "Future statement definitions": https://docs.python.org/dev/library/__future__.html

It's more a statement than a module, even if a real module exists.

Maybe use test_future_statement or test_future_stmt name.

@sobolevn
Copy link
Member Author

I've created a new issue to fix test_future/ folder name bug: #109402

@sobolevn
Copy link
Member Author

👍 test_future_stmt

@vstinner
Copy link
Member

Honestly, the test filenames are not very helpful :-( While we are moving code and renaming files anyway, would it be possible to come up with better names?

$ cd Lib/test/test_future_stmt/
$ ls test_*.py -1
test_future3.py
test_future4.py
test_future5.py
test___future__.py
test_future.py

@sobolevn
Copy link
Member Author

How about these names?

» ./python.exe -m test test_future_stmt
0:00:00 load avg: 1.47 Run 5 tests sequentially
0:00:00 load avg: 1.47 [1/5] test_future_stmt.test_future
0:00:00 load avg: 1.47 [2/5] test_future_stmt.test_future_flags  (test___future__)
0:00:00 load avg: 1.47 [3/5] test_future_stmt.test_future_multiple_features (test_future5)
0:00:00 load avg: 1.47 [4/5] test_future_stmt.test_future_multiple_imports (test_future4)
0:00:00 load avg: 1.47 [5/5] test_future_stmt.test_future_single_import (test_future3)

@vstinner
Copy link
Member

vstinner commented Sep 15, 2023

How about these names?

These names are different from each others and look more useful, thanks! Can you update your PR please?

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.

The tests could be enhanced to have a better name and description (documentation/comment/docstring) than just... a counter: test1, test2, test3, ...

Maybe using inline code rather than loading file would help to make tests more explicit.

I don't want to hold this PR further. Enhancing the test can be done later.

@@ -25,57 +25,71 @@ def check_syntax_error(self, err, basename, lineno, offset=1):
self.assertEqual(err.offset, offset)

def test_future1(self):
Copy link
Member

Choose a reason for hiding this comment

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

test_future1 has an interesting comment that you can copy there:

# Import the name nested_scopes twice to trigger SF bug #407394 (regression).

I suggest to rename the file and test to: test_import_nested_scope_twice. For the filename, just drop test_ prefix: import_nested_scope_twice.py?

with import_helper.CleanImport('future_test1'):
from test import future_test1
with import_helper.CleanImport('test.test_future_stmt.future_test1'):
from test.test_future_stmt import future_test1
self.assertEqual(future_test1.result, 6)

def test_future2(self):
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to rename the file and test to: test_nested_scope.

with import_helper.CleanImport(
"test.test_future_stmt.test_future_multiple_features",
):
from test.test_future_stmt import test_future_multiple_features

def test_badfuture3(self):
Copy link
Member

Choose a reason for hiding this comment

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

This one tests that a typo is catched properly, that an invalid name raises SyntaxError: from __future__ import rested_snopes.

Maybe: test_unknown_feature. The doc calls __future__ names as "features": https://docs.python.org/dev/library/__future__.html


def test_badfuture3(self):
with self.assertRaises(SyntaxError) as cm:
from test import badsyntax_future3
from test.test_future_stmt import badsyntax_future3
self.check_syntax_error(cm.exception, "badsyntax_future3", 3)

def test_badfuture4(self):
Copy link
Member

Choose a reason for hiding this comment

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

$ python badsyntax_future4.py 
SyntaxError: from __future__ imports must occur at the beginning of the file

self.check_syntax_error(cm.exception, "badsyntax_future3", 3)

def test_badfuture4(self):
with self.assertRaises(SyntaxError) as cm:
from test import badsyntax_future4
from test.test_future_stmt import badsyntax_future4
self.check_syntax_error(cm.exception, "badsyntax_future4", 3)

def test_badfuture5(self):
Copy link
Member

Choose a reason for hiding this comment

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

SyntaxError: from __future__ imports must occur at the beginning of the file

import fails if done after another import.

self.check_syntax_error(cm.exception, "badsyntax_future4", 3)

def test_badfuture5(self):
with self.assertRaises(SyntaxError) as cm:
from test import badsyntax_future5
from test.test_future_stmt import badsyntax_future5
self.check_syntax_error(cm.exception, "badsyntax_future5", 4)

def test_badfuture6(self):
Copy link
Member

Choose a reason for hiding this comment

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

SyntaxError: from __future__ imports must occur at the beginning of the file

Import after a statement.

self.check_syntax_error(cm.exception, "badsyntax_future5", 4)

def test_badfuture6(self):
with self.assertRaises(SyntaxError) as cm:
from test import badsyntax_future6
from test.test_future_stmt import badsyntax_future6
self.check_syntax_error(cm.exception, "badsyntax_future6", 3)

def test_badfuture7(self):
Copy link
Member

Choose a reason for hiding this comment

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

... Maybe these tests would be more explicit if they would not use a file, but pass the code as a string (maybe use textwrap.dedent()).

@vstinner vstinner merged commit 82505dc into python:main Sep 15, 2023
@vstinner
Copy link
Member

Merged, thanks for this nice change!

Feel free to ignore my remarks on finding better name/doc for tests ;-)

@sobolevn
Copy link
Member Author

I will open a PR with better names later today :)
Thanks a lot, Victor!

@vstinner vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Sep 21, 2023
@miss-islington
Copy link
Contributor

Thanks @sobolevn for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry, @sobolevn and @vstinner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 82505dc351b2f7e37aa395218709b432d83292cd 3.11

@miss-islington
Copy link
Contributor

Sorry, @sobolevn and @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 82505dc351b2f7e37aa395218709b432d83292cd 3.12

vstinner pushed a commit to vstinner/cpython that referenced this pull request Sep 21, 2023
@bedevere-app
Copy link

bedevere-app bot commented Sep 21, 2023

GH-109679 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Sep 21, 2023
vstinner pushed a commit to vstinner/cpython that referenced this pull request Sep 21, 2023
@bedevere-app
Copy link

bedevere-app bot commented Sep 21, 2023

GH-109680 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Sep 21, 2023
vstinner added a commit that referenced this pull request Sep 21, 2023
…bdir (#109368) (#109680)

gh-108303: Move `test_future` into its own test_future_stmt subdir (#109368)

(cherry picked from commit 82505dc)

Co-authored-by: Nikita Sobolev <[email protected]>
Yhg1s pushed a commit that referenced this pull request Oct 2, 2023
…bdir (#109368) (#109679)

gh-108303: Move `test_future` into its own test_future_stmt subdir (#109368)

(cherry picked from commit 82505dc)

Co-authored-by: Nikita Sobolev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants