Skip to content

test: gate ablytest sandbox provisioning behind an internal build tag#706

Closed
SimonWoolf wants to merge 1 commit into
mainfrom
sandbox-build-tag
Closed

test: gate ablytest sandbox provisioning behind an internal build tag#706
SimonWoolf wants to merge 1 commit into
mainfrom
sandbox-build-tag

Conversation

@SimonWoolf

@SimonWoolf SimonWoolf commented Jul 7, 2026

Copy link
Copy Markdown
Member

The ablytest package is importable, so anything it exports is callable by downstream consumers. Those helpers are intended only for running ably-go's own integration tests, not as a supported API.

We don't want to actually put any security barries in place, because we want SDK development to be open to all, and there's no way to verify whether a given test app will be used for SDK development or not. But we can at least add a build tag that makes it at obvious that this is not the intended usecase.

This is not a security boundary (the code can be copied, or an external caller can just specify that tag), but rather a polite request, intended to avoid misunderstandings and unintentional reliance.

Summary by CodeRabbit

  • Chores
    • Tightened integration test execution to run only with the new internal test tag, helping separate these tests from standard builds.
    • Updated test workflow output to continue generating per-protocol reports.
  • Documentation
    • Clarified integration test setup steps, including protocol-specific environment settings and the required build tags for new test files.

The ablytest package is importable, so anything it exports is callable by
downstream consumers. Those helpers are intended only for running
ably-go's own integration tests, not as a supported API.

We don't want to actually put any security barries in place, because we
want SDK development to be open to all, and there's no way to verify
whether a given test app will be used for SDK development or not. But we
can at least add a build tag that makes it at obvious that this is not the
intended usecase.

This is not a security boundary (the code can be copied, or an external
caller can just specify that tag), but rather a polite request, intended
to avoid misunderstandings and unintentional reliance.

CI and CONTRIBUTING are updated to pass -tags=integration,ably_internal_sdk_tests_only.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@SimonWoolf SimonWoolf requested a review from lmars July 7, 2026 15:35
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 724b2641-02c2-43a7-b60f-6ede561da890

📥 Commits

Reviewing files that changed from the base of the PR and between aab2209 and 77861f5.

📒 Files selected for processing (23)
  • .github/workflows/integration-test.yml
  • CONTRIBUTING.md
  • ably/auth_integration_test.go
  • ably/event_emitter_integration_test.go
  • ably/event_emitter_spec_integration_test.go
  • ably/http_paginated_response_integration_test.go
  • ably/main_integration_test.go
  • ably/message_updates_integration_test.go
  • ably/proto_message_integration_test.go
  • ably/realtime_channel_delta_integration_test.go
  • ably/realtime_channel_integration_test.go
  • ably/realtime_channel_spec_integration_test.go
  • ably/realtime_client_integration_test.go
  • ably/realtime_conn_integration_test.go
  • ably/realtime_conn_spec_integration_test.go
  • ably/realtime_presence_integration_test.go
  • ably/recovery_context_test.go
  • ably/rest_channel_integration_test.go
  • ably/rest_channel_spec_integration_test.go
  • ably/rest_client_integration_test.go
  • ably/rest_presence_spec_integration_test.go
  • ably/transitioner_integration_test.go
  • ablytest/sandbox.go

Walkthrough

This PR introduces a new Go build tag, ably_internal_sdk_tests_only, required in addition to !unit for compiling integration test files and the sandbox helper. The CI workflow and CONTRIBUTING.md documentation are updated to pass this tag when running go test.

Changes

Build tag gating for integration tests

Layer / File(s) Summary
CI workflow and contributing docs
.github/workflows/integration-test.yml, CONTRIBUTING.md
Updates go test invocations to include the ably_internal_sdk_tests_only tag and per-protocol ABLY_PROTOCOL env vars; documents the new required build constraint for integration tests.
Sandbox helper build constraint
ablytest/sandbox.go
Adds //go:build !unit && ably_internal_sdk_tests_only and matching legacy // +build directive.
Integration test files build constraints
ably/auth_integration_test.go, ably/event_emitter_integration_test.go, ably/event_emitter_spec_integration_test.go, ably/http_paginated_response_integration_test.go, ably/main_integration_test.go, ably/message_updates_integration_test.go, ably/proto_message_integration_test.go, ably/realtime_channel_delta_integration_test.go, ably/realtime_channel_integration_test.go, ably/realtime_channel_spec_integration_test.go, ably/realtime_client_integration_test.go, ably/realtime_conn_integration_test.go, ably/realtime_conn_spec_integration_test.go, ably/realtime_presence_integration_test.go, ably/recovery_context_test.go, ably/rest_channel_integration_test.go, ably/rest_channel_spec_integration_test.go, ably/rest_client_integration_test.go, ably/rest_presence_spec_integration_test.go, ably/transitioner_integration_test.go
All these files change their //go:build and legacy // +build lines from !unit to !unit && ably_internal_sdk_tests_only, restricting when each file is compiled.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • ably/ably-go#705: Also modifies ably/main_integration_test.go's TestMain execution, affecting compilation/run conditions shared with this PR's build tag changes.

Suggested reviewers: lmars

Poem

A tag was born, so small yet mighty,
"ably_internal_sdk_tests_only," a bit flighty,
Twenty files stood in a row,
Each getting the same build-tag glow,
Hop hop, CI runs clean and bright! 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title matches the main intent of gating internal test helpers behind an internal build tag, though it omits the broader integration test tag updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sandbox-build-tag

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@SimonWoolf

Copy link
Copy Markdown
Member Author

replacing with #707

@SimonWoolf SimonWoolf deleted the sandbox-build-tag branch July 7, 2026 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant