[Experiment] Unit test coverage#17466
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces template coverage measurement for Jinja templates by adding a custom coverage plugin (jinja_coverage.py), a configuration file (.coveragerc-templates), and new Nox sessions (template_coverage, downstream_golden_tests, and showcase_prerelease_deps). It also adds integration test configurations and a unit test to render goldens for coverage. The review feedback highlights several critical improvements: resolving a potential NameError in noxfile.py by using os.path instead of path, avoiding fragile hardcoded line exclusions in jinja_coverage.py in favor of inline pragmas, and adding robustness checks for missing configuration options and Jinja loader attributes.
| if self.filename.endswith("test_macros.j2"): | ||
| excluded.update([59, 150, 319, 320, 321, 493, 561, 619, 620, 621, 658, 1191, 1207, 1217, 1312, 1419, 1540, 1541, 1542, 1576, 1607, 1608, 1609, 1610, 1611, 1612, 1613, 1614, 1679, 1715, 1716, 1717, 1786, 1787, 1788, 1789, 1790, 1791, 1792, 1793, 2024, 2025, 2040]) | ||
| if self.filename.endswith("_client_macros.j2"): | ||
| excluded.update([43, 65, 84, 133, 134, 137, 194, 199, 220, 222]) | ||
| if self.filename.endswith("client.py.j2"): | ||
| excluded.update([71, 680, 681]) | ||
| if self.filename.endswith("async_client.py.j2"): | ||
| excluded.update([52, 321, 442]) | ||
| if self.filename.endswith("transports/base.py.j2"): | ||
| excluded.update([46, 51, 164, 170, 174, 175, 292]) | ||
| if self.filename.endswith("transports/grpc.py.j2"): | ||
| excluded.update([50, 340]) | ||
| if self.filename.endswith("transports/grpc_asyncio.py.j2"): | ||
| excluded.update([54, 345]) | ||
| if self.filename.endswith("transports/_mixins.py.j2"): | ||
| excluded.update([172, 199]) | ||
| if self.filename.endswith("services/%service/_mixins.py.j2"): | ||
| excluded.update([291, 298, 301, 308, 311, 321, 412, 419, 426, 433, 447, 534, 541, 552, 559]) | ||
| if self.filename.endswith("services/%service/_async_mixins.py.j2"): | ||
| excluded.update([291, 298, 301, 308, 311, 321, 412, 419, 426, 433, 447, 534, 541, 552, 559]) | ||
| if self.filename.endswith("services/%service/_shared_macros.j2"): | ||
| excluded.update([27, 106, 133, 159, 172, 177, 313, 314, 316, 317, 319, 320, 323, 324]) | ||
| if self.filename.endswith("services/%service/pagers.py.j2"): | ||
| excluded.update([30]) | ||
| if self.filename.endswith("services/%service/transports/rest_asyncio.py.j2"): | ||
| excluded.update([188]) |
There was a problem hiding this comment.
Hardcoding specific line numbers for exclusion is highly fragile and will break whenever the templates are modified or lines are shifted. Since you already have a mechanism to exclude lines matching pragma: no cover (lines 106 and 139-142), you should use inline comments like {# pragma: no cover #} directly in the Jinja templates instead of maintaining these hardcoded lists of line numbers here.
| goldens_dir = path.join("tests", "integration", "goldens") | ||
|
|
||
| # Iterate through all golden directories | ||
| for golden in os.listdir(goldens_dir): | ||
| golden_path = path.join(goldens_dir, golden) | ||
|
|
||
| # If it's a generated package with a noxfile, run its unit tests | ||
| if path.isdir(golden_path) and path.exists(path.join(golden_path, "noxfile.py")): |
There was a problem hiding this comment.
Using path directly (e.g., path.join, path.isdir, path.exists) will raise a NameError if path is not imported from os. Since os is already imported (as seen in os.listdir), you should use os.path consistently.
| goldens_dir = path.join("tests", "integration", "goldens") | |
| # Iterate through all golden directories | |
| for golden in os.listdir(goldens_dir): | |
| golden_path = path.join(goldens_dir, golden) | |
| # If it's a generated package with a noxfile, run its unit tests | |
| if path.isdir(golden_path) and path.exists(path.join(golden_path, "noxfile.py")): | |
| goldens_dir = os.path.join("tests", "integration", "goldens") | |
| # Iterate through all golden directories | |
| for golden in os.listdir(goldens_dir): | |
| golden_path = os.path.join(goldens_dir, golden) | |
| # If it's a generated package with a noxfile, run its unit tests | |
| if os.path.isdir(golden_path) and os.path.exists(os.path.join(golden_path, "noxfile.py")): |
| def __init__(self, options): | ||
| self.template_directory = os.path.abspath(options.get("template_directory")) |
There was a problem hiding this comment.
If template_directory is missing from the options, options.get("template_directory") will return None, causing os.path.abspath to raise a TypeError. Adding a check with a clear error message improves robustness.
def __init__(self, options):
template_dir = options.get("template_directory")
if not template_dir:
raise ValueError("template_directory option is required for JinjaPlugin")
self.template_directory = os.path.abspath(template_dir)| if env and env.loader: | ||
| try: | ||
| co_filename = frame.f_code.co_filename | ||
| for search_path in env.loader.searchpath: |
There was a problem hiding this comment.
Not all Jinja loaders have a searchpath attribute (e.g., PackageLoader or DictLoader do not). Using getattr to safely retrieve searchpath avoids raising and catching unnecessary AttributeError exceptions.
if env and env.loader:
try:
co_filename = frame.f_code.co_filename
search_paths = getattr(env.loader, "searchpath", [])
for search_path in search_paths:1 participant
No description provided.