Skip to content

Fix wrong assertion of the test: test_buckets_list_ctime#437

Closed
sumedhak27 wants to merge 1 commit intoceph:masterfrom
sumedhak27:FixListBucketsCtimeTest
Closed

Fix wrong assertion of the test: test_buckets_list_ctime#437
sumedhak27 wants to merge 1 commit intoceph:masterfrom
sumedhak27:FixListBucketsCtimeTest

Conversation

@sumedhak27
Copy link
Copy Markdown
Contributor

TestName:
s3tests_boto3.functional.test_s3:test_buckets_list_ctime

Problem:
The test creates 5 buckets for a user but in an assertion check,
it asserts false if any bucket of the user has CreationTime less than a day prior to the current time.
Due to this reason, the test fails if the user has pre-existing buckets older than a day.

Solution:
Assert only on the CreationTime of buckets that were created with test execution.

TestName:
s3tests_boto3.functional.test_s3:test_buckets_list_ctime

Problem:
The test creates 5 buckets for a user but in an assertion check,
it asserts false if any bucket of the user has CreationTime less
than a day prior to current time.
Due to this reason the test fails if the user has pre-existing
buckets older than a day.

Solution:
Assert only on the CreationTime of buckets that were created with
test execution.

Signed-off-by: Sumedh A. Kulkarni <sumedh.a.kulkarni@seagate.com>
@jheunis-bloomberg
Copy link
Copy Markdown

Is there something specific that is blocking this PR being merged? Or did it just slip through the cracks?
We've been running into this issue ourselves, would be handy if we could use this fix.

@cbodley
Copy link
Copy Markdown
Contributor

cbodley commented Feb 29, 2024

Is there something specific that is blocking this PR being merged? Or did it just slip through the cracks? We've been running into this issue ourselves, would be handy if we could use this fix.

thanks for the reminder, @jheunis-bloomberg. @alimaredia would you be willing to test this?

@bobham-bloomberg
Copy link
Copy Markdown

bobham-bloomberg commented Mar 1, 2024

FYI, I cherry-picked this commit and tested it on our setup where the test is ordinarily failing and it fixes the test:

2024-03-01 12:29:22,899 - DEBUG - 146 - Test OK after 0.73s: test_buckets_list_ctime

@BBoozmen
Copy link
Copy Markdown
Contributor

BBoozmen commented Mar 1, 2024

FYI, I cherry-picked this commit and tested it on our setup where the test is ordinarily failing and it fixes the test:

2024-03-01 12:29:22,899 - DEBUG - 146 - Test OK after 0.73s: test_buckets_list_ctime

@cbodley - Bob has verified the fix. Can we merge this one in?

buckets = []
for i in range(5):
client.create_bucket(Bucket=get_new_bucket_name())
buckets.append(client.create_bucket(Bucket=get_new_bucket_name()))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this creates an array of create_bucket() response objects, not of bucket names. so the loop below filters out all bucket names and doesn't check any ctimes

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I've fixed this and created a new pull request, #565, as it looks like I don't have rights to update this pull request.

@cbodley
Copy link
Copy Markdown
Contributor

cbodley commented May 14, 2024

merged in #565, thanks @sumedhak27 @bobham-bloomberg

@cbodley cbodley closed this May 14, 2024
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.

7 participants