Skip to content
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

bpo-10572 : Move sqlite3 tests to Lib/test/test_sqlite3 #24148

Closed

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jan 7, 2021

Erlend E. Aasland added 6 commits January 7, 2021 01:41
* git rm __init__.py
* git mv Lib/sqlite3/test => Lib/test/test_sqlite3
* git mv test_sqlite3.py => __init__.py
* add test_ prefix to test files
@erlend-aasland
Copy link
Contributor Author

I know you find this change controversial, @berkerpeksag, but I think it'll be nice in the end. Motivations:

  • https://bugs.python.org/issue10572#msg179596
  • test suites are simplified and less error prone (for example forgetting to include a test suite)
  • it'll be possible to do python3 Lib/test/test_sqlite3/test_types.py, for example
  • no special treatment in Makefile

What do you think?

@erlend-aasland
Copy link
Contributor Author

BTW, history is preserved by git mv.

Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

https://bugs.python.org/issue10572#msg179596

Unfortunately, I don't find Brett's arguments convincing. Linux distros are making too many customizations that would make stripping tests a trivial change compared to others.

test suites are simplified and less error prone (for example forgetting to include a test suite)

I don't even remember last time when this was an issue, to be honest.

it'll be possible to do python3 Lib/test/test_sqlite3/test_types.py, for example

This is a nice UX improvement. However, sqlite3 tests take almost no time to run and it's relatively easy to disable other test cases.

no special treatment in Makefile

Well, sqlite3 is hardly the only exception there :)

Also, we already made backporting fixes to maintenance branches automatically harder by renaming the test names. Moving tests around would make auto backporting almost impossible which I would like to avoid doing.

I'm sure you can find more interesting stuff to work on :)

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 7, 2021

test suites are simplified and less error prone (for example forgetting to include a test suite)
I don't even remember last time when this was an issue, to be honest.

Ok, but they're still simplified :) This PR removed a lot of lines.

it'll be possible to do python3 Lib/test/test_sqlite3/test_types.py, for example
This is a nice UX improvement. However, sqlite3 tests take almost no time to run and it's relatively easy to disable other test cases.

That's true

no special treatment in Makefile
Well, sqlite3 is hardly the only exception there :)

True, but it still declutters the Makefile (by one line)

Also, we already made backporting fixes to maintenance branches automatically harder by renaming the test names. Moving tests around would make auto backporting almost impossible which I would like to avoid doing.

Not impossible, but cumbersome. Anyway, it's a strong argument. However, the recent test changes will also make backporting cumbersome (for the next two releases only), which is a minor argument pro this change.

OTOH, this is no big deal for me. I'm ok with letting it go for now. It was a nice exercise, though :)

I'm sure you can find more interesting stuff to work on :)

I'm twiddling thumbs, waiting for #24135 ;)

Yes, there's a lot of stuff to work on. After the AC transition, I've got PR's for module state and multi-phase init. When we're there, I would like to get rid of the cyclic ref hack between Cache and Connection. I had a PR for it, but for some reason it failed on Windows. I haven't spent much time figuring out that issue, though. New features can wait for now, IMO.

@berkerpeksag
Copy link
Member

I had a PR for it, but for some reason it failed on Windows. I haven't spent much time figuring out that issue, though.

I'm currently on Windows and I can take a look. It may take a while though because I mainly use it for gaming and I need to set up my development environment.

I'm closing this for now. We can consider it later. Thank you for your effort! :)

@erlend-aasland
Copy link
Contributor Author

I had a PR for it, but for some reason it failed on Windows. I haven't spent much time figuring out that issue, though.
I'm currently on Windows and I can take a look. It may take a while though because I mainly use it for gaming and I need to set up my development environment.

Great, thanks!

We can consider it later. Thank you for your effort! :)

I sense a sliver of hope for this bpo issue ;) I'll keep the branch around

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.

4 participants