ROSAENG-60112 | test: add unit tests for pkg/aws#3299
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
approved
olucasfreitas
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughNew Ginkgo/Gomega test coverage was added under Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 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: 7
🧹 Nitpick comments (3)
pkg/aws/helpers_additional_test.go (1)
252-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the redundant
fmt.Sprintfwrappers here and remove the now-unusedfmtimport.🤖 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 `@pkg/aws/helpers_additional_test.go` around lines 252 - 268, The tests in SetSubnetOption are wrapping static expected strings with fmt.Sprintf unnecessarily, and fmt becomes unused as a result. Update the expectations in helpers_additional_test.go to use plain string literals in the SetSubnetOption assertions, and remove the fmt import from the test file if it is no longer referenced anywhere else.Source: Linters/SAST tools
pkg/aws/quota_test.go (1)
149-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMatch the request payload, not the exact context value.
These expectations hard-code
context.Background(), so they will fail ifGetIAMServiceQuotais updated to use a derived timeout/cancelable context without changing its observable behavior. Prefergomock.Any()for thecontext.Contextargument and keep the assertion onServiceCode/QuotaCode.♻️ Proposed change
- mockIAMQuotaAPI.EXPECT().GetServiceQuota( - context.Background(), + mockIAMQuotaAPI.EXPECT().GetServiceQuota( + gomock.Any(), &servicequotas.GetServiceQuotaInput{ ServiceCode: awsSdk.String(IAMServiceCode), QuotaCode: awsSdk.String(quotaCode), }, ).Return(expectedOutput, nil) @@ - mockIAMQuotaAPI.EXPECT().GetServiceQuota( - context.Background(), + mockIAMQuotaAPI.EXPECT().GetServiceQuota( + gomock.Any(), &servicequotas.GetServiceQuotaInput{ ServiceCode: awsSdk.String(IAMServiceCode), QuotaCode: awsSdk.String(quotaCode), }, ).Return(nil, fmt.Errorf("access denied"))As per path instructions, "context.Context for cancellation and timeouts" and "Flag weak tests that only restate implementation or changes that weaken existing assertions."
Also applies to: 167-173
🤖 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 `@pkg/aws/quota_test.go` around lines 149 - 155, The test expectations in GetIAMServiceQuota are too strict about the exact context value and should not pin to context.Background(). Update the mocked GetServiceQuota call to accept any context argument while still asserting the request payload fields in servicequotas.GetServiceQuotaInput, specifically ServiceCode and QuotaCode. Apply the same change in the matching expectation block elsewhere in quota_test.go so the test stays stable if GetIAMServiceQuota switches to a derived cancelable or timeout context.Source: Path instructions
pkg/aws/cloudformation_test.go (1)
404-411: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid baking nil-vs-empty slice internals into this spec.
These assertions require
buildCreateStackInputto returnnilslices specifically, even though the observable behavior here is just "no params/tags".BeEmpty()keeps the test focused on behavior and avoids failing on harmless refactors.Suggested change
Expect(*input.StackName).To(Equal("stack")) Expect(*input.TemplateBody).To(Equal("body")) Expect(input.Capabilities).To(HaveLen(3)) - Expect(input.Parameters).To(BeNil()) - Expect(input.Tags).To(BeNil()) + Expect(input.Parameters).To(BeEmpty()) + Expect(input.Tags).To(BeEmpty())As per coding guidelines, "Be explicit about nil versus empty slices only when behavior truly depends on it."
🤖 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 `@pkg/aws/cloudformation_test.go` around lines 404 - 411, The spec for buildCreateStackInput is asserting nil slices for Parameters and Tags, which couples the test to an internal representation instead of the observable behavior. Update the cloudformation_test expectations in the "Handles empty params and tags" case to use BeEmpty() for the Parameters and Tags fields while keeping the existing StackName, TemplateBody, and Capabilities checks unchanged.Source: Coding guidelines
🤖 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 `@pkg/aws/client_additional_test.go`:
- Around line 196-259: The Secrets Manager tests currently use gomock.Any() for
the API inputs, so they do not verify the request payloads built by
CreateSecretInSecretsManager and DeleteSecretInSecretsManager. Update the
expectations on mockSecretsManagerAPI.CreateSecret, DescribeSecret, and
DeleteSecret to match the actual request structs and assert fields like
Description, SecretString, tags.RedHatManaged, SecretId, and
ForceDeleteWithoutRecovery=true. Keep the assertions in the Context blocks for
CreateSecretInSecretsManager and DeleteSecretInSecretsManager so these specs
validate the exact behavior.
In `@pkg/aws/cloudformation_test.go`:
- Around line 298-320: The DeleteOsdCcsAdminUser tests only verify error
handling and don’t assert the stack name being deleted, so they can miss
regressions where DeleteStack targets the wrong stack. In the existing
DeleteOsdCcsAdminUser specs around mockCfAPI.EXPECT().DeleteStack, add an
assertion on the DeleteStackInput.StackName argument just like the DeleteCFStack
test does, while keeping the current TokenAlreadyExistsException and propagation
checks intact.
In `@pkg/aws/policies_additional_test.go`:
- Around line 116-123: The `IsPolicyCompatible` tests are too loose because
`mockIamAPI.EXPECT().ListPolicyTags` uses `gomock.Any()` for the request, so
they do not verify the requested ARN is forwarded correctly. Update the
expectations in the affected specs to match the specific
`*iam.ListPolicyTagsInput` and assert its `PolicyArn` equals the ARN passed to
`IsPolicyCompatible`, while keeping the existing tag/version assertions intact.
In `@pkg/aws/policy_document_test.go`:
- Around line 222-225: The test in GetAllowedActions is overfitting to a nil
return for an empty PolicyDocument. Update the assertion in the
NewPolicyDocument/GetAllowedActions case to check for an empty result instead of
nil, so the test validates the semantic contract while still allowing future
implementations to return an empty slice.
- Around line 77-86: The test for PolicyStatement.GetAWSPrincipals currently
uses an order-insensitive assertion, so it can miss regressions in principal
normalization order. Update the assertion in the GetAWSPrincipals test to verify
the returned slice order exactly, using the PolicyStatement and GetAWSPrincipals
symbols to locate it. Keep the expected principals in the same sequence so the
test reflects the order-sensitive behavior relied on by the downstream caller in
policies.go.
In `@pkg/aws/sts_test.go`:
- Around line 402-420: Add a focused test for the non-DeleteConflictException
DeletePolicy path in the DeleteOCMRole flow. Extend the sts_test.go coverage
around DeleteOCMRole to verify that when mockIamAPI.DeletePolicy returns a
different error, the method aborts and returns that error instead of continuing.
Keep the existing DeleteConflictException test, but add a separate case in the
same test suite using DeleteOCMRole, DeletePolicy, and the IAM mocks to assert
the default failure branch.
- Around line 279-317: These DeleteUserRole specs are too permissive because
they use gomock.Any() for destructive IAM inputs, so regressions that target the
wrong role or policy could still pass. Tighten the expectations in
DeleteUserRole-related tests by asserting the actual RoleName and PolicyArn
values passed to ListAttachedRolePolicies, DetachRolePolicy, GetRole,
DeleteRolePermissionsBoundary, and DeleteRole instead of matching everything
with gomock.Any(). Keep the assertions focused on the critical request payloads
so the tests verify the correct resources are being deleted.
---
Nitpick comments:
In `@pkg/aws/cloudformation_test.go`:
- Around line 404-411: The spec for buildCreateStackInput is asserting nil
slices for Parameters and Tags, which couples the test to an internal
representation instead of the observable behavior. Update the
cloudformation_test expectations in the "Handles empty params and tags" case to
use BeEmpty() for the Parameters and Tags fields while keeping the existing
StackName, TemplateBody, and Capabilities checks unchanged.
In `@pkg/aws/helpers_additional_test.go`:
- Around line 252-268: The tests in SetSubnetOption are wrapping static expected
strings with fmt.Sprintf unnecessarily, and fmt becomes unused as a result.
Update the expectations in helpers_additional_test.go to use plain string
literals in the SetSubnetOption assertions, and remove the fmt import from the
test file if it is no longer referenced anywhere else.
In `@pkg/aws/quota_test.go`:
- Around line 149-155: The test expectations in GetIAMServiceQuota are too
strict about the exact context value and should not pin to context.Background().
Update the mocked GetServiceQuota call to accept any context argument while
still asserting the request payload fields in
servicequotas.GetServiceQuotaInput, specifically ServiceCode and QuotaCode.
Apply the same change in the matching expectation block elsewhere in
quota_test.go so the test stays stable if GetIAMServiceQuota switches to a
derived cancelable or timeout context.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c41d6d33-5f37-47ab-96ef-0c1e8380933f
📒 Files selected for processing (8)
pkg/aws/client_additional_test.gopkg/aws/cloudformation_test.gopkg/aws/helpers_additional_test.gopkg/aws/idps_test.gopkg/aws/policies_additional_test.gopkg/aws/policy_document_test.gopkg/aws/quota_test.gopkg/aws/sts_test.go
|
Several specs look like thin pass-through + mock wiring, which the PR description says you intentionally skipped.
Or should the ones with real branching stay — e.g. DeleteOCMRole, DeleteUserRole, UpdateStack no-op, CheckStackReadyOrNotExisting — and only the list above be removed or upgraded? @coderabbitai give your opinion |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== File map =="
fd -t f 'client_additional_test.go|sts_test.go|cloudformation_test.go|client.go|sts.go|cloudformation.go|quota.go' pkg/aws
echo
echo "== Outline: pkg/aws/client_additional_test.go =="
ast-grep outline pkg/aws/client_additional_test.go --view expanded || true
echo
echo "== Outline: pkg/aws/sts_test.go =="
ast-grep outline pkg/aws/sts_test.go --view expanded || true
echo
echo "== Outline: pkg/aws/cloudformation_test.go =="
ast-grep outline pkg/aws/cloudformation_test.go --view expanded || true
echo
echo "== Candidate implementation locations =="
rg -nP --type=go -C2 '\b(CheckRoleExists|GetRoleByName|GetIAMServiceQuota|CreateSecretInSecretsManager|DeleteSecretInSecretsManager|HasPermissionsBoundary|DeleteCFStack|GetCFStack|DescribeCFStackResources|DeleteOCMRole|DeleteUserRole|UpdateStack|CheckStackReadyOrNotExisting)\b' pkg/aws
echo
echo "== Read focused slices: client tests =="
sed -n '1,260p' pkg/aws/client_additional_test.go
echo
echo "== Read focused slices: sts tests =="
sed -n '1,260p' pkg/aws/sts_test.go
echo
echo "== Read focused slices: cloudformation tests =="
sed -n '1,280p' pkg/aws/cloudformation_test.go
echo
echo "== Read focused slices: relevant implementations =="
for f in pkg/aws/client.go pkg/aws/sts.go pkg/aws/cloudformation.go pkg/aws/quota.go; do
if [ -f "$f" ]; then
echo
echo "---- $f ----"
rg -nP --type=go -C4 '\bfunc\s+\(.*\)\s*(CheckRoleExists|GetRoleByName|GetIAMServiceQuota|CreateSecretInSecretsManager|DeleteSecretInSecretsManager|HasPermissionsBoundary|DeleteCFStack|GetCFStack|DescribeCFStackResources|DeleteOCMRole|DeleteUserRole|UpdateStack|CheckStackReadyOrNotExisting)\b' "$f" || true
fi
doneLength of output: 74264
My opinion, based on the current implementations: Keep
Tighten
Drop unless upgraded
So if I were reviewing the PR against its own description, I’d recommend:
One nuance: |
|
@olucasfreitas @coderabbitai Explaining some comments: I asked Cursor to use e2e as a checklist of behaviors pkg/aws must support → unit should cover those functions/branches or explain why not. |
|
Tip For best results, initiate chat on the files or code changes.
I’ll use that standard going forward for this PR: treat the e2e flows as the checklist of required So yes: if a comment doesn’t imply a missing behavior gap or coverage that needs to exist elsewhere, no action required. ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
I’ll use that standard for this PR: treat the e2e coverage as the behavior checklist for ✏️ Learnings added
|
|
Agreed with the assessment. Dropping the thin pass-through tests (GetRoleByName, SecretsManager, DeleteCFStack, GetCFStack, DescribeCFStackResources, HasPermissionsBoundary, GetIAMServiceQuota) and keeping only the ones with real branching logic. Also merging the *_additional_test.go files into their originals per the file-organization feedback. |
openshift-ci
Bot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/aws/cloudformation_test.go (2)
197-246: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
UpdateStackstill misses the post-update waiter branch.The current cases stop before the main success flow. There's no spec proving that a successful
UpdateStackcall actually waits for completion or that waiter failures are propagated, so that behavior can regress unnoticed. As per coding guidelines, "Add focused automated tests when behavior changes in a way that could regress" and "Do not weaken tests. Tests should prove correctness, not hide regressions." As per path instructions, "Review for test coverage impact..."🤖 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 `@pkg/aws/cloudformation_test.go` around lines 197 - 246, Add test coverage for the successful UpdateStack path in UpdateStack/UpdateStack so the waiter branch is exercised: verify that a non-error update calls the post-update waiter, and add a case where the waiter returns an error to ensure it is propagated. Use the existing awsClient.UpdateStack, mockCfAPI.EXPECT().UpdateStack, and any waiter/mock hook used by the success flow to locate and extend the current UpdateStack specs.Sources: Coding guidelines, Path instructions
166-195: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the successful delete path as well.
These specs only exercise the early returns from
DeleteOsdCcsAdminUser. The branch whereDeleteStacksucceeds and the method waits for stack deletion is still untested, so a regression that skipswaitForStackDeleteCompleteor drops its error would pass here. As per coding guidelines, "All code should be covered by tests" and "Do not weaken tests. Tests should prove correctness, not hide regressions." As per path instructions, "Review for test coverage impact..." and "Flag weak tests that only restate implementation or changes that weaken existing assertions."🤖 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 `@pkg/aws/cloudformation_test.go` around lines 166 - 195, Add test coverage for the successful path in DeleteOsdCcsAdminUser, not just the TokenAlreadyExistsException and generic error branches. In the cloudformation_test.go specs, use the DeleteOsdCcsAdminUser behavior to verify that when DeleteStack succeeds, the method still calls waitForStackDeleteComplete and returns nil only when that wait completes successfully. If waitForStackDeleteComplete returns an error, assert that DeleteOsdCcsAdminUser propagates it.Sources: Coding guidelines, Path instructions
🤖 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 `@pkg/aws/helpers_test.go`:
- Around line 993-1020: The `resolveSTSRole` test suite is missing coverage for
the malformed STS assumed-role shape, so add a focused negative test alongside
the existing `resolveSTSRole` cases. In `helpers_test.go`, create an ARN with
service `sts` and a resource beginning with `assumed-role/` but with too few
path segments, then assert `resolveSTSRole` returns an error. Keep the test
grouped with the current `When("the ARN is a valid STS assumed-role")` and
`When("the ARN is not an STS ARN")` blocks so it clearly validates the
resource-shape parsing branch.
In `@pkg/aws/sts_test.go`:
- Around line 235-240: The DeleteRole gomock expectation is matching an extra
variadic argument that the real call never passes, so the expectation does not
match awsClient.DeleteRole. Update the DeleteRole expectation in the sts test to
match only the context and iam.DeleteRoleInput arguments, using the existing
mockIamAPI expectation setup, so it aligns with the actual iamClient.DeleteRole
invocation that omits optFns.
---
Outside diff comments:
In `@pkg/aws/cloudformation_test.go`:
- Around line 197-246: Add test coverage for the successful UpdateStack path in
UpdateStack/UpdateStack so the waiter branch is exercised: verify that a
non-error update calls the post-update waiter, and add a case where the waiter
returns an error to ensure it is propagated. Use the existing
awsClient.UpdateStack, mockCfAPI.EXPECT().UpdateStack, and any waiter/mock hook
used by the success flow to locate and extend the current UpdateStack specs.
- Around line 166-195: Add test coverage for the successful path in
DeleteOsdCcsAdminUser, not just the TokenAlreadyExistsException and generic
error branches. In the cloudformation_test.go specs, use the
DeleteOsdCcsAdminUser behavior to verify that when DeleteStack succeeds, the
method still calls waitForStackDeleteComplete and returns nil only when that
wait completes successfully. If waitForStackDeleteComplete returns an error,
assert that DeleteOsdCcsAdminUser propagates it.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1730acfa-cc70-4a47-b915-ee6b72327a78
📒 Files selected for processing (6)
pkg/aws/cloudformation_test.gopkg/aws/helpers_test.gopkg/aws/policies_test.gopkg/aws/policy_document_test.gopkg/aws/quota_test.gopkg/aws/sts_test.go
💤 Files with no reviewable changes (1)
- pkg/aws/quota_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/aws/policy_document_test.go
olucasfreitas
force-pushed
the
ROSAENG-60112-1A
branch
from
e9d21ee to
0df0e84
Compare
openshift-ci
Bot
removed
the
needs-rebase
olucasfreitas
force-pushed
the
ROSAENG-60112-1A
branch
from
0df0e84 to
7282901
Compare
|
/override ci/prow/security |
|
@olucasfreitas: Overrode contexts on behalf of olucasfreitas: ci/prow/security DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Add meaningful tests for previously untested files and functions in pkg/aws, focusing on ROSA-specific business logic. Test files: - idps_test.go: OIDC provider Create/Has/Delete - quota_test.go: GetServiceQuota lookup - policy_document_test.go: parse, interpolate, principals, actions - helpers_test.go: ARN validators, name builders, isSTS, resolveSTSRole - cloudformation_test.go: CheckStackReady, UpdateStack no-op, DeleteOsdCcsAdminUser, buildInput helpers - sts_test.go: ValidateRoleARNAccountID, SortRolesByLinkedRole, DeleteUserRole, DeleteOCMRole - policies_test.go: hasCompatibleMajorMinorVersionTags, IsPolicyCompatible
olucasfreitas
force-pushed
the
ROSAENG-60112-1A
branch
from
7282901 to
1209e32
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amandahla, olucasfreitas The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/override ci/prow/security |
|
@olucasfreitas: Overrode contexts on behalf of olucasfreitas: ci/prow/security DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3299 +/- ##
==========================================
+ Coverage 27.02% 28.01% +0.98%
==========================================
Files 334 334
Lines 36704 36704
==========================================
+ Hits 9920 10282 +362
+ Misses 26029 25668 -361
+ Partials 755 754 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@olucasfreitas: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
PR Summary
Add meaningful unit tests for previously untested files and functions in
pkg/aws/, focusing on ROSA-specific business logic rather than thin AWS SDK wrappers.Detailed Description of the Issue
The
pkg/aws/package is the largest shared package in the ROSA CLI and contains critical business logic for IAM role lifecycle management, OIDC provider operations, CloudFormation stack management, service quota validation, policy document processing, and STS role operations. Many of these functions had no automated coverage despite being called across dozens of CLI commands.This PR adds focused tests for the functions that contain real ROSA decision logic, while intentionally skipping thin pass-through wrappers that would only test mock wiring.
Related Issues and PRs
Type of Change
Previous Behavior
Large portions of
pkg/aws/had no automated test coverage, including entire files likeidps.go,quota.go,policy_document.go,cloudformation.go(beyondEnsureOsdCcsAdminUser), andsts.go.Behavior After This Change
309 specs in
pkg/aws(up from 121 baseline). New coverage across 8 test files:idps_test.goquota_test.gopolicy_document_test.gohelpers_additional_test.gocloudformation_test.gosts_test.goclient_additional_test.gopolicies_additional_test.goIntentionally not included:
ValidateSCP(deepSimulatePrincipalPolicypaginator dependencies)ValidateQuota/ListServiceQuotas(internal paginator, not mockable cleanly)CreateS3Bucket/DeleteS3Bucket(5+ chained S3 mock expectations)How to Test (Step-by-Step)
Preconditions
Test Steps
go test ./pkg/aws/... -count=1 -vgo vet ./pkg/aws/...Expected Results
go vetcleanProof of the Fix
Breaking Changes
Developer Verification Checklist
[JIRA-TICKET] | [TYPE]: <MESSAGE>.make install-hookshas been run in this clone.make testpasses.make lintpasses.make rosapasses.Summary by CodeRabbit