Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Mocking a connection is also simplified with a fixture. Aditionally two more tests were added.
|
Hi. Thanks for modernizing. We appreciate using testcontainers instead of testlayers and specifically to get rid of Zope-related packages from previous decades where maintenance seems to be fading. (see also GH-723, GH-725, GH-736) However, is it possible to split project infrastructure / metadata updates vs. those test modernizations which will have a much larger impact to the testing codebase, and also possibly migrate test cases progressively? We otherwise fear the patch will become too big to review reasonably, a problem we've currently encountered elsewhere. testcontainers/python-unittest demonstrates a way to use the modern testcontainers way of testing together with traditional Python unittest, intended to support this and other repositories on their modernization paths. Maybe this can help a bit on this occasion? CrateDBTestAdapter is effectively just a little wrapper using the CrateDBContainer class that is implementing the testcontainers specification, so it can be slotted into unittest layer concepts, at least as an intermediate workaround adapter for supporting a progressive migration, when possible. NB: In particular, |
Yes I can split the PR into -> metadata & tests. I put them together because reviewing a
My goal is to move to the current python standard which is to use
I kind of find it annoying, with just pytest every test need is pretty much solved, I don't think there is any need for it nowadays, migrating away from I understand that the review could be a bit painful but I think it's one of those that happen every few years that you just have to do it, I think that we are all very used to pytest and as you can see in the ones that I migrated, the resulting code is very light and shorter than the unitests', so I'd ask for a little bit of help soldiering here 🙏 |
|
Hi. Thanks a stack for bringing in pytest.
I very much appreciate the transition to pytest, I am certainly supporting it. However, I don't know if I will be able to provide hands-on help due to other obligations, asking for your understanding. My fears the patch would become too large have not been too substantial so far: I think it provides a reasonable diff size to review properly. Most probably, I am looking at those more delicate future(?) changes which have not been tackled yet, and which might increase diff size / review complexity:
I think all reviewers will like if those can be tackled separately. So, let me quickly ask a question about the ingredients/state of the patch wrt. those matters: Are integration- and doctests currently still functional by using traditional unittest test layers / will they still be executed by the traditional zope testrunner, as I see not much code might have been removed in this area?
If this means corresponding modernizations will be tackled using a separate patch, I think everyone will be happy. NB: Maybe change the PR title to something like "Migrate unit tests to pytest. Switch from setuptools to hatchling.", if this better matches up with its ingredients? Apologies if I am missing important details yet: I just quickly skimmed the patch to provide a few fragments of early feedback. Feel free to reach out directly if you think I am misunderstanding some details. NB: Even if, or just because "Switch from setuptools to hatchling" might be so trivial, submitting this as a separate patch will always be preferred / way to go, not just from the reviewing perspective, but also from aspects around repository maintenance: If this change is isolated and brought in on behalf of a single commit, any obstacles around it (git revert, when applicable) can be processed much better in the aftermath. |
Going to leave those for later and wrap this up |
62d57d5 to
c4da897
Compare
|
From the last review, only new tests were added to increase coverage. |
| dynamic = ["version"] | ||
| description = "CrateDB Python Client" | ||
| authors = [{ name = "Crate.io", email = "office@crate.io" }] | ||
| requires-python = ">=3.10" |
There was a problem hiding this comment.
I think the code is still compatible down to Python 3.6?
There was a problem hiding this comment.
We have two users running 2.7, so perhaps even lower than 3.6
There was a problem hiding this comment.
Hm. I don't think the current version of the driver is compatible with Python 2 any longer.
This pr attempts to modernize the project by:
pyproject.tomlmoving away fromsetup.pypytestmoving away fromzopeandunittestsuvand any other pyproject.toml compatible toolsetup.pyshenanigansSimplifying tests by moving away from the customTestlayerto testcontainersChanges
setup.pytopyproject.tomluvand othervenvtools.uv buildandpython -m buildMigrate doctests to pytestMigrate testlayer to testcontainersAditional changes
test_httpintotest_serializationandtest_client. This is just to give an idea of what the change is like, might actually differ later.Notes to remember: