test(users): fix membership external_id test cases#173
test(users): fix membership external_id test cases#173srinivaskarre-sk wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors ChangesUser and Permission Test Rewrites
Estimated code review effort: 2 (Simple) | ~15 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 `@scalekit/core.py`:
- Around line 192-196: The exception handling in the token refresh block
silently swallows authentication failures in all cases, but this is only safe
when the token is near expiry with a valid buffer remaining. When
_token_needs_refresh() returns True because the token is missing or blank (as
checked in lines 157-158), failing to re-authenticate should not be silently
ignored. Modify the exception handling to distinguish between these two
scenarios: check if the token is definitively absent or blank before deciding
whether to swallow the exception from __authenticate_client(), and re-raise the
exception when the token is missing rather than applying the "token still valid
within buffer" comment logic to both cases.
- Around line 212-213: The grpc_exec method currently retries unconditionally on
all non-UNAUTHENTICATED errors, including deterministic failures like
INVALID_ARGUMENT, NOT_FOUND, and PERMISSION_DENIED that should never be retried.
Instead of the current approach at lines 212-213, add a condition to check if
the error status code is a transient/retryable error before proceeding with the
retry. Only retry on transient status codes such as UNAVAILABLE,
DEADLINE_EXCEEDED, RESOURCE_EXHAUSTED, and INTERNAL, while allowing
deterministic errors to fail immediately without retrying.
🪄 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: 318365fb-a621-4ecc-b4b6-5430074b68a8
📒 Files selected for processing (2)
scalekit/core.pytests/test_users.py
There was a problem hiding this comment.
🧹 Nitpick comments (2)
specs/fix-unauthenticated.md (2)
125-129: 💤 Low valueReduce repetitive sentence starters in the implementation list.
The implementation steps (lines 125–129) begin three consecutive items with "Add" (items 2, 3, and 4), which reduces readability. Varying the phrasing will improve clarity.
📝 Proposed fix
2. Add `TOKEN_EXPIRY_BUFFER_SECS = 300` constant. -3. Add `_token_expiry: float` and `_token_lock: threading.Lock` to `__init__`. +3. Store `_token_expiry: float` and `_token_lock: threading.Lock` in `__init__`. -4. Rename `__authenticate_client` → `_fetch_new_token`; extract JWT `exp` after fetch. +4. Rename `__authenticate_client` → `_fetch_new_token` and extract JWT `exp` after fetch. -5. Add `_token_needs_refresh() -> bool`. +5. Implement `_token_needs_refresh() -> bool`.🤖 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 `@specs/fix-unauthenticated.md` around lines 125 - 129, The implementation steps contain repetitive sentence starters with "Add" appearing in items 3, 5, and 6 of the list starting at line 125. To improve readability, vary the phrasing by using different action verbs and sentence structures. For example, replace some "Add" statements with alternative verbs like "Introduce", "Implement", "Create", or "Define", ensuring each item begins with a distinct phrase while maintaining the same technical content and clarity about what needs to be added or modified to the codebase.Source: Linters/SAST tools
60-62: 💤 Low valueAdd language specification to fenced code blocks for clarity.
Lines 60–62 and 103–108 contain fenced code blocks without language identifiers. For consistency with markdown best practices and to enable syntax highlighting, specify the language (e.g.,
```pythonor```plaintext).📝 Proposed fix
Line 60:
-``` +```python TOKEN_EXPIRY_BUFFER_SECS = 300 # 5 minutes -``` +```Line 103:
-``` +```plaintext on UNAUTHENTICATED: if tool_error_info present → raise immediately (connected account domain error) elif _auth_retry > 0 → force refresh + retry once else → raise -``` +```Also applies to: 103-108
🤖 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 `@specs/fix-unauthenticated.md` around lines 60 - 62, The fenced code blocks in the markdown file lack language identifiers, which prevents proper syntax highlighting and violates markdown best practices. Add the language specification to the opening fence of the code block containing TOKEN_EXPIRY_BUFFER_SECS by changing the opening fence from triple backticks to triple backticks followed by "python". Similarly, add the language specification to the opening fence of the code block starting with "on UNAUTHENTICATED:" by changing the opening fence to triple backticks followed by "plaintext". This ensures consistent formatting and enables syntax highlighting across both code blocks.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@specs/fix-unauthenticated.md`:
- Around line 125-129: The implementation steps contain repetitive sentence
starters with "Add" appearing in items 3, 5, and 6 of the list starting at line
125. To improve readability, vary the phrasing by using different action verbs
and sentence structures. For example, replace some "Add" statements with
alternative verbs like "Introduce", "Implement", "Create", or "Define", ensuring
each item begins with a distinct phrase while maintaining the same technical
content and clarity about what needs to be added or modified to the codebase.
- Around line 60-62: The fenced code blocks in the markdown file lack language
identifiers, which prevents proper syntax highlighting and violates markdown
best practices. Add the language specification to the opening fence of the code
block containing TOKEN_EXPIRY_BUFFER_SECS by changing the opening fence from
triple backticks to triple backticks followed by "python". Similarly, add the
language specification to the opening fence of the code block starting with "on
UNAUTHENTICATED:" by changing the opening fence to triple backticks followed by
"plaintext". This ensures consistent formatting and enables syntax highlighting
across both code blocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 316b76a1-5a72-4271-89b4-1c1104c10a55
📒 Files selected for processing (1)
specs/fix-unauthenticated.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_users.py (1)
428-462: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPartner org leaks if assertions fail before cleanup.
_create_partner_orgcleanup only runs at the end of the test body (Lines 458-461, 564-567). If any assertion earlier in the test fails (e.g. Line 450-456 or 553-562), thetry/exceptcleanup block is skipped and the partner org is never deleted, leaking test data on every failed run.Use
self.addCleanup(...)right after creating the partner org so deletion always runs regardless of test outcome:♻️ Suggested fix
+ def _safe_delete_org(self, organization_id: str): + try: + self.scalekit_client.organization.delete_organization(organization_id=organization_id) + except ScalekitNotFoundException: + pass + def test_create_membership_by_external_id(self): """Adding a Workday employee (by external ID) to a partner org.""" partner_org_id = self._create_partner_org() + self.addCleanup(self._safe_delete_org, partner_org_id) ... - try: - self.scalekit_client.organization.delete_organization(organization_id=partner_org_id) - except ScalekitNotFoundException: - passApply the same pattern to
test_delete_membership_by_external_id(and the pre-existingtest_create_membership, if desired).Also applies to: 533-568
🤖 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 `@tests/test_users.py` around lines 428 - 462, The partner org cleanup in `test_create_membership_by_external_id` is only executed at the end of the test, so any earlier assertion failure skips deletion and leaks test data. Move the `organization.delete_organization` cleanup into `self.addCleanup(...)` immediately after `_create_partner_org()` so it always runs, and apply the same pattern in `test_delete_membership_by_external_id` (and `test_create_membership` if kept consistent).
🤖 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.
Nitpick comments:
In `@tests/test_users.py`:
- Around line 428-462: The partner org cleanup in
`test_create_membership_by_external_id` is only executed at the end of the test,
so any earlier assertion failure skips deletion and leaks test data. Move the
`organization.delete_organization` cleanup into `self.addCleanup(...)`
immediately after `_create_partner_org()` so it always runs, and apply the same
pattern in `test_delete_membership_by_external_id` (and `test_create_membership`
if kept consistent).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94718fad-261c-4041-8907-a59a7693863b
📒 Files selected for processing (1)
tests/test_users.py
srinivaskarre-sk
force-pushed
the
fix/membership-external-id-tests
branch
from
1fc9d0e to
85bf72a
Compare
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 `@tests/test_users.py`:
- Around line 587-595: The invite status assertion is using the wrong string
case; update the check in the test around response.invite so it matches the
API’s lowercase status value. Use the existing invite/status assertions in this
test to locate the spot and change invite.status to expect pending_invite
instead of PENDING_INVITE.
🪄 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: 1097584f-3044-4798-b529-0d66dd0c9c8e
📒 Files selected for processing (1)
tests/test_users.py
| self.assertEqual(status.code().name, "OK") | ||
| invite = response.invite | ||
| self.assertIsNotNone(invite) | ||
| self.assertEqual(invite.user_id, self.user_id) | ||
| self.assertEqual(invite.organization_id, self.org_id) | ||
| self.assertEqual(invite.status, "PENDING_INVITE") | ||
| self.assertIsNotNone(invite.created_at) | ||
| self.assertIsNotNone(invite.expires_at) | ||
| self.assertEqual(invite.resent_count, 1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
scalekit proto ResendInviteResponse invite status field type enum or string
💡 Result:
In the Scalekit API and Protobuf definitions, the status field within the invitation object (returned in ResendInviteResponse) is a string [1][2]. Official Scalekit documentation and API references describe the invite status as a string type [1][2]. Examples provided in the API docs demonstrate this field returning string values such as "pending_invite" [1][2]. While Scalekit uses Protobuf for its underlying gRPC services [3][4], the publicly documented representations in their API and SDK references consistently treat this status as a string [1][2].
Citations:
- 1: https://docs.scalekit.com/apis/?product=agentkit
- 2: https://docs.scalekit.com/apis.md
- 3: https://github.com/scalekit-inc/scalekit-sdk-node/blob/main/src/user.ts
- 4: https://pkg.go.dev/github.com/scalekit-inc/scalekit-sdk-go/v2/pkg/grpc/scalekit/v1/commons
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the referenced test and any invite status type definitions/usages.
git ls-files | rg '(^|/)(tests/test_users\.py|.*invite.*|.*user.*|.*proto.*|.*grpc.*)$'
printf '\n--- searches for status literals/usages ---\n'
rg -n --hidden --no-messages 'PENDING_INVITE|pending_invite|invite\.status|status.*invite' .
printf '\n--- candidate type definitions ---\n'
rg -n --hidden --no-messages 'ResendInviteResponse|Invite|invite status|status:' .Repository: scalekit-inc/scalekit-sdk-python
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the test file exists, inspect the relevant slice with line numbers.
if git ls-files --error-unmatch tests/test_users.py >/dev/null 2>&1; then
sed -n '560,610p' tests/test_users.py | cat -n
fiRepository: scalekit-inc/scalekit-sdk-python
Length of output: 2467
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test and any generated or documented invite status values in-repo.
if git ls-files --error-unmatch tests/test_users.py >/dev/null 2>&1; then
sed -n '580,600p' tests/test_users.py | cat -n
fi
printf '\n--- status literals in repository ---\n'
rg -n --hidden --no-messages '"PENDING_INVITE"|"pending_invite"|PENDING_INVITE|pending_invite' .Repository: scalekit-inc/scalekit-sdk-python
Length of output: 50388
Use the API’s lowercase invite status value here. invite.status is a string, but the response schema/docs use pending_invite, not PENDING_INVITE.
🤖 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 `@tests/test_users.py` around lines 587 - 595, The invite status assertion is
using the wrong string case; update the check in the test around response.invite
so it matches the API’s lowercase status value. Use the existing invite/status
assertions in this test to locate the spot and change invite.status to expect
pending_invite instead of PENDING_INVITE.
2 participants
Summary
Three test methods in
tests/test_users.pywere named for membership*_by_external_idoperations but were actually testing unrelated methods:test_create_membership_by_external_id— calledget_user_by_external_idinstead ofcreate_membership_by_external_idtest_update_membership_by_external_id— calledupdate_user_by_external_idinstead ofupdate_membership_by_external_idtest_delete_membership_by_external_id— calleddelete_user_by_external_idinstead ofdelete_membership_by_external_idThis meant
create_membership_by_external_id,update_membership_by_external_id, anddelete_membership_by_external_idinscalekit/users.pyhad zero test coverage despite having dedicated test methods.Changes
Each broken test is rewritten to actually exercise its target SDK method:
test_create_membership_by_external_id: Creates a user withexternal_idin org1, then callscreate_membership_by_external_id(org2_id, external_id, membership)to add them to a second org, asserts the membership response carries the correctexternal_id.test_update_membership_by_external_id: Creates a user withexternal_id, then callsupdate_membership_by_external_id(org_id, external_id, UpdateMembership(roles=[admin])), asserts the response reflects the updated membership.test_delete_membership_by_external_id: Creates a user withexternal_idin org1, adds them to org2 viacreate_membership_by_external_id, then callsdelete_membership_by_external_id(org2_id, external_id), asserts the call succeeds and verifies the user is no longer listed in org2.All three tests follow the same two-organization pattern used by the existing
test_create_membershiptest.Test plan
pytest tests/test_users.py::TestUsers::test_create_membership_by_external_id -s— should callcreate_membership_by_external_idand passpytest tests/test_users.py::TestUsers::test_update_membership_by_external_id -s— should callupdate_membership_by_external_idand passpytest tests/test_users.py::TestUsers::test_delete_membership_by_external_id -s— should calldelete_membership_by_external_idand passpytest tests/test_users.py -s— all tests should passSummary by CodeRabbit