Repository navigation
Python 3.12 tests currently fail #1357
Description
The error is:
Traceback (most recent call last):
File "/Users/facelessuser/Code/github/markdown/tests/test_meta.py", line 20, in test__version__IsValid
import packaging.version
ModuleNotFoundError: No module named 'packaging'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/facelessuser/Code/github/markdown/tests/test_meta.py", line 22, in test__version__IsValid
from pkg_resources.extern import packaging
ModuleNotFoundError: No module named 'pkg_resources'
This will require more investigation. 3.12 is still in beta. We will need to be fixed before we officially state support for 3.12.
Activity
waylan commentedon Jul 17, 2023
@pawamoy stated in this comment:
waylan commentedon Jul 17, 2023
Note that the error results from
from pkg_resources.extern import packaging. We are usingpackagingfor version parsing, which is not part ofimportliband still requirespackaging. IIRC, we were intentionally importingpackagingthis way to ensure that the same version of the lib was used that was used for installation. However, now thatpkg_resourcesis deprecated (even by Setuptools) and importlib does not seem to have anyextern.packaging(or similar) object, we should just importpackagingdirectly. Of course, that would require thatpackagingbe included in the list of dependencies for our test envs as well.waylan commentedon Jul 17, 2023
Did some digging through the history and found this comment which explains why we did not need to list
packagingas a dependency (it also alludes to why we usedpkg_resources.extern.packaging). Specifically, havingpkg_resourcesinstalled guaranteed thatpackagingwould be installed. Oh how things change.facelessuser commentedon Jul 18, 2023
While I understand why we rely on
packaging, it'd be nice to just eliminate such a dependency with a simplistic approach, but I realize this may not happen.waylan commentedon Jul 18, 2023
I was thinking similarly. In the relevant tests, we are comparing the output of
markdown.__meta__._get_versionto parsed version strings, where the parsing is done bypackaging. Why not just compare the output to the string directly?According to the commit message (in 82d2b25) the thinking was that by parsing the string, we were ensuring that we were outputting a normalized format. But what is going to cause those tests to fail in the future? It's not likely that
packagingwill change its behavior for the current API. I would expect a new API would be introduced for a new format. So, I suppose, in the end there isn't much value in keeping the tests as-is.I could go either way on this. In the one hand, it isn't much work to add one more dependency to the existing list. On the other had, if the tests are more complex than they need to be, why not simplify them?
waylan commentedon Jul 24, 2023
Just noting that #760 tracks the Python versions we support. Python 3.7 reached end-of life on 2023-06-27 and Python 3.12 is scheduled for release on 2023-10-02. Therefore, I think it makes sense to drop support for 3.7 at the same time that we add official support for 3.12 (even if we do so prior to 31.2's final release).
As an aside, I will note that this will be the last time that Python drops a version on a different date that it releases a new version. Going forward, it is expected that both are to always happen in October of each year. Then we shouldn't need to be concerned with addressing them separately any more. While we could this year, I think we may as well combine them now anyway.
Finally, I am assigning this to the 3.5 milestone. This needs to happen in the next point release we make. Hopefully we can get it done by 2023-10-02 or shortly thereafter.
waylan commentedon Aug 7, 2023
So we are already just comparing the string. The one test where we call
packagingis for the current version. It is ensuring the the actual version number is the normalized form. As it is generated (and changes with each release) there is no way to know what the expected string is ahead of time without a manual edit. In other words, we either need to drop thetest__version__IsValidtest or listpackagingas a dependency.facelessuser commentedon Aug 7, 2023
I personally opt for just dropping the test. Unless you are truly concerned we are going to have an issue with people putting in invalid versions that we won't catch in code review. I'd like to avoid dependencies for something like this, but that is my opinion.
Add PY3.12 & PYPY3.10 and drop 3.7
Add PY3.12 & PYPY3.10 and drop 3.7
Add PY3.12 & PYPY3.10 and drop 3.7
mitya57 commentedon Aug 7, 2023
Another option would be to raise
unittest.SkipTestifpackagingmodule is not available.This way we won't need it most of the time, but if you bumped a version and want to double-check its correctness, you can run tests with
packaginginstalled.waylan commentedon Aug 8, 2023
I was just thinking similarly. The question is, how do we want to determine when to install the extra dependency? In practice, we only need the test to run when we edit the version. I see a few options:
markdown/__meta__.pyfile.Option 1 is the easiest way to not miss anything. Option 2 assumes the documented release process is being followed. And Option 3 would require an extra step in the release process. I'm inclined to go with option 1 as it is less prone to human error. However, with option 3 it is always possible to add the label later and rerun the tests. Option 2 seems like it is the most fragile as a simple typo could easily cause it to get missed.
I think I will attempt option 1 and if it proves too difficult to get a GitHub workflow to properly detect the edited file, then I will switch to option 3. Unless someone has a better suggestion...
waylan commentedon Aug 8, 2023
That was easier than expected. See a working solution in #1371. Thanks @mitya57 for the suggestion.
facelessuser commentedon Aug 8, 2023
I like this approach
Add PY3.12 & PYPY3.10 and drop 3.7