Uh oh!
There was an error while loading. Please reload this page.
gh-109485: Further improve test_future_stmt tests - #109486
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
left a comment
There was a problem hiding this comment.
Would it be possible to check for the error message in assertSyntaxError?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| self.assertSyntaxError(code, 3) | ||
| def test_badfuture10(self): | ||
| def test_bad_future_as_module(self): |
There was a problem hiding this comment.
Test name is not clear. The expected error is:
SyntaxError: from __future__ imports must occur at the beginning of the file
sobolevn
commented
Sep 18, 2023
With error messages it should be much better! |
Uh oh!
There was an error while loading. Please reload this page.
| lineno, | ||
| message=TOP_LEVEL_MSG, offset=1, | ||
| parametrize_docstring=True): | ||
| code = dedent(code) |
There was a problem hiding this comment.
It seems like code first line is always empty. I suggest using:
| code=dedent(code) | |
| code=dedent(code.lstrip()) |
And adapt lineno -= 2 and other references to lineno below.
For example, for me, test_unknown_future_flag() code has only 3 lines and expect an error on the 4th line, it's strange.
vstinner
left a comment
There was a problem hiding this comment.
LGTM. Thanks! It's way way more readable now! The test methods have better name, it's easier to understand the purpose of the test, and you added many checks!
| code = dedent(code.lstrip('\n')) | ||
| for trim_docstring in ([False, True] if parametrize_docstring else [False]): | ||
| with self.subTest(code=code, trim_docstring=trim_docstring): | ||
| if trim_docstring: |
There was a problem hiding this comment.
Last question: instead of copy/paste '''Docstring''' in each test. Would it make sense to add it here instead? I'm waiting for your opinion to decide if we merge the change like that, or you prefer to remove docstrings in tests and add it here instead.
vstinner
commented
Sep 19, 2023
Thanks a lot @sobolevn, it's way better! I edited the final message. |
I've also enabled docstring parametrization, because tests must works the same way with and without docstrings most of the time.
test_future_stmt#109485