Skip to content

Conversation

@kans
Copy link
Contributor

@kans kans commented Dec 3, 2025

Summary by CodeRabbit

  • Tests

    • Modified a unit test expectation (may change test outcome) and added a small test-local change; this affects the test suite results.
  • Chores

    • No production code or user-facing behavior changed; only the test suite was updated.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 3, 2025

Warning

Rate limit exceeded

@kans has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 53 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 7b5a037 and 9e36515.

⛔ Files ignored due to path filters (1)
  • .github/workflows/ci.yaml is excluded by none and included by none
📒 Files selected for processing (1)
  • pkg/client/service_account_test.go (1 hunks)

Walkthrough

Updated a unit test in pkg/client/service_account_test.go: the test case "almost service account but different domain" now expects true instead of false, and an unused local variable a := 2 was added before the test loop. No production code or exported declarations changed.

Changes

Cohort / File(s) Summary
Test file update
pkg/client/service_account_test.go
Modified TestIsServiceAccount expectations (changed one case from false to true) and introduced an unused local variable a := 2 before the test loop.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Inspect the adjusted test case expectation to confirm it matches intended behavior.
  • Verify the added a := 2 is either intentional (e.g., for a future change) or should be removed as dead code.

Poem

🐰 I nudged a test with a curious paw,
Switched a truth and left a tiny flaw.
A stray little var — a hop, not a race,
Quiet mischief in a cozy test space. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'testing remote workflow' is vague and does not clearly relate to the actual changes in the changeset, which involve modifications to a service account test case (changing an expectation from false to true) and adding an unused variable. Provide a more specific title that describes the actual changes, such as 'Fix service account domain validation test' or 'Update TestIsServiceAccount expectations'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

@kans kans force-pushed the kans/lint-workflow branch 4 times, most recently from 8cee67d to 2efab9d Compare December 3, 2025 23:05
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9875c6 and 2efab9d.

⛔ Files ignored due to path filters (1)
  • .github/workflows/ci.yaml is excluded by none and included by none
📒 Files selected for processing (1)
  • pkg/client/service_account_test.go (1 hunks)

@kans kans force-pushed the kans/lint-workflow branch 13 times, most recently from ab9e643 to 7b5a037 Compare December 4, 2025 17:58
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a070135 and 7b5a037.

⛔ Files ignored due to path filters (1)
  • .github/workflows/ci.yaml is excluded by none and included by none
📒 Files selected for processing (1)
  • pkg/client/service_account_test.go (1 hunks)

Comment on lines 44 to 47
name: "almost service account but different domain",
email: "test@serviceaccount.atlassian.org",
expected: false,
expected: true,
},
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the isServiceAccount function implementation
ast-grep --pattern $'func isServiceAccount($$$) $_ {
  $$$
}'

Repository: ConductorOne/baton-jira

Length of output: 249


Fix incorrect test expectation: email with .org domain should return false.

The isServiceAccount() implementation (pkg/client/client.go:37) checks: strings.HasSuffix(email, "@serviceaccount.atlassian.com"). This only accepts emails with the .com domain.

The test email test@serviceaccount.atlassian.org uses .org, so it will not match and isServiceAccount() returns false. The test expectation must be changed back to false, not true. The test case name "almost service account but different domain" correctly reflects this intent.

🤖 Prompt for AI Agents
In pkg/client/service_account_test.go around lines 44 to 47, the test case
"almost service account but different domain" incorrectly expects true for email
"test@serviceaccount.atlassian.org"; update the expected value to false so the
test aligns with isServiceAccount() which only matches
"@serviceaccount.atlassian.com". Ensure the test assertion uses the corrected
expected boolean.

},
}

a := 2
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove the unused variable.

The local variable a is declared but never used. This is dead code that should be removed.

Apply this diff to remove the unused variable:

-	a := 2
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
a := 2
🤖 Prompt for AI Agents
In pkg/client/service_account_test.go around line 49, there is an unused local
variable declaration "a := 2" that should be removed; delete that line so the
test file no longer contains the dead variable and re-run tests to ensure no
other references to "a" exist.

@kans kans force-pushed the kans/lint-workflow branch from 7b5a037 to 9e36515 Compare December 4, 2025 18:03
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