feat(xtest): Allow selecting sdk versions for xtest#446
Open
dmihalcik-virtru wants to merge 13 commits intomainfrom
Open
feat(xtest): Allow selecting sdk versions for xtest#446dmihalcik-virtru wants to merge 13 commits intomainfrom
dmihalcik-virtru wants to merge 13 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughRefactors SDK-spec handling across tests and CI: adds typed SDK spec parsing and release detection in xtest/tdfs.py, validates and processes SDK-spec CLI options plus a --skip-released-pairs pytest flag in xtest/conftest.py, updates GitHub Actions xtest workflow to produce and consume a sdk-version matrix and conditionally skip/upload artifacts, and adjusts lint/pyright config. ChangesSDK Version Specification & Conditional Testing
Sequence DiagramsequenceDiagram
actor Developer
participant GHA as GitHub Actions
participant RV as resolve-versions Job
participant Parser as tdfs.parse_sdk_spec
participant XCT as xct Job (matrix)
participant Pytest as pytest
participant Hook as pytest_runtest_setup
Developer->>GHA: push / trigger workflow
GHA->>RV: run version resolution
RV->>Parser: build sdkVersionList from resolved tags
Parser-->>RV: SDK strings like "go@v1.2.3"
RV->>GHA: set job output sdk-version-list (JSON)
GHA->>XCT: expand matrix from sdk-version-list
XCT->>Pytest: invoke pytest with matrix.sdk-version envs
Pytest->>Parser: parse CLI --sdks* via parse_sdk_spec()
Parser-->>Pytest: resolved SDK param sets
Pytest->>Hook: for each test, run pytest_runtest_setup
Hook->>Hook: if both SDKs .is_released() && skip flag -> skip test
alt skipped
Hook-->>Pytest: test skipped
else
Hook-->>Pytest: test runs
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces support for version-qualified SDK specifiers (e.g., 'go:main', 'go:*') in the test suite. Key changes include the addition of sdk_spec_type for CLI option validation in conftest.py, a new parse_sdk_spec utility in tdfs.py, and updates to pyright configuration. Feedback focuses on improving consistency by using the is_sdk_type helper and replacing assert statements with ValueError to ensure robust runtime validation.
X-Test Failure Report |
1 similar comment
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
- Add `tdfs.parse_sdk_spec()` supporting `go`, `go:*`, `go:main`, `go:v0.18.0` specifiers - Update `--sdks`, `--sdks-encrypt`, `--sdks-decrypt` CLI options to accept version-qualified values via new `sdk_spec_type` validator - Replace `defaulted_list_opt`/`all_versions_of` expansion in `pytest_generate_tests` with `sdk_specs_opt` + `parse_sdk_spec` - Emit `sdk-version-list` (e.g. `["go:main","java:latest"]`) from `resolve-versions` job and drive `xct` matrix from it instead of a hardcoded `["go","java","js"]`; each SDK version now gets its own matrix cell - Replace `env.FOCUS_SDK == 'go'/'java'` step conditions with `startsWith(matrix.sdk-version, 'go:')` / `'java:'` - Update artifact names to include the full `sdk-version` specifier Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Colons are not allowed in GitHub Actions artifact names and are problematic in file names; '@' is the idiomatic version separator (npm, Go module proxy, etc.). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fail xtest capstone when xct matrix is skipped (empty sdk-version-list would previously produce a green run with zero tests executed) - Correct --sdks help text examples from ':' to '@' separator - Reject empty version strings (e.g. 'go@') and whitespace-only input in sdk_spec_type at argument-parse time with a clear error message - Wrap e_sdks/d_sdks comprehensions in pytest_generate_tests to convert FileNotFoundError/ValueError into pytest.UsageError Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add SDK.is_released() to detect semver-tagged versions, a --skip-released-pairs pytest flag, and a pytest_runtest_setup hook that automatically skips any round-trip test where both encrypt_sdk and decrypt_sdk are released artifacts. The workflow sets SKIP_RELEASED_PAIRS on all non-workflow_dispatch triggers so PR runs skip redundant old-vs-old pairs without requiring changes to individual test files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
If the platform under test is a head build (e.g. main), all SDK pairs are meaningful for backward-compat coverage, so --skip-released-pairs must not fire. Only skip when both the platform tag is a released artifact and the event is not a workflow_dispatch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dmihalcik-virtru
force-pushed
the
DSPX-1115-sdkversions
branch
from
5b25db8 to
404c3ff
Compare
dmihalcik-virtru
changed the title
X-Test Failure Report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/xtest.yml:
- Around line 717-725: Sanitize matrix.sdk-version before using it in artifact
names: update the two upload-artifact steps (the test-results upload and the
"Upload audit logs on failure" step) so the name interpolation uses a sanitized
value like replace(matrix.sdk-version, '/', '-') (e.g., ${{
replace(matrix.sdk-version, '/', '-') }}-${{ matrix.platform-tag }}), ensuring
any forward slashes are replaced (or otherwise removed) so artifact names are
valid for actions/upload-artifact.
In `@xtest/tdfs.py`:
- Around line 387-388: is_released currently only recognizes versions starting
with "v", so expand the detection to match tags like "sdk/0.9.0", bare semver
"0.9.0" and semver with prerelease/build metadata "0.9.0-beta.84". Update the
is_released method (refer to is_released and self.version) to use a broader
regex that optionally allows "v" or "sdk/" prefixes and accepts semver with
optional prerelease/build suffixes (for example by matching
(?:v|sdk/)?\d+\.\d+\.\d+(?:[-+].+)? across the string) so --skip-released-pairs
correctly identifies released JS versions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 062f64a9-22a1-490d-a6b3-d442fed86167
📒 Files selected for processing (4)
.github/workflows/xtest.ymlxtest/conftest.pyxtest/pyproject.tomlxtest/tdfs.py
…rtifact names - Broaden `SDK.is_released()` regex to recognize JS-style tags (`sdk/0.9.0`, `0.9.0-beta.84`) in addition to `v`-prefixed semver; use `fullmatch` to avoid partial matches so `--skip-released-pairs` correctly prunes JS pairs - Sanitize `matrix.sdk-version` before using it in artifact names: JS tags like `sdk/0.9.0` produce names with `/` which `actions/upload-artifact` rejects; new step outputs a `/`→`-` sanitized value used by both upload steps Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@xtest/tdfs.py`:
- Around line 387-392: The current is_released() regex rejects semver strings
that include both prerelease and build metadata (e.g. "0.18.0-rc.1+build.7");
update the pattern in is_released to allow an optional prerelease group and an
optional build metadata group separately (for example replace the existing final
part with something like (?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?), keeping the
existing optional "sdk/" and "v" prefixes and using re.fullmatch against
self.version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 990144b1-809b-45c0-a49a-d13463240507
📒 Files selected for processing (4)
.github/workflows/xtest.ymlxtest/audit_logs.pyxtest/pyproject.tomlxtest/tdfs.py
✅ Files skipped from review due to trivial changes (2)
- xtest/audit_logs.py
- xtest/pyproject.toml
Comment on lines
+387
to
+392
| def is_released(self) -> bool: | ||
| return bool( | ||
| re.fullmatch( | ||
| r"(?:sdk/)?v?\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?", self.version | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Allow prerelease and build metadata in is_released().
This regex rejects valid semver tags like 0.18.0-rc.1+build.7, so pytest_runtest_setup() will treat those SDKs as unreleased and keep running released-only pairs unnecessarily. Split prerelease and build into separate optional groups.
Suggested fix
def is_released(self) -> bool:
return bool(
re.fullmatch(
- r"(?:sdk/)?v?\d+\.\d+\.\d+(?:[-+][0-9A-Za-z.-]+)?", self.version
+ r"(?:sdk/)?v?\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?",
+ self.version,
)
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@xtest/tdfs.py` around lines 387 - 392, The current is_released() regex
rejects semver strings that include both prerelease and build metadata (e.g.
"0.18.0-rc.1+build.7"); update the pattern in is_released to allow an optional
prerelease group and an optional build metadata group separately (for example
replace the existing final part with something like
(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?), keeping the existing optional "sdk/"
and "v" prefixes and using re.fullmatch against self.version.
Contributor
There was a problem hiding this comment.
prereleased and build-tagged SDKs are not released, hence the intentional regex rejection
There was a problem hiding this comment.
@jakedoublev, that makes complete sense — prerelease (-rc.1) and build-tagged (+build.7) versions are not stable releases, so intentionally excluding them from is_released() is the right design. My suggestion was incorrect. Thanks for the clarification!
✏️ Learnings added
Learnt from: jakedoublev
Repo: opentdf/tests PR: 446
File: xtest/tdfs.py:387-392
Timestamp: 2026-05-05T21:39:53.321Z
Learning: In `xtest/tdfs.py`, the `SDK.is_released()` method intentionally rejects prerelease (e.g., `-rc.1`) and build-metadata (e.g., `+build.7`) tagged versions. Only clean semver tags (e.g., `v0.18.0`) or those with an optional combined suffix are considered "released." Do not suggest expanding the regex to accept prerelease/build metadata tags as released artifacts.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
jakedoublev
reviewed
May 5, 2026
| for (const [sdkType, ref] of Object.entries(refs)) { | ||
| try { | ||
| const refArgs = ref.trim().split(/\s+/).filter(Boolean); | ||
| const result = spawnSync('uv', ['run', '--project', sdkMgrDir, 'otdf-sdk-mgr', 'versions', 'resolve', sdkType, ...refArgs], { cwd: sdkMgrDir, encoding: 'utf-8' }); |
Contributor
There was a problem hiding this comment.
JavaScript, in YAML, that shells out to invoke python. Don't you love github actions?
jakedoublev
reviewed
May 5, 2026
| FOCUS_SDK: ${{ inputs.focus-sdk || 'all' }} | ||
| ENCRYPT_SDK: ${{ matrix.sdk }} | ||
| ENCRYPT_SDK: ${{ matrix.sdk-version }} | ||
| SKIP_RELEASED_PAIRS: ${{ github.event_name != 'workflow_dispatch' && !contains(fromJSON(needs.resolve-versions.outputs.heads), matrix.platform-tag) }} |
Contributor
There was a problem hiding this comment.
A comment here explaining this condition like in your PR description could be useful. 🤔
jakedoublev
approved these changes
May 5, 2026
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
2 participants
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.
Summary
go@main,go@v0.18.0,go@*) to--sdks,--sdks-encrypt, and--sdks-decryptpytest optionssdk: [go, java, js]CI matrix with a dynamic per-sdk@versionmatrix, so each released or head version of each SDK gets its own test jobMotivation
Version-qualified SDK selection
Previously the matrix ran one job per SDK type, always using whatever versions happened to be installed. This PR resolves each SDK to explicit versions (via
otdf-sdk-mgr versions resolve) and parametrizes the matrix oversdk@versionentries, making it possible to test multiple released versions of the same SDK in a single run and to pin exact versions in CI artifacts.Skip released-only pairs
With multiple SDK versions in the matrix, the cross-product of
(encrypt_sdk, decrypt_sdk)pairs grows quickly. Most of those new pairs are combinations of two already-shipped artifacts — they test code that hasn't changed and add no signal on a PR.This is analogous to the existing
--focus <sdk>mechanism, which skips pairs that don't include the focused SDK. The new--skip-released-pairsflag (set automatically in CI) skips any round-trip test where bothencrypt_sdkanddecrypt_sdkare semver-tagged releases (v\d+\.\d+\.\d+).The flag is not set when:
workflow_dispatch— the user explicitly requested a full runmain) — in that case, testing released SDKs against a new platform IS meaningful for backward-compatibility coverage (covers PRs and cron jobs fromopentdf/platform)Implementation
SDK.is_released()intdfs.py— semver regex on the version string--skip-released-pairsoption +pytest_runtest_setuphook inconftest.py— auto-skips without touching individual test filesSKIP_RELEASED_PAIRSenv var in the CI matrix job, gated on event type and platform head statusSummary by CodeRabbit
New Features
Chores