FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-63882: Comment out empty `test_minidom` tests by srinivasreddy · Pull Request #128948 · python/cpython · GitHub

Repository navigation

gh-63882: Comment out empty test_minidom tests - #128948

Closed
srinivasreddy wants to merge 6 commits into
python:mainfrom
srinivasreddy:gh-19683-comment-empty-tests
Closed

srinivasreddy wants to merge 6 commits into
python:mainfrom
srinivasreddy:gh-19683-comment-empty-tests

Conversation

srinivasreddy commented Jan 17, 2025 •
edited by terryjreedy
Loading

Copy link
Copy Markdown
Contributor

Given the absence of immediate replacements, this was considered better than deletion.

bedevere-app Bot added tests Tests in the Lib/test dir awaiting review labels Jan 17, 2025

srinivasreddy commented Jan 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This is the second item from the action items proposed by @terryjreedy (#63882 (comment))

  1. Minidom docstrings issue gh-128508: Add some docstrings to xml.dom.minidom #128477
  2. Comment out the empty tests gh-63882: Comment out empty test_minidom tests #128948
  3. Test revisions - to be addressed in later pr

srinivasreddy changed the title gh-19683: Comment out empty tests gh-63882: Comment out empty tests Jan 17, 2025

Copy link
Copy Markdown
Member

@terryjreedy (I can’t request a review in any other way)

StanFromIreland 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think all of them are included. Looks good to me!

picnixz commented Jan 17, 2025

Copy link
Copy Markdown
Member

Why are we using ## instead of a simple # for commenting out the test? Is there any plan of actually implementing those tests in the future? can we add some # TODO as well so that we can find them easily in the future?

Copy link
Copy Markdown
Member

@picnixz This was requested by Terry in his comment on the issue. I guess it is easier to find them?

picnixz changed the title gh-63882: Comment out empty tests gh-63882: Comment out empty test_minidom tests Jan 17, 2025

picnixz commented Jan 17, 2025

Copy link
Copy Markdown
Member

I think I'll me comfortable with an explicit "TODO" since we also used # CRASH for the test C API to make them easier to find (with a ##, we need to filter out the results but with # TODO, we know that's a TODO).


Ok, I've seen that #63882 (comment) is actually the comment that explained the different steps to do (it was hidden by GH). So there's a plan to implement something. If the tests are meant to be implemented in a short future, ## is fine, but if it's not, then maybe # TODO is better.

terryjreedy left a comment •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Double hashes, '##', are an old standard for (temporarily) commenting out code with something other than normal code comments. I added a "# TODO" comment.

Comment thread Lib/test/test_minidom.py Outdated
Comment thread Lib/test/test_minidom.py Outdated
Comment thread Lib/test/test_minidom.py Outdated

terryjreedy left a comment •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Make the first 3 tests 1 line as with all the others.

Comment thread Lib/test/test_minidom.py Outdated

hugovk commented Apr 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

These stubs have been present for 25 years, since Python 2.0!
7993bcc

I'd rather see tests added rather than commented out. If not added, let's delete them otherwise I can see these being around until 2050 🙃

Quoting @voidspace:

The same is true for commenting-out code; if a block of commented code is going into a release, it shouldn't exist. If it is code that may be restored, make a ticket and reference the commit hash for the code delete.

https://opensource.com/article/17/5/30-best-practices-software-development-and-testing

Copy link
Copy Markdown
Member

There is actually already a patch that would add some of them: https://bugs.python.org/file33258/issue19683.patch

Tough I am not sure who submitted it.

hugovk commented Apr 24, 2025

Copy link
Copy Markdown
Member

Maybe checking the old bpo page helps?

https://bugs.python.org/issue19683

StanFromIreland 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some of these tests have been implemented. Surprisingly not a merge conflict.

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 21, 2026

Copy link
Copy Markdown
Member

Thank you for the patch. Commenting the tests out was the right thing while there was no replacement, but they are implemented now in #156677, so this is no longer needed.

The ## def testX(self): pass form would also have to be reconciled with the tests which have been implemented since January 2025, as noted in the last review.

github-actions Bot removed the stale Stale PR or inactive for long period of time. label Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting merge skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL