fix(xtest): Skip tests for public_client_id mismatch for otdfctl #311
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @elizabethhealy, 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!
Summary of Changes
Hello team,
Gemini or gemini-code-assist here to provide a summary of this pull request. This PR addresses an issue in the xtest suite where tests involving the otdfctl (Go SDK CLI) fail when run against newer platform versions. The root cause is that the public_client_id configuration was removed from the platform in version 0.21.0 (for the Go SDK's perspective, based on the cli.sh logic) or after 0.5.5 (for the Python test framework's perspective, based on tdfs.py), but older versions of otdfctl still expect it. To resolve this, the PR introduces logic to detect this version skew and skip the affected tests gracefully, preventing false failures in the test matrix.
Highlights
- Test Skipping Logic: Introduced a new mechanism in the Python test framework (
tdfs.py) to detect when the Go SDK (otdfctl) is being tested against a platform version that no longer supports thepublic_client_idfeature. - Go SDK Feature Detection: Added a
supports public-client-idcheck to the Go SDK test script (xtest/sdk/go/cli.sh) to determine if a specific version ofotdfctlexpects thepublic_client_id. - Platform Feature Tracking: Updated the
PlatformFeatureSetclass intdfs.pyto correctly identify platform versions (<= 0.5.5) that support thepublic-client-id. - Test Suite Updates: Integrated the new
skip_public_client_id_skewfunction into numerous tests acrosstest_abac.py,test_legacy.py, andtest_tdfs.pyto apply the conditional skipping.
Changelog
Click here to see the changelog
- xtest/sdk/go/cli.sh
- Added a new
supports public-client-idcheck (lines 75-79) that determines if the Go SDK version is less than 0.21.0, indicating it expects thepublic_client_id.
- Added a new
- xtest/tdfs.py
- Updated
PlatformFeatureSet.__init__to addpublic-client-idto supported features if the platform version is 0.5.5 or older (lines 103-105). - Added a new function
skip_public_client_id_skew(lines 478-483) that skips a test if a Go SDK expectspublic-client-idbut the platform does not support it.
- Updated
- xtest/test_abac.py
- Added calls to
tdfs.skip_public_client_id_skewintest_autoconfigure_one_attribute_standard(line 45). - Added calls to
tdfs.skip_public_client_id_skewintest_autoconfigure_two_kas_or_standard(line 91). - Added calls to
tdfs.skip_public_client_id_skewintest_autoconfigure_double_kas_and(line 145). - Added calls to
tdfs.skip_public_client_id_skewintest_autoconfigure_one_attribute_attr_grant(line 199). - Added calls to
tdfs.skip_public_client_id_skewintest_autoconfigure_two_kas_or_attr_and_value_grant(line 247). - Added calls to
tdfs.skip_public_client_id_skewintest_autoconfigure_two_kas_and_attr_and_value_grant(line 302). - Added calls to
tdfs.skip_public_client_id_skewintest_autoconfigure_one_attribute_ns_grant(line 356). - Added calls to
tdfs.skip_public_client_id_skewintest_autoconfigure_two_kas_or_ns_and_value_grant(line 404). - Added calls to
tdfs.skip_public_client_id_skewintest_autoconfigure_two_kas_and_ns_and_value_grant(line 459).
- Added calls to
- xtest/test_legacy.py
- Added calls to
tdfs.skip_public_client_id_skewintest_decrypt_small(line 26). - Added calls to
tdfs.skip_public_client_id_skewintest_decrypt_big(line 48). - Added calls to
tdfs.skip_public_client_id_skewintest_decrypt_no_splitid(line 70). - Added calls to
tdfs.skip_public_client_id_skewintest_decrypt_object_statement_value_json(line 92).
- Added calls to
- xtest/test_tdfs.py
- Added calls to
tdfs.skip_public_client_id_skewintest_tdf_roundtrip(line 115). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_spec_target_422(line 167). - Added calls to
tdfs.skip_public_client_id_skewintest_manifest_validity(line 272). - Added calls to
tdfs.skip_public_client_id_skewintest_manifest_validity_with_assertions(line 290). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_assertions_unkeyed(line 319). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_assertions_with_keys(line 353). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_assertions_422_format(line 392). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_with_unbound_policy(line 565). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_with_altered_policy_binding(line 595). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_with_altered_root_sig(line 624). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_with_altered_seg_sig_wrong(line 654). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_with_altered_enc_seg_size(line 687). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_with_altered_assertion_statement(line 723). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_with_altered_assertion_with_keys(line 763). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_altered_payload_end(line 812). - Added calls to
tdfs.skip_public_client_id_skewintest_tdf_with_malicious_kao(line 845).
- Added calls to
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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 is currently in preview and 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 to provide feedback.
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
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the test failures caused by public_client_id mismatches between otdfctl and the platform. The changes are clear, well-targeted, and the logic for skipping tests appears sound.
Key improvements:
- A new
public-client-idfeature check is added toxtest/sdk/go/cli.shto determine if a givenotdfctlversion expects this feature. PlatformFeatureSetinxtest/tdfs.pynow correctly identifies if the platform version supportspublic-client-id.- The new
skip_public_client_id_skewfunction inxtest/tdfs.pyneatly encapsulates the logic to skip tests when the Go SDK (otdfctl) expectspublic_client_idbut the platform does not provide it. - This skip logic is consistently applied across relevant test files (
test_abac.py,test_legacy.py,test_tdfs.py).
The code quality is good, and the solution directly matches the problem described. Well done!
Summary of Findings
- Bash
awkversion check robustness: Inxtest/sdk/go/cli.sh, theawkcondition($1 == 0 && $2 < 21)for checkingotdfctlversion is robust for current0.minor.patchversioning. Ifotdfctlwere to have a major version bump (e.g.,1.0.0), this specific condition would need an update. This is a minor, future-proofing observation and not an immediate issue. (Severity: low, not commented due to review settings) - Specificity of
skip_public_client_id_skewto Go SDK: Theskip_public_client_id_skewfunction inxtest/tdfs.pyincludessdk.sdk == "go", making it specific to the Go SDK/otdfctl. This is appropriate given the PR's focus. If similarpublic_client_idversioning issues were to arise for other SDKs, this function might need generalization. (Severity: low, not commented due to review settings)
Merge Readiness
The pull request is in good shape and addresses the intended issue effectively. The changes are clear and maintainable. Based on this review, the PR appears ready for merging. As an AI, I am not authorized to approve pull requests; please ensure it undergoes any further required review processes.
xtest/tdfs.py
Outdated
| @@ -100,6 +100,10 @@ def __init__(self, **kwargs: dict[str, Any]): | |||
| if self.semver >= (0, 4, 28): | |||
| self.features.add("connectrpc") | |||
|
|
|||
| # public_client_id removed in service 0.5.6 | |||
| if self.semver >= (0, 5, 6): | |||
| self.features.add("no-public-client-id") | |||
There was a problem hiding this comment.
had to reverse the logic here bc main is still 0.5.5 because it hasnt been released yet but public_client_id has been removed
|
successful run with 0.15.0 https://github.com/opentdf/tests/actions/runs/15495688218 |
|
the public_client_id was removed from the platform configuration, skip tests if otdfctl expects but the platform does not have it