Copilot commented on code in PR #2052:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2052#discussion_r2469478646
##########
extensions/sql/tests/features/steps/postgress_server_container.py:
##########
@@ -41,4 +41,4 @@ def deploy(self) -> bool:
def check_query_results(self, query, number_of_rows):
(code, output) = self.exec_run(["psql", "-U", "postgres", "-c", query])
- return code == 0 and str(number_of_rows) + " rows" in output
+ return code == 0 and str(number_of_rows) + " row" in output
Review Comment:
The check for row count is incomplete. It will match both '1 row' and '1
rows', but for counts greater than 1, PostgreSQL outputs 'N rows' (plural). The
original code checked for 'rows' which would work for all counts >= 0. This
change breaks the check for counts != 1. Change to: `str(number_of_rows) + ('
row' if number_of_rows == 1 else ' rows')`
```suggestion
return code == 0 and (str(number_of_rows) + (" row" if
number_of_rows == 1 else " rows")) in output
```
##########
behave_framework/src/minifi_test_framework/containers/container.py:
##########
@@ -69,8 +72,8 @@ def deploy(self) -> bool:
"mode": directory.mode
}
for host_file in self.host_files:
- self.volumes[host_file.container_path] = {
- "bind": host_file.host_path,
+ self.volumes[host_file.host_path] = {
+ "bind": host_file.container_path,
Review Comment:
The volume binding is inverted. The dictionary key should be
`host_file.host_path` (the source on the host), and the 'bind' value should be
`host_file.container_path` (the target in the container). However, based on
Docker's volume syntax, this appears correct. The issue is that this
contradicts the pattern used for directories above (lines 69-73), where
`self.volumes[directory.path]` uses the container path as the key. This
inconsistency could lead to confusion or bugs if the directory pattern is the
intended standard.
##########
behave_framework/src/minifi_test_framework/containers/minifi_container.py:
##########
@@ -24,8 +24,8 @@
class MinifiContainer(Container):
- def __init__(self, image_name: str, scenario_id: str, network: Network):
- super().__init__(image_name, f"minifi-{scenario_id}", network)
+ def __init__(self, image_name: str, container_name: str, scenario_id: str,
network: Network):
+ super().__init__(image_name, f"{container_name}-{scenario_id}",
network)
Review Comment:
The constructor signature changed from `__init__(self, image_name: str,
scenario_id: str, network: Network)` to add a `container_name` parameter.
Ensure all call sites have been updated to pass the new parameter, as this is a
breaking change to the API.
##########
behave_framework/src/minifi_test_framework/core/hooks.py:
##########
@@ -32,28 +32,50 @@ def get_minifi_container_image():
return "apacheminificpp:behave"
+def inject_scenario_id(context: MinifiTestContext, step):
+ if "${scenario_id}" in step.name:
+ step.name = step.name.replace("${scenario_id}", context.scenario_id)
+ if getattr(step, "table", None):
+ for row in step.table:
+ row.cells = [cell.replace("${scenario_id}", context.scenario_id)
if "${scenario_id}" in cell else cell for cell in row.cells]
+
+
Review Comment:
[nitpick] The scenario_id injection modifies step names and table cells in
place. Consider documenting this side effect clearly, as mutating test step
data during initialization could make debugging more difficult. Also consider
whether this should be applied to step.text (multiline strings) for consistency.
```suggestion
"""
Injects the scenario_id into step data in place.
Mutates the step object by replacing '${scenario_id}' with the actual
scenario_id in:
- step.name
- step.table cells (if present)
- step.text (multiline string, if present)
This mutation occurs during scenario initialization and may affect
debugging or reporting.
"""
if "${scenario_id}" in step.name:
step.name = step.name.replace("${scenario_id}", context.scenario_id)
if getattr(step, "table", None):
for row in step.table:
row.cells = [cell.replace("${scenario_id}", context.scenario_id)
if "${scenario_id}" in cell else cell for cell in row.cells]
if hasattr(step, "text") and step.text and "${scenario_id}" in step.text:
step.text = step.text.replace("${scenario_id}", context.scenario_id)
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
