Skip to content

gh-63882: Comment out empty test_minidom tests #128948

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

srinivasreddy
Copy link
Contributor

@srinivasreddy srinivasreddy commented Jan 17, 2025

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

@bedevere-app bedevere-app bot added tests Tests in the Lib/test dir awaiting review labels Jan 17, 2025
@srinivasreddy
Copy link
Contributor Author

srinivasreddy commented Jan 17, 2025

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 srinivasreddy changed the title gh-19683: Comment out empty tests gh-63882: Comment out empty tests Jan 17, 2025
@StanFromIreland
Copy link
Contributor

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

Copy link
Contributor

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

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

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

@picnixz
Copy link
Member

picnixz commented Jan 17, 2025

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?

@StanFromIreland
Copy link
Contributor

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

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

picnixz commented Jan 17, 2025

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.

Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

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

@hugovk
Copy link
Member

hugovk commented Apr 24, 2025

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

@StanFromIreland
Copy link
Contributor

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
Copy link
Member

hugovk commented Apr 24, 2025

Maybe checking the old bpo page helps?

https://bugs.python.org/issue19683

Copy link
Contributor

@StanFromIreland StanFromIreland left a comment

Choose a reason for hiding this comment

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

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

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.

5 participants