Address review comments: fix docstrings and restore test mismatch behavior by Copilot · Pull Request #13 · greenc-FNAL/phlex

Copilot · 2026-02-09T22:58:17Z

Addressed Exception Handling Feedback

Fixed overly broad exception catching in verify.py.

Change Made

verify.py (line 124): Changed except Exception: to except KeyError:

Rationale

The original code caught all exceptions, which could mask genuine errors from m.observe() or other unexpected failures. Since we only expect a KeyError when expected_bool is not present in the config, catching only that specific exception makes the error handling more precise and prevents masking other issues.

# Before - too broad
except Exception:
    pass

# After - specific and correct
except KeyError:
    pass

Verification

✅ Python file compiles successfully
✅ Exception handling now only catches expected config lookup errors
✅ Genuine failures will no longer be masked

This ensures that if there are actual problems with the verifier registration or observation, they will properly propagate as errors rather than being silently swallowed.

Original prompt

Please investigate and address test failures in the upstream PR associated with this branch remaining after the merge of @greenc-FNAL/phlex/pull/12

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

Copilot

Pull request overview

Updates Python test algorithms to align with the Python plugin’s name-based annotation lookup (introduced in PR Framework-R-D#213), so test call-site input_family labels match Python callable parameter names.

Changes:

Renamed Python function parameters across multiple test modules to match Jsonnet input_family labels.
Added specialized reducer functions / verifier class variants where different label sets are required.
Added/updated type annotations to satisfy stricter annotation lookup requirements.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
test/python/test_types.py	Renames parameters (f1/f2, d1/d2, u1/u2, b1/b2) to match configured input labels.
test/python/vectypes.py	Renames collectify parameters (u1/u2, l1/l2, ul1/ul2, f1/f2, d1/d2) to match input labels in vec-type configs.
test/python/test_callbacks.py	Renames callback parameters to match input labels; modifies mismatch test helper.
test/python/test_coverage.py	Renames scalar collection parameters (f→f1, d→d1) to match configured labels.
test/python/reducer.py	Adds distinct reducer functions so parameter names match intermediate label sets (sum0/sum1, sum2/sum3, etc.).
test/python/verify.py	Updates verifier call signatures to match observed labels; adds a `sum_ijk`-specific verifier path.
test/python/test_mismatch.py	Adds explicit return type annotation.

Copilot · 2026-02-09T23:16:01Z

test/python/test_types.py

+        d1 (float): First input.
+        d2 (float): Second input.


The collect_double docstring lists argument types as float, but the function takes double objects. Update the docstring types to match the signature.

Suggested change

d1 (float): First input.

d2 (float): Second input.

d1 (double): First input.

d2 (double): Second input.

Copilot · 2026-02-09T23:16:01Z

test/python/verify.py

        Raises:
            AssertionError: if the provided value does not matches the
-                pre-registed value.
+                pre-registered value.


Docstring grammar: "does not matches" should be "does not match" (also appears in the other verifier class below).

Copilot · 2026-02-09T23:16:02Z

test/python/verify.py

+        """Verify a the `sum_ijk`.
+
+        Check that `sum_ijk` matches the pre-registered value.


Docstring grammar: "Verify a the sum_ijk" should be corrected (e.g., "Verify the sum_ijk").

Copilot · 2026-02-09T23:16:02Z

test/python/verify.py

+        Raises:
+            AssertionError: if the provided value does not matches the
+                pre-registered value.


Docstring grammar: "does not matches" should be "does not match".

Copilot · 2026-02-09T23:16:02Z

test/python/test_types.py

+        d1 (float): First input.
+        d2 (float): Second input.

    Returns:
        float: Sum of the two inputs.


The add_double docstring still describes the parameters/return as float, but the function signature/return type are double. Update the docstring types so they match what the function actually accepts/returns.

Suggested change

d1 (float): First input.

d2 (float): Second input.

Returns:

float: Sum of the two inputs.

d1 (double): First input.

d2 (double): Second input.

Returns:

double: Sum of the two inputs.

Copilot · 2026-02-09T23:16:02Z

test/python/test_types.py

+        u1 (int): First input.
+        u2 (int): Second input.

    Returns:
        int: Sum of the two inputs.


The add_unsigned docstring says the return type is int, but the function is annotated to return "unsigned int". Update the docstring return type/description so it matches the annotation.

Suggested change

u1 (int): First input.

u2 (int): Second input.

Returns:

int: Sum of the two inputs.

u1 ("unsigned int"): First input.

u2 ("unsigned int"): Second input.

Returns:

"unsigned int": Sum of the two inputs.

Copilot · 2026-02-09T23:16:03Z

test/python/test_callbacks.py

+def two_args(i: int, j: int, k: int) -> int:  # noqa: ARG001
+    """Sum only the first two of three integers (tests parameter count mismatch).
+
+    Note: Parameter k is intentionally unused - this tests that the function
+    accepts 3 parameters to match input_family but doesn't use all of them.
+    """


two_args is used by pymismatch_variant.jsonnet, which intentionally provides 3 inputs to a 2-argument function to trigger the "number of inputs ... does not match number of annotation types" failure. Changing the signature to accept 3 parameters (and annotating all 3) removes that mismatch and will cause py:mismatch_variant to stop failing as expected. Keep two_args at 2 parameters (but rename them to match the input labels, e.g., i and j) so the config's extra k still triggers the mismatch error.

Suggested change

def two_args(i: int, j: int, k: int) -> int: # noqa: ARG001

"""Sum only the first two of three integers (tests parameter count mismatch).

Note: Parameter k is intentionally unused - this tests that the function

accepts 3 parameters to match input_family but doesn't use all of them.

"""

def two_args(i: int, j: int) -> int:

"""Sum two integers while config provides three inputs (tests parameter count mismatch)."""

Copilot · 2026-02-09T23:16:03Z

test/python/test_callbacks.py

-    return a + b
+def two_args(i: int, j: int, k: int) -> int:  # noqa: ARG001
+    """Sum only the first two of three integers (tests parameter count mismatch).
+


There is trailing whitespace on the blank line inside this docstring (violates the repo formatting rules and can fail whitespace checks). Remove the spaces so the line is empty.

Suggested change

Copilot · 2026-02-09T23:16:03Z

test/python/verify.py

+        """Verify a the `sum`.

-        Check that `value` matches the pre-registered value.
+        Check that `sum` matches the pre-registered value.


Docstring grammar: "Verify a the sum" should be corrected (e.g., "Verify the sum").

…signature Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

greenc-FNAL · 2026-02-09T23:28:31Z

@phlexbot python-fix

github-actions · 2026-02-09T23:28:50Z

Automatic ruff fixes pushed (commit a106387).
⚠️ Note: Some issues may require manual review and fixing.

greenc-FNAL · 2026-02-09T23:29:06Z

@phlexbot format

github-actions · 2026-02-09T23:29:26Z

No automatic markdownlint fixes were necessary.

github-actions · 2026-02-09T23:29:28Z

No automatic cmake-format fixes were necessary.

github-actions · 2026-02-09T23:29:34Z

No automatic jsonnetfmt fixes were necessary.

github-actions · 2026-02-09T23:29:35Z

No automatic clang-format fixes were necessary.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Copilot · 2026-02-09T23:31:06Z

test/python/verify.py

    except Exception:
        pass

-    assert_sum = Verifier(config["sum_total"])
+    # Check if this is for sum_ijk (from callback3 test)
+    if config["input"] == ["sum_ijk"]:


Catching Exception here will also swallow unexpected failures in the bool-verifier path (e.g., exceptions thrown by m.observe), then fall through into the numeric verifier logic and potentially raise misleading follow-on errors. Prefer catching only the expected config lookup/type errors (e.g., KeyError/TypeError) or guarding explicitly on the presence of expected_bool so genuine failures aren’t masked.

@copilot apply changes based on this feedback

Changed to catch only KeyError instead of broad Exception to avoid masking genuine failures. Commit: 5ab7b4a

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

…avior (#13) * Initial plan * Fix Python parameter names to match input_family labels Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> * Address code review feedback: fix typo and improve docstring Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> * Add comment explaining unused parameter k in test function Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> * Address copilot review comments: fix docstrings and restore two_args signature Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> * Apply ruff fixes * Narrow exception handling to catch only KeyError in verify.py Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Initial plan

a38f08b

Copilot AI assigned Copilot and greenc-FNAL Feb 9, 2026

Copilot started work on behalf of greenc-FNAL February 9, 2026 22:58 View session

Copilot AI and others added 3 commits February 9, 2026 23:05

Fix Python parameter names to match input_family labels

c9e2a31

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

Address code review feedback: fix typo and improve docstring

fdd8b4f

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

Add comment explaining unused parameter k in test function

da6b1ee

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

greenc-FNAL marked this pull request as ready for review February 9, 2026 23:12

Copilot AI review requested due to automatic review settings February 9, 2026 23:12

Copilot started reviewing on behalf of greenc-FNAL February 9, 2026 23:12 View session

Copilot AI changed the title ~~[WIP] Fix remaining test failures after merging upstream PR~~ Fix Python test parameter names for name-based annotation lookup Feb 9, 2026

Copilot AI requested a review from greenc-FNAL February 9, 2026 23:12

Copilot finished work on behalf of greenc-FNAL February 9, 2026 23:12

Copilot AI reviewed Feb 9, 2026

View reviewed changes

Copilot started work on behalf of greenc-FNAL February 9, 2026 23:20 View session

Address copilot review comments: fix docstrings and restore two_args …

5328021

…signature Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

Copilot AI changed the title ~~Fix Python test parameter names for name-based annotation lookup~~ Address review comments: fix docstrings and restore test mismatch behavior Feb 9, 2026

Copilot finished work on behalf of greenc-FNAL February 9, 2026 23:23

greenc-FNAL requested a review from Copilot February 9, 2026 23:26

Copilot started reviewing on behalf of greenc-FNAL February 9, 2026 23:26 View session

Apply ruff fixes

a106387

Copilot AI reviewed Feb 9, 2026

View reviewed changes

Copilot started work on behalf of greenc-FNAL February 9, 2026 23:33 View session

Narrow exception handling to catch only KeyError in verify.py

5ab7b4a

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>

Copilot finished work on behalf of greenc-FNAL February 9, 2026 23:35

greenc-FNAL merged commit d6a3278 into maintenance/improve-test-coverage Feb 9, 2026

		"""Verify a the `sum_ijk`.

		Check that `sum_ijk` matches the pre-registered value.

Conversation

Copilot AI commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Addressed Exception Handling Feedback

Change Made

Rationale

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

greenc-FNAL commented Feb 9, 2026

Uh oh!

github-actions bot commented Feb 9, 2026

Uh oh!

greenc-FNAL commented Feb 9, 2026

Uh oh!

github-actions bot commented Feb 9, 2026

Uh oh!

github-actions bot commented Feb 9, 2026

Uh oh!

github-actions bot commented Feb 9, 2026

Uh oh!

github-actions bot commented Feb 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

greenc-FNAL Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Feb 9, 2026 •

edited

Loading