test(integration): lint the Python in ros2_medkit_integration_tests - #598
Open
bburda wants to merge 7 commits into
Open
test(integration): lint the Python in ros2_medkit_integration_tests#598bburda wants to merge 7 commits into
bburda wants to merge 7 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR closes the linting gap in ros2_medkit_integration_tests by registering a Python linter test (flake8) so the package is covered by colcon test --ctest-args -L linter, aligning CI lint behavior with the pre-commit hooks.
Changes:
- Register
ament_cmake_flake8+ament_flake8()underif(BUILD_TESTING)for the integration tests package. - Add the required
ament_cmake_flake8test dependency inpackage.xml. - Apply mechanical quote-style adjustments in a small set of Python tests to satisfy the configured flake8 rules.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_integration_tests/CMakeLists.txt | Adds ament_cmake_flake8 discovery and registers ament_flake8() in the BUILD_TESTING block. |
| src/ros2_medkit_integration_tests/package.xml | Adds ament_cmake_flake8 as a test dependency so the linter test is available in CI. |
| src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py | Quote-style adjustments in f-strings to satisfy flake8. |
| src/ros2_medkit_integration_tests/test/features/test_discovery_namespace_filter.test.py | Quote-style adjustments in assertion message. |
| src/ros2_medkit_integration_tests/test/features/test_discovery_legacy_mode.test.py | Quote-style adjustments in assertion messages (including multi-part f-string). |
| src/ros2_medkit_integration_tests/test/features/test_discovery_layer_policies.test.py | Quote-style adjustments in assertion messages. |
| src/ros2_medkit_integration_tests/test/features/test_discovery_gap_fill.test.py | Quote-style adjustments in assertion messages. |
| src/ros2_medkit_integration_tests/test/features/test_daisy_chain_aggregation.test.py | Switches f-string triple quotes to avoid quote-rule violations while preserving the literal content. |
| src/ros2_medkit_integration_tests/test/docker/introspection/test_systemd_introspection.py | Quote-style adjustments across decorators, requests, and asserts. |
| src/ros2_medkit_integration_tests/test/docker/introspection/test_container_introspection.py | Quote-style adjustments across decorators, requests, and asserts. |
bburda
force-pushed
the
fix/lint-integration-tests
branch
from
August 6, 2026 14:59
23b949a to
b667e4c
Compare
mfaferek93
reviewed
Aug 6, 2026
| # sits outside `colcon test -L linter`, so the documented lint command passes | ||
| # while none of these files are checked, and the pre-commit hooks are the | ||
| # first thing to see a problem. The rule set matches those hooks (see .flake8 | ||
| # and .pre-commit-config.yaml) so the two never disagree. |
Collaborator
There was a problem hiding this comment.
The parity claim doesn't hold: ament_flake8 pulls flake8-quotes, -builtins and -comprehensions, while the pre-commit hook installs blind-except, class-newline, docstrings and import-order - so Q, A and C4 run in CI and nowhere locally. The 117 quote-only lines rewritten in this PR are the evidence: those files passed the hooks with Q000 unreported. Adding those three to the hook's additional_dependencies makes the sentence true and stops the next requests.get("...") from turning quality.yml red after a green pre-commit run --all-files.
mfaferek93
reviewed
Aug 6, 2026
| # while none of these files are checked, and the pre-commit hooks are the | ||
| # first thing to see a problem. The rule set matches those hooks (see .flake8 | ||
| # and .pre-commit-config.yaml) so the two never disagree. | ||
| # |
Collaborator
There was a problem hiding this comment.
ament_flake8() without CONFIG_FILE reads its own bundled ini, so the repo .flake8 never applies here. The two configs are hand-synced today (the extend-ignore list) and will drift silently. Pass CONFIG_FILE pointing at the repo .flake8 so there is a single source.
bburda
force-pushed
the
fix/lint-integration-tests
branch
from
August 6, 2026 19:43
a7b1734 to
362b200
Compare
The package registered no linter, so its 92 Python files sat outside `colcon test -L linter` and the documented lint command reported success without checking any of them. The pre-commit hooks did check them, which made the gap show up as a push rejected after a clean local lint run. Register flake8 and correct the quote style it reports. The rewrite is mechanical: every touched file parses to an identical AST. ament_pep257 belongs here too - ros2_medkit_graph_watchdog is the only package in the tree that skips both flake8 and pep257 - but this package drifted while unchecked and 559 docstrings put the summary on the opening line. That is its own change. Closes #596
The package registers no linter, so flake8 never saw this file and it landed with its import groups out of order: a third-party import placed after requests, and a stdlib import in its own trailing group. Only the order changes. The import set is identical and the AST outside the import statements compares equal to the previous version.
ament_flake8() without CONFIG_FILE reads the ini bundled inside the ament_flake8 package, so the repository .flake8 had no effect here. The two carried the same extend-ignore list by hand and nothing would have reported it if they diverged. The call now points at the repository file. Both currently produce the same verdict, so no finding changes; what changes is that an edit to .flake8 now reaches this gate.
scripts/ and docs/ sit outside every ROS package, so no ament_flake8 run has ever checked them, and the pre-commit hook lacks the quote, builtins and comprehensions plugins that would have. Both files had drifted from the style the rest of the tree is held to: 198 double-quoted literals and 4 dict() calls in place of literals. Quotes were converted with a tokenizer pass that skips any literal containing a quote or a backslash, so no escaping decision is ever made automatically. That left 36 literals to convert by hand: 10 f-strings, which the tokenizer does not expose as plain STRING tokens, and 26 non-f-string literals the tokenizer skipped for containing a backslash (raw regex patterns and '\n' separators) but which contain no quote, so no escaping decision was needed for them either. generate_verification.py parses to an identical AST before and after. conf.py parses to an identical AST except at the four dict() call sites, now written as literals; their keys and values were verified equal by direct comparison, not by AST equality. conf.py was checked by building the documentation, and generate_verification.py by running it.
The hook installed blind-except, class-newline, docstrings and import-order. ament_flake8 depends on builtins, comprehensions, docstrings, import-order and quotes. So Q, A and C4 ran in quality.yml and nowhere locally, and a file could pass every hook and then turn CI red. The hook now installs all five ament_flake8 has, keeping the two it already had on top. That makes it a superset rather than a match: blind-except and class-newline stay local-only because ament_flake8 comments both out of its package.xml. The comment in the integration tests CMakeLists said the rule sets matched; it now says what is actually true.
…ndings in e2e tests These four e2e test files carried findings that predate this branch. Running flake8 with only D, I and E selected against the files as they stood at 930ca5d reproduces the same 15 findings fixed here: 4 D205, 1 D209, 4 D400, 2 E501 and 4 I100. Selecting Q, A and C4 against the same content finds nothing, so none of this is fallout from adding flake8-builtins, flake8-comprehensions or flake8-quotes to the pre-commit hook in the previous commit. D comes from flake8-docstrings and I from flake8-import-order, both already installed in the hook before that change. pre-commit only runs flake8 against files that are part of a commit, so nothing had exercised these four files since those two plugins were added. `pre-commit run flake8 --all-files` is what finally scanned them and turned up the findings fixed here: module and class docstring summaries reshaped into a single, period-terminated sentence on the opening line with a blank line before the description; the harness import in each file annotated with `# noqa: E402, I100` and the same explanation already used in test_param_drift_e2e.test.py, since the sys.path.insert one line above makes the import order this checker wants impossible; and two lines wrapped under the 99 column limit.
The ros2_medkit_integration_tests CMakeLists comment counted 563 docstrings that put their summary on the opening line and claimed every other package in the tree runs ament_pep257. The real count is 559, and ros2_medkit_graph_watchdog is the one package that still excludes both flake8 and pep257 for its Python files. The same comment also claimed the pre-commit hook installing the plugins ament_flake8 depends on means nothing the gate rejects can pass the hook. CI installs those plugins from apt while the hook's additional_dependencies are unpinned and resolve from PyPI, so the comment now states plugin coverage rather than an outcome guarantee. The pre-commit config comment had a matching grammar slip and did not say that the hook only checks the files a commit touches, unlike quality.yml, which lints the whole package. The ros2_medkit_graph_watchdog CMakeLists comment said no package in the workspace lints Python. ros2_medkit_integration_tests now does, so the comment is rewritten to say this package is the last one still excluding those checks.
bburda
force-pushed
the
fix/lint-integration-tests
branch
from
August 6, 2026 19:54
362b200 to
7923285
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
ros2_medkit_integration_testshas 91 Python files and registered no linter. Itsif(BUILD_TESTING)block set up the launch tests but never calledament_lint_auto_find_test_dependencies(), and the package had no flake8 test dependency. Every other package in the tree that ships Python registers one.So
./scripts/test.sh lint, andcolcon test --ctest-args -L linterbehind it, reported success while the largest Python package in the repository was not checked. The pre-commit hooks did check these files, so problems were still caught, but only at commit or push time. That is confusing: you run the documented lint command, it passes, and then the push is rejected. This happened twice in one session on the same branch, first for a line over the limit and then for a docstring rule.This registers flake8 for the package and corrects the quote style it reports.
The rewrite is mechanical and quote-only. Every changed file parses to an identical AST before and after, which was checked file by file. The conversion was done with the tokenizer and skipped any literal containing a quote or a backslash; the two that needed a different outer quote were done by hand.
ament_pep257also belongs here, and every other package that ships Python runs it. It is not in this change: the package drifted while it was unchecked, and 563 multi-line docstrings put the summary on the opening line, which is not what the rest of the tree does. Adding that checker means correcting those docstrings, which is a large mechanical change of its own and would collide with the branches currently open. It has its own issue.Issue
Type
Testing
colcon test --packages-select ros2_medkit_integration_tests --ctest-args -L linternow runs two linter tests and both pass. Before this change the same command ran none.ament_flake8reports zero findings for the package after the quote fixes.Every touched file was compared with
ast.dump(ast.parse(...))against its previous version. All eight are identical, so no behaviour changed.Checklist
No behaviour change and no public API change. The CMake comment records why only flake8 is registered for now and why
ament_lint_commonis not used here: it would also apply the C++ style checks to the demo nodes in this package, which is a separate decision.