Skip to content

fix: binary data support is not tested properly.#135

Open
ehsavoie wants to merge 1 commit into
a2aproject:mainfrom
ehsavoie:issue_binary_data_support
Open

fix: binary data support is not tested properly.#135
ehsavoie wants to merge 1 commit into
a2aproject:mainfrom
ehsavoie:issue_binary_data_support

Conversation

@ehsavoie

@ehsavoie ehsavoie commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

When a SUT returns a non--32601 error (e.g., a processing error), the test fails with "Binary data should be supported in A2A v1.0.0", which is confusing because the server does support it -> it just had a different error.
The intent is: if the server returns an error, it should NOT be a "method not found" (-32601) error, because binary data handling should be a supported feature. But the current code asserts the opposite -> it passes only if the error IS "method not found", which contradicts the
error message.

Fixes #<issue_number_goes_here> 🦕

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a confusing test failure message in the test_binary_data_handling test. Previously, the test incorrectly asserted that a "method not found" error was expected, leading to misleading failures when the system under test returned other types of errors. The change corrects this assertion to accurately reflect that a "method not found" error should not occur, aligning the test with the expected behavior of binary data support.

Highlights

  • Test Assertion Logic: The assertion in test_binary_data_handling was inverted, causing the test to pass only if a "method not found" error (-32601) was returned, which contradicted the expected behavior for supported binary data handling. This has been corrected to assert that this specific error code is not returned.
Changelog
  • tests/optional/capabilities/test_transport_specific_features.py
    • Corrected the assertion logic in test_binary_data_handling from == -32601 to != -32601 to properly validate that a "method not found" error is not returned.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@ehsavoie ehsavoie force-pushed the issue_binary_data_support branch from 5eaa124 to 0329562 Compare March 9, 2026 14:24
@ehsavoie ehsavoie changed the title When a SUT returns a non--32601 error (e.g., a processing error), the… fix: binary data support is not tested properly. Mar 9, 2026
@ehsavoie ehsavoie requested a review from jmesnil March 9, 2026 14:24

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request corrects an assertion in the test_binary_data_handling test within tests/optional/capabilities/test_transport_specific_features.py. The change modifies the assertion from assert error.get("code") == -32601 to assert error.get("code") != -32601, ensuring that if an error occurs during binary data handling, it is not a 'Method Not Found' error, which aligns with the expectation that binary data should be supported in A2A v1.0.0.

@ehsavoie ehsavoie force-pushed the issue_binary_data_support branch from 0329562 to dfbc57b Compare March 9, 2026 14:36
When a SUT returns a non--32601 error (e.g., a processing error), the test fails with "Binary data should be supported in A2A v1.0.0", which is confusing because the server does support it ->  it just had a different error.
The intent is: if the server returns an error, it should NOT be a "method not found" (-32601) error, because binary data handling should be a supported feature. But the current code asserts the opposite -> it passes only if the error IS "method not found", which contradicts the
error message.

Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
@millsmillsymills

Copy link
Copy Markdown
Contributor

Review: core fix correct, but blocked by conflicts + incomplete coverage

Blocked: branch is behind main and CONFLICTING/DIRTY (target file relocated/changed upstream). Needs a rebase before it can merge.

The assertion flip is correct and worth keepingtest_transport_specific_features.py:514:

  • Old: assert error.get("code") == -32601, "Binary data should be supported..." contradicted its own message (passed only when the server returned method-not-found, i.e. did not support binary data).
  • New: assert error.get("code") != -32601 now matches intent.

Remaining gaps (suited to a follow-up):

  • The filename addition (:499) is schema-valid but no assertion verifies the SUT honors/echoes it — purely additive input.
  • The test still passes for SUTs that don't actually support binary data: any non--32601 error (e.g. -32603) satisfies it. It asserts "not method-not-found" rather than "binary data was actually processed." The PR title promises more than the diff delivers.
  • ruff format would fix "filename":"test.txt" spacing; fill in the Fixes #... placeholder.

Recommendation: rebase onto main, keep the assertion fix, and strengthen the test to positively verify binary processing. Tracked in a follow-up issue.

@millsmillsymills

Copy link
Copy Markdown
Contributor

Fix intent is sound (non--32601 errors shouldn't fail the binary-data test). Blocked on merge: branch conflicts with main and needs a rebase. @author — could you rebase onto current main? Happy to re-review once conflicts are resolved.

@millsmillsymills

Copy link
Copy Markdown
Contributor

Heads-up: the binary-data coverage this PR targets is already on main. The tests/optional tree was restructured into tests/compatibility/, and tests/compatibility/core_operations/test_artifacts.py (TestFileArtifact/TestFileUrlArtifact, DM-ART-001) now positively asserts the file part variant (raw/url), filename == "output.txt", and mediaType — i.e. the strengthening goal here, with no -32601 pass-through. This PR edits the removed path and is CONFLICTING; rebasing would duplicate/regress the existing tests. Suggest closing in favour of the test_artifacts.py coverage. Context in #189.

@millsmillsymills

Copy link
Copy Markdown
Contributor

Review findings — change is correct, needs rebase

The assertion fix is right. The old assert error.get("code") == -32601, "Binary data should be supported in A2A v1.0.0" contradicted its own message — it asserted the error is MethodNotFound, i.e. binary data is not supported. Flipping to != -32601 matches the intent: a v1.0.0 SUT must not reject binary parts with method-not-found. Adding filename to the part also makes it a valid FilePart. 👍

Non-blockers:

  • 🔵 Style: "filename":"test.txt" — missing space after colon; ruff format will fix.
  • ⚠️ This PR is currently DIRTY (merge conflicts with main); needs a rebase before it can merge.

Reviewed via automated multi-agent PR review; external contributor (read-only).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants