You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "zwoop (via GitHub)" <gi...@apache.org> on 2026/03/12 06:24:48 UTC
[PR] hrw4u: Add code coverage support for tests [trafficserver]
zwoop opened a new pull request, #12957:
URL: https://github.com/apache/trafficserver/pull/12957
- Adds some new tests, for additional coverage (e.g. debug runs)
- Eliminates some dead code discovered when there's no way to get the coverage over such code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] hrw4u: Add code coverage support for tests [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop merged PR #12957:
URL: https://github.com/apache/trafficserver/pull/12957
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] hrw4u: Add code coverage support for tests [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #12957:
URL: https://github.com/apache/trafficserver/pull/12957#discussion_r2956491760
##########
tools/hrw4u/Makefile:
##########
@@ -169,6 +169,14 @@ $(PKG_DIR_LSP)/%: src/%
test:
uv run pytest --tb=short tests
+coverage:
+ uv run pytest --cov --cov-report=term-missing --cov-report=html tests
+ @echo ""
+ @echo "HTML report: open htmlcov/index.html"
Review Comment:
I hit an issue with the new coverage workflow on a clean checkout: `make coverage` fails before pytest starts because the package is still installed from `build/`, but that directory does not exist until the generated sources have been created.
In my repro, `make coverage` failed with:
error in 'egg_base' option: 'build' does not exist or is not a directory
After generating the sources first, the suite passed.
Could we make `coverage` ensure `build/` exists first (for example by depending on `gen`, or otherwise preparing the generated package layout), so the documented workflow works from a fresh checkout?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] hrw4u: Add code coverage support for tests [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #12957:
URL: https://github.com/apache/trafficserver/pull/12957#issuecomment-4714038614
Cherry-picked to 10.2.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] hrw4u: Add code coverage support for tests [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #12957:
URL: https://github.com/apache/trafficserver/pull/12957#issuecomment-4048390452
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] hrw4u: Add code coverage support for tests [trafficserver]
Posted by "zwoop (via GitHub)" <gi...@apache.org>.
zwoop commented on PR #12957:
URL: https://github.com/apache/trafficserver/pull/12957#issuecomment-4048667750
[approve ci autest 0]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] hrw4u: Add code coverage support for tests [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12957:
URL: https://github.com/apache/trafficserver/pull/12957#discussion_r2922570448
##########
tools/hrw4u/Makefile:
##########
@@ -167,6 +167,14 @@ $(PKG_DIR_LSP)/%: src/%
test:
uv run pytest --tb=short tests
+coverage:
+ uv run pytest --cov --cov-report=term-missing --cov-report=html tests
+ @echo ""
+ @echo "HTML report: open htmlcov/index.html"
+
+coverage-open: coverage
+ open htmlcov/index.html
Review Comment:
`coverage-open` uses the `open` command, which is macOS-specific and will fail on Linux/BSD environments. Consider using a more portable approach (e.g., `python -m webbrowser htmlcov/index.html`, or conditional `xdg-open`/`open`) so the target works across common dev/CI platforms.
```suggestion
@echo "HTML report: use 'make coverage-open' or open htmlcov/index.html in a browser"
coverage-open: coverage
uv run python -m webbrowser htmlcov/index.html
```
##########
tools/hrw4u/src/common.py:
##########
@@ -112,12 +112,7 @@ def fatal(message: str) -> NoReturn:
def create_base_parser(description: str) -> tuple[argparse.ArgumentParser, argparse._MutuallyExclusiveGroup]:
"""Create base argument parser with common options."""
parser = argparse.ArgumentParser(description=description, formatter_class=argparse.RawDescriptionHelpFormatter)
- parser.add_argument(
- "input_file",
- help="The input file to parse (default: stdin)",
- nargs="?",
- type=argparse.FileType("r", encoding="utf-8"),
- default=sys.stdin)
+ parser.add_argument("input_file", help="The input file to parse (default: stdin)", nargs="?", default=None)
Review Comment:
`create_base_parser()` advertises "default: stdin" but the new `input_file` argument defaults to `None` and no longer uses `argparse.FileType`. That makes `args.input_file` inconsistent with `process_input()` (which expects a `TextIO`) and breaks any callers that previously relied on getting an open file handle / stdin fallback. Consider restoring `type=argparse.FileType(...), default=sys.stdin` or updating the argument name/help/type annotations and any downstream usage to reflect that this is now a path/None rather than a file object.
```suggestion
parser.add_argument(
"input_file",
help="The input file to parse (default: stdin)",
nargs="?",
type=argparse.FileType("r"),
default=sys.stdin,
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org