gh-108303: Move test_future into its own subdir#109368
Uh oh!
There was an error while loading. Please reload this page.
Conversation
sobolevn requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners
sobolevn commented Sep 13, 2023
Yeap, they are executed: https://github.com/python/cpython/actions/runs/6170416321/job/16746672105?pr=109368 |
vstinner commented Sep 14, 2023
I see that the change doesn't remove tests, but adds one test method (29 => 30 tests in total). Before: After: Oh, there is an additional test AFTER: It's this test: deftest_future4(self): withimport_helper.CleanImport('test.test_future_import.test_future4'): fromtest.test_future_importimporttest_future4I don't understand the design of these tests. |
vstinner commented Sep 14, 2023
I'm not sure about
|
sobolevn commented Sep 14, 2023
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.
Indeed, thanks for the suggestion! |
vstinner commented Sep 14, 2023
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 commented Sep 14, 2023
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 |
sobolevn commented Sep 14, 2023
I've created a new issue to fix |
sobolevn commented Sep 14, 2023
👍 |
vstinner commented Sep 14, 2023
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? |
sobolevn commented Sep 14, 2023
How about these names? |
These names are different from each others and look more useful, thanks! Can you update your PR please? |
vstinner left a comment
vstinner left a comment There was a problem hiding this comment.
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.
| self.assertEqual(err.lineno, lineno) | ||
| self.assertEqual(err.offset, offset) | ||
| deftest_future1(self): |
There was a problem hiding this comment.
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?
| fromtest.test_future_stmtimportfuture_test1 | ||
| self.assertEqual(future_test1.result, 6) | ||
| deftest_future2(self): |
There was a problem hiding this comment.
I suggest to rename the file and test to: test_nested_scope.
| ): | ||
| fromtest.test_future_stmtimporttest_future_multiple_features | ||
| deftest_badfuture3(self): |
There was a problem hiding this comment.
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
| fromtest.test_future_stmtimportbadsyntax_future3 | ||
| self.check_syntax_error(cm.exception, "badsyntax_future3", 3) | ||
| deftest_badfuture4(self): |
There was a problem hiding this comment.
$ python badsyntax_future4.py SyntaxError: from __future__ imports must occur at the beginning of the file | fromtest.test_future_stmtimportbadsyntax_future4 | ||
| self.check_syntax_error(cm.exception, "badsyntax_future4", 3) | ||
| deftest_badfuture5(self): |
There was a problem hiding this comment.
SyntaxError: from __future__ imports must occur at the beginning of the file import fails if done after another import.
| fromtest.test_future_stmtimportbadsyntax_future5 | ||
| self.check_syntax_error(cm.exception, "badsyntax_future5", 4) | ||
| deftest_badfuture6(self): |
There was a problem hiding this comment.
SyntaxError: from __future__ imports must occur at the beginning of the file Import after a statement.
| fromtest.test_future_stmtimportbadsyntax_future6 | ||
| self.check_syntax_error(cm.exception, "badsyntax_future6", 3) | ||
| deftest_badfuture7(self): |
There was a problem hiding this comment.
... 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 commented Sep 15, 2023
Merged, thanks for this nice change! Feel free to ignore my remarks on finding better name/doc for tests ;-) |
sobolevn commented Sep 15, 2023
I will open a PR with better names later today :) |
vstinner added needs backport to 3.11 
miss-islington commented Sep 21, 2023
miss-islington commented Sep 21, 2023
miss-islington commented Sep 21, 2023
Sorry, @sobolevn and @vstinner, I could not cleanly backport this to |
miss-islington commented Sep 21, 2023
Sorry, @sobolevn and @vstinner, I could not cleanly backport this to |
…dir (python#109368) (cherry picked from commit 82505dc)
GH-109679 is a backport of this pull request to the 3.12 branch. |
…dir (python#109368) (cherry picked from commit 82505dc)
GH-109680 is a backport of this pull request to the 3.11 branch. |
3 participants
Two important notes:
test:» ./python.exe Lib/test/test_future_import/test_future.py ........................ ---------------------------------------------------------------------- Ran 24 tests in 0.087s OK