GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-10572: Move tkinter tests to /test by idomic · Pull Request #18727 · python/cpython

Conversation

@idomic

@idomic idomic commented Mar 1, 2020 •
edited by terryjreedy
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue10572

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@idomic
idomic requested review from berkerpeksag and rhettinger as code owners March 1, 2020 22:51
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 1, 2020
@bedevere-bot bedevere-bot added the awaiting review label Mar 1, 2020
@idomic idomic changed the title Moving tests - clean start - tkinter, sqlite3, ctypes bpo-10572: Moving tests - clean start - tkinter, sqlite3, ctypes Mar 1, 2020
berkerpeksag
berkerpeksag requested changes Mar 2, 2020
View reviewed changes

@berkerpeksag berkerpeksag left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please do not touch sqlite3 tests. It needs more work than just moving things around.

Also, submitting separate PRs for each module would increase the chance of getting them accepted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot removed the awaiting review label Mar 2, 2020
@bedevere-bot

bedevere-bot commented Mar 2, 2020

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Mar 2, 2020
@idomic
Hard reset + Tkinter module only
ffa202d
@idomic
idomic force-pushed the bpo-10572-testlibs branch from d659ed4 to ffa202d Compare March 15, 2020 14:44
merwok
merwok reviewed Mar 15, 2020
View reviewed changes
Comment thread Lib/test/test_tk.py Outdated
support.requires('gui')

from tkinter.test import runtktests
from Lib.test.tkinter_test import runtktests

@merwok merwok Mar 15, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This can’t be correct. Lib is not an importable Python package, it is a path containing modules and packages.
The new import should be from test.tkinter_test import runtktests.

(Or for what it’s worth, this nearly empty file could be removed, if the new package for tkinter tests would be named in a way that’s automatically found by unittest discover and runtests)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@idomic idomic Mar 16, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done, I think it might be better to separate the changes no?
Like the first stage - migrate the module. Second stage - remove redundant legacy files.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@idomic
Fixing import to point to test
844ce31
@idomic

idomic commented Mar 16, 2020

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Mar 16, 2020
@bedevere-bot

bedevere-bot commented Mar 16, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from berkerpeksag March 16, 2020 12:58
@idomic
Merge branch 'master' into bpo-10572-testlibs
095a075
terryjreedy
terryjreedy requested changes Sep 6, 2020
View reviewed changes

@terryjreedy terryjreedy left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I don't think that the actual tkinter tests should be installed unless tkinter is installed. So I don't think that tkinter tests should be changed. I certainly would not do so unless Serhiy wants the move and approves of how it is done.

EDIT: So I am actually suggesting that this be closed and that you pick something else to move that is always installed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Sep 6, 2020

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting change review labels Sep 6, 2020
merwok
merwok reviewed Sep 6, 2020
View reviewed changes
Comment thread Lib/test/test_tk.py
@@ -5,7 +5,7 @@
# Skip test if tk cannot be initialized.

@merwok merwok Sep 6, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Lib/__init__.py should be deleted.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
merwok
merwok reviewed Sep 6, 2020
View reviewed changes
Comment thread Lib/test/test_ttk_textonly.py
from test.tkinter_test import runtktests

def test_main():
support.run_unittest(

@merwok merwok Sep 6, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

After moving the tests, files like this (just contains an import and a wrapper to interface runtests with test_ttk) end up being a slightly confusing, mostly unneeded layer.

As long as many files are moved and history is broken, why not also clean up this tangle by merging the compat layer into the real test files?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@terryjreedy
terryjreedy requested a review from serhiy-storchaka July 15, 2021 22:29
@terryjreedy terryjreedy changed the title bpo-10572: Moving tests - clean start - tkinter, sqlite3, ctypes bpo-10572: Move tkinter tests to /test Jul 15, 2021
@rhettinger
rhettinger removed their request for review May 3, 2022 06:30
@voidspace voidspace mentioned this pull request Jun 20, 2022
Move test sub-packages to Lib/test #54781
Closed
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@erlend-aasland

erlend-aasland commented Jul 20, 2022

Copy link
Copy Markdown
Contributor

This was implemented in #94070.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland closed this Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@terryjreedy terryjreedy terryjreedy requested changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@idomic @bedevere-bot @erlend-aasland @berkerpeksag @merwok @terryjreedy @the-knights-who-say-ni @ezio-melotti
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.