Skip to content

gh-108303: Move all doctest related files and tests to Lib/test/test_doctest/#112109

Merged
vstinner merged 8 commits into
python:mainfrom
sobolevn:issue-108303-test_doctest
Jan 18, 2024
Merged

gh-108303: Move all doctest related files and tests to Lib/test/test_doctest/#112109
vstinner merged 8 commits into
python:mainfrom
sobolevn:issue-108303-test_doctest

Conversation

@sobolevn

@sobolevn sobolevn commented Nov 15, 2023

Copy link
Copy Markdown
Member

self.lines = lines

def readline(self):
line = self.lines.pop(0)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Calling list.pop(0) frequently is inefficient. Maybe the constructor should create a collections.deque() and call self.lines.popleft() instead?

@sobolevn sobolevn Nov 15, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is usually used in cases like sys.stdin = FakeInput(['next', 'print(x)', 'continue']), I don't think it matters much. But, I can do this if you want :)

Comment thread Makefile.pre.in
@brettcannon brettcannon removed their request for review November 15, 2023 21:06
Comment thread Lib/test/support/pty_helper.py Outdated
Comment thread Lib/test/support/pty_helper.py Outdated
@sobolevn

sobolevn commented Dec 2, 2023

Copy link
Copy Markdown
Member Author

I solved merge conflicts, cc @vstinner

@vstinner

vstinner commented Dec 4, 2023

Copy link
Copy Markdown
Member

Since we are moving code, would it be worth it to split test_doctest.py which has 3 374 lines into multiple files? I found the following tests / test cases:

  • SampleClass
  • sample_func
  • SampleNewStyleClass
  • test_CLI
  • test_debug
  • test_DocFileSuite
  • test_DocTest
  • test_DocTestFinder
  • test_DocTestParser
  • test_DocTestRunner
  • test_DocTestSuite
  • test_Example
  • test_exception_with_multiple_notes
  • test_exception_with_note
  • test_lineendings
  • test_look_in_unwrapped
  • test_no_trailing_whitespace_stripping
  • test_pdb_set_trace
  • test_pdb_set_trace_nested
  • test_run_doctestsuite_multiple_times
  • test_syntax_error_subclass_from_stdlib
  • test_syntax_error_with_incorrect_expected_note
  • test_syntax_error_with_note
  • test_testfile
  • test_testmod
  • test_testsource
  • test_trailing_space_in_test
  • test_unicode
  • test_unittest_reportflags

For example, it would be nice to have 1000 lines or less per file. If you are interested to make such change, I suggest creating a new PR, so we can compare.

About test_doctest2: why is it a separated file? Should it be renamed? The docstring says:

A module to test whether doctest recognizes some 2.2 features, like static and class methods.

I suppose that 2.2 stands for Python 2.2.

Is it a test on the "parser" or "test discovery"?

@sobolevn

Copy link
Copy Markdown
Member Author

Since we are moving code, would it be worth it to split test_doctest.py which has 3 374 lines into multiple files?

I don't think so, because moving a file is a rather simple operation. It does not spoil git blame. While splitting files can be devastating to its history.

I work with test_doctest quite a lot, I don't think that it has maintability problems.

About test_doctest2: why is it a separated file? Should it be renamed?

I agree that the naming of this file is not optimal. It also clashes with test_doctest2.txt, I don't think that this is intentional.

Looks like that all tests from it are just duplicates. We already test classes, classmethods, staticmethods and everything else in test_doctest. So, it can be simply removed.

It has some details that I want to explicitly port:

  • property with a setter
  • poluting namespace with other objects

@sobolevn sobolevn requested a review from vstinner December 22, 2023 08:19

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I cannot see where Lib/test/test_doctest2.py test goes. Did you just remove it?

@sobolevn

Copy link
Copy Markdown
Member Author

Yes:

I agree that the naming of this file is not optimal. It also clashes with test_doctest2.txt, I don't think that this is intentional.

Looks like that all tests from it are just duplicates. We already test classes, classmethods, staticmethods and everything else in test_doctest. So, it can be simply removed.

@brettcannon brettcannon requested a review from vstinner January 12, 2024 19:12
@vstinner

Copy link
Copy Markdown
Member

Looks like that all tests from it are just duplicates. We already test classes, classmethods, staticmethods and everything else in test_doctest. So, it can be simply removed.

I'm not convinced that test_doctest2 is useless. It calls doctest.testmod(sys.modules[__name__]) and test_doctest2.py contains 2 non-ASCII docstrings. Example:

"""...
We include some (random) encoded (utf-8) text in the text surrounding
the example.  It should be ignored:

ЉЊЈЁЂ
"""

It runs tests in verbose mode instead of running them in quiet mode:

  • test_doctest: doctest.testmod(unicodedata, verbose=False) (quiet)
  • test_doctest2: doctest.testmod(sys.modules[__name__]) (verbose)

Even we agree that test_doctest2 is useless, I would prefer to not include this silent removal in the middle of such huge refactoring (move all tests code).

Please add back test_doctest2.py in the new test_doctest/ directory.

@vstinner vstinner merged commit 9c93350 into python:main Jan 18, 2024
@miss-islington-app

Copy link
Copy Markdown

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 18, 2024
…t/test_doctest/` (pythonGH-112109)

(cherry picked from commit 9c93350)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
Co-authored-by: Brett Cannon <brett@python.org>
@bedevere-app

bedevere-app Bot commented Jan 18, 2024

Copy link
Copy Markdown

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

@miss-islington-app

Copy link
Copy Markdown

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 9c93350f582fe6f5fed2cd873869dfe4fbf2dfe8 3.11

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jan 18, 2024
@vstinner

Copy link
Copy Markdown
Member

Merged, thanks.

@sobolevn: The automated backport to 3.11 failed, do you want to attempt to backport the change to 3.11 manually?

vstinner pushed a commit that referenced this pull request Jan 18, 2024
…st/test_doctest/` (GH-112109) (#114254)

gh-108303: Move all doctest related files and tests to `Lib/test/test_doctest/` (GH-112109)
(cherry picked from commit 9c93350)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
Co-authored-by: Brett Cannon <brett@python.org>
@sobolevn

Copy link
Copy Markdown
Member Author

Sure thing!

@brettcannon

Copy link
Copy Markdown
Member

sobolevn added a commit to sobolevn/cpython that referenced this pull request Jan 19, 2024
…les and tests to `Lib/test/test_doctest/` (python#112109)"

This reverts commit 9c93350.
sobolevn added a commit to sobolevn/cpython that referenced this pull request Jan 19, 2024
…lated files and tests to `Lib/test/test_doctest/` (python#112109)""

This reverts commit c3701e9.
@vstinner

Copy link
Copy Markdown
Member

sobolevn added a commit to sobolevn/cpython that referenced this pull request Jan 19, 2024
@bedevere-app

bedevere-app Bot commented Jan 19, 2024

Copy link
Copy Markdown

GH-114313 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 Jan 19, 2024
vstinner pushed a commit that referenced this pull request Jan 19, 2024
…st/test_doctest/` (GH-112109) (#114313)

gh-108303: Move all doctest related files and tests to `Lib/test/test_doctest/` (GH-112109)
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
…t/test_doctest/` (python#112109)

Co-authored-by: Brett Cannon <brett@python.org>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…t/test_doctest/` (python#112109)

Co-authored-by: Brett Cannon <brett@python.org>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…t/test_doctest/` (python#112109)

Co-authored-by: Brett Cannon <brett@python.org>
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