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

Update the supported coverage versions in ansible-test #80427

Closed
mattclay opened this issue Apr 5, 2023 · 6 comments · Fixed by #81077
Closed

Update the supported coverage versions in ansible-test #80427

mattclay opened this issue Apr 5, 2023 · 6 comments · Fixed by #81077
Assignees
Labels
affects_2.16 ansible-test Release work related to ansible-test feature This issue/PR relates to a feature request. has_pr This issue has an associated PR. P2 Priority 2 - Issue Blocks Release

Comments

@mattclay
Copy link
Member

mattclay commented Apr 5, 2023

Summary

Add support for the next version of coverage as needed to support Python 3.12.

Issue Type

Feature Idea

Component Name

ansible-test

@mattclay mattclay added the P2 Priority 2 - Issue Blocks Release label Apr 5, 2023
@mattclay mattclay added this to TODO: proposed items in ansible-core 2.16 via automation Apr 5, 2023
@mattclay mattclay moved this from TODO: proposed items to Phase 1 (May 01 - Jun 23) in ansible-core 2.16 Apr 5, 2023
@ansibot

This comment was marked as resolved.

@ansibot ansibot added affects_2.16 feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. labels Apr 5, 2023
@mattclay mattclay removed the needs_triage Needs a first human triage before being processed. label Apr 5, 2023
@webknjaz webknjaz self-assigned this May 9, 2023
@webknjaz
Copy link
Member

webknjaz commented May 9, 2023

@mattclay do you have any specific incompatibilities in mind?

@mattclay
Copy link
Member Author

mattclay commented May 9, 2023

@webknjaz That's something you'll need to research, to determine if there are any compatibility or performance issues with coverage 6.5.0 (the latest version currently used by ansible-test) on Python 3.12. I'd keep an eye on any issues or PRs for coverage that mention Python 3.12 compatibility during the development cycle. You should coordinate with @s-hertel, who is assigned to add support for Python 3.12, in case there ends up being a blocking issue there.

It's entirely possible that the only issues will be lack of support for syntax not present in Python 3.10 (which will become our minimum supported controller Python version). If that's the case, we may not need to support a newer version of coverage and you can just report that here and close the issue. We shouldn't upgrade simply because a newer version is available (that will almost always be the case) -- and instead focus on whether or not the supported versions handle our use case.

@mattclay
Copy link
Member Author

mattclay commented May 9, 2023

As for specific issues, one of the most likely will be if no wheels for Python 3.12 are published for 6.5.0. If that's the case, then we'll want to add a version which does have wheels.

@mattclay
Copy link
Member Author

The coverage 7.2.7 release is the first to include wheels for Python 3.12, so we'll need to use that. Since it still supports Python 3.7 and the schema version hasn't changed, we can probably just use it to replace 6.5.0.

However, we'll need to look over the release notes and see if there are any changes in behavior that will impact our usage.

Taking a quick look, it appears that 7.0.0b1 introduced a change to file pattern matching that's likely to affect our usage. If so, handling that will likely be complicated, as we'll need to generate version specific configuration files and make sure the correct one is used in each environment.

A few other changes in 7.0.0b1 that may also affect us (more research required):

  • Combining data files with coverage combine now hashes the data files
  • When searching for completely un-executed files, coverage.py uses the presence of __init__.py

It's also possible I missed something, or that there were changes not covered in the release notes.

We'll also need to keep an eye on further coverage releases during the 2.16 development cycle and update to newer versions if there are relevant bug fixes before our code freeze.

@webknjaz Once you've determined what changes affect us, if any, let me know so we can figure out how we're going to address them.

@sivel sivel added the ansible-test Release work related to ansible-test label Jun 7, 2023
@webknjaz
Copy link
Member

https://coverage.readthedocs.io/en/7.2.7/migrating.html#migrating-to-python-3-12:

  • Python 3.12 now inlines list, dict, and set comprehensions. Previously, they were compiled as functions that were called internally. Coverage.py would warn you if comprehensions weren’t fully completed, but this no longer happens with Python 3.12.

This is just something curious I noticed.


https://coverage.readthedocs.io/en/7.2.7/migrating.html#migrating-to-coverage-py-7-x:

  • The way that wildcards when specifying file paths work in certain cases has changed in 7.x:

    • Previously, * would incorrectly match directory separators, making precise matching difficult. Patterns such as *tests/* will need to be changed to */tests/*.

    • ** now matches any number of nested directories. If you wish to retain the behavior of **/tests/* in previous versions then */**/tests/* can be used instead.

    • When remapping file paths with [paths], a path will be remapped only if the resulting path exists. Ensure that remapped [paths] exist when upgrading as this is now being enforced.

  • The [report] exclude_also setting is new in 7.2.0. It adds exclusion regexes while keeping the default built-in set. It’s better than the older [report] exclude_lines setting, which overwrote the entire list. Newer versions of coverage.py will be adding to the default set of exclusions. Using exclude_also will let you benefit from those updates.

nedbat/coveragepy@ec6205a implemented this.
It seems to me that the only change needed for us would be * -> ** with how we generate the configs.


https://coverage.readthedocs.io/en/7.2.7/config.html#report-include-namespace-packages:

[report] include_namespace_packages

(boolean, default False) When searching for completely un-executed files, include directories without __init__.py files. These are implicit namespace packages, and are usually skipped.

New in version 7.0.

Looking at the PR nedbat/coveragepy#1387, when this is unset, the behavior seems to remain unchanged as it was before v7. If we plan to use namespace packages in the future, it may be worth adding later but it shouldn't be necessary for the compatibility at the moment.


Combining data files with coverage combine now hashes the data files to skip files that add no new information. This can reduce the time needed. Many details affect the speed-up, but for coverage.py’s own test suite, combining is about 40% faster. Closes issue 1483.

These are the patches: nedbat/coveragepy@09bc2d6 + nedbat/coveragepy@1cd6c9b. Judging by nedbat/coveragepy#1483 (comment), it should improve the combine performance by skipping the exact duplicate files. I hope we don't rely on duplicate files being present?

@ansibot ansibot added the has_pr This issue has an associated PR. label Jun 16, 2023
ansible-core 2.16 automation moved this from Phase 1 (May 01 - Jun 23) to Done Aug 30, 2023
@ansible ansible locked and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.16 ansible-test Release work related to ansible-test feature This issue/PR relates to a feature request. has_pr This issue has an associated PR. P2 Priority 2 - Issue Blocks Release
Projects
Development

Successfully merging a pull request may close this issue.

4 participants