Create extra integration tests for Resource lock #870
Conversation
psnoch-akamai
requested review from
ezilber-akamai,
vshanthe and
zliang-akamai
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR adds two integration tests for the Resource lock functionality to verify error handling scenarios: attempting to create conflicting lock types on the same resource, and attempting to create a lock with an invalid entity ID.
Key Changes
- Added
TestTryToLockTwoResourcesWithTheSameTypeto verify that creating a second lock with a different lock type on the same resource produces an error - Added
TestTryToCreateWithInvalidDatato verify that attempting to create a lock with a non-existent entity ID returns the expected error message - Imported
testify/assertandtestify/requirepackages for improved test assertions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/integration/locks_test.go | Added two new test functions with testify assertions to validate lock creation error scenarios |
| test/integration/fixtures/TestLockTryToLockResourceTwice.yaml | Added fixture data for testing conflicting lock types on the same resource |
| test/integration/fixtures/TestLockTryToCreateWithNotExistingEntityID.yaml | Added fixture data for testing lock creation with an invalid entity ID |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/integration/locks_test.go
Outdated
|
|
||
| func TestTryToLockTwoResourcesWithTheSameType(t *testing.T) { | ||
| client, instance, _, teardown, err := setupInstanceWithoutDisks(t, | ||
| "fixtures/TestLockTryToLockResourceTwice", false) |
There was a problem hiding this comment.
The fixture file name 'TestLockTryToLockResourceTwice' doesn't match the test function name 'TestTryToLockTwoResourcesWithTheSameType'. For consistency and maintainability, the fixture filename should match the test name.
| "fixtures/TestLockTryToLockResourceTwice", false) | |
| "fixtures/TestTryToLockTwoResourcesWithTheSameType", false) |
test/integration/locks_test.go
Outdated
| _, errAlreadyExist := client.CreateLock(context.Background(), createOpts) | ||
| require.Error(t, errAlreadyExist) |
There was a problem hiding this comment.
The variable name 'errAlreadyExist' is misleading. Based on the fixture response (line 574-576), the error is about conflicting lock types, not about a lock already existing. Consider renaming to 'errConflictingLock' or 'createSecondLockErr' for clarity.
| _, errAlreadyExist := client.CreateLock(context.Background(), createOpts) | |
| require.Error(t, errAlreadyExist) | |
| _, errConflictingLock := client.CreateLock(context.Background(), createOpts) | |
| require.Error(t, errConflictingLock) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/integration/locks_test.go
Outdated
| } | ||
|
|
||
| func TestTryToCreateWithInvalidData(t *testing.T) { | ||
| client, teardown := createTestClient(t, "fixtures/TestLockTryToCreateWithNotExistingEntityID") |
There was a problem hiding this comment.
The fixture filename 'TestLockTryToCreateWithNotExistingEntityID' is inconsistent with the test function name 'TestTryToCreateWithInvalidData'. The fixture should be named 'TestTryToCreateWithInvalidData.yaml' to maintain consistency between test names and their corresponding fixtures.
| client, teardown := createTestClient(t, "fixtures/TestLockTryToCreateWithNotExistingEntityID") | |
| client, teardown := createTestClient(t, "fixtures/TestTryToCreateWithInvalidData") |
psnoch-akamai
force-pushed
the
TPT-4097-resource-lock-extra-integration-tests
branch
from
ed420ff to
2a6b8eb
Compare
|
|
||
| createdLock, err := client.CreateLock(context.Background(), createOpts) | ||
| require.NoError(t, err) | ||
| defer client.DeleteLock(context.Background(), createdLock.ID) |
There was a problem hiding this comment.
| defer client.DeleteLock(context.Background(), createdLock.ID) | |
| t.Cleanup(func() { | |
| client.DeleteLock(context.Background(), createdLock.ID) | |
| }) |
Let's use t.Cleanup everywhere to ensure consistency and the order of executions of different deleting operations.
|
|
||
| func TestTryToCreateWithInvalidData(t *testing.T) { | ||
| client, teardown := createTestClient(t, "fixtures/TestTryToCreateWithInvalidData") | ||
| defer teardown() |
There was a problem hiding this comment.
| defer teardown() | |
| t.Cleanup(teardown) |
| LockType: linodego.LockTypeCannotDeleteWithSubresources, | ||
| } | ||
|
|
||
| _, createLockErr := client.CreateLock(context.Background(), createOpts) |
There was a problem hiding this comment.
Maybe just use err variable naming for createLockErr?
✔️ How to Test