Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect if-condition in test_create_with_invalid_url #1495

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

brandboat
Copy link
Contributor

@brandboat brandboat commented Sep 2, 2024

Which issue(s) this PR fixes:

What this PR does / why we need it:

before harvester/harvester@13bb968, if a vm image failed to recognize the image url, we didn't have any condition in status until retry 3 times, while after the commit, we will have RetryLimitExceeded condition in status upon retry failure in the first round retry. Now the test didn't wait until the right condition and do assertion thus cause test failed.

This pr mainly move the assertion part into the while loop, make sure the assertion is correct until timeout.

Special notes for your reviewer:

N/A

Additional documentation or context

N/A

@lanfon72
Copy link
Member

lanfon72 commented Sep 2, 2024

Hi @brandboat, It looks the change is not related to #1173 which is about checksum.

and as you mentioned, the new condition RetryLimitExceeded will be added to fix the bug in #1173, I want to know whether the initialized would still be the last one of conditions or not?

@brandboat
Copy link
Contributor Author

Hi @brandboat, It looks the change is not related to #1173 which related to checksum.

You are right, should I create another issue ?

and as you mentioned, the new condition RetryLimitExceeded will be added to fix the bug in #1173, I want to know whether the initialized would still be the last one of conditions or not?

It depends, but for the invalid_url test case, the Initialized condition is still the last one in the array. For me it's a bit unusual to guarantee the condition's position, so I adjusted the test to locate the Initialized condition rather than assuming it’s always the last element.

@lanfon72 lanfon72 requested a review from a team September 2, 2024 09:31
@lanfon72
Copy link
Member

lanfon72 commented Sep 2, 2024

Hi @brandboat, It looks the change is not related to #1173 which related to checksum.

You are right, should I create another issue ?

No, but please don't refer this to #1173.

and as you mentioned, the new condition RetryLimitExceeded will be added to fix the bug in #1173, I want to know whether the initialized would still be the last one of conditions or not?

It depends, but for the invalid_url test case, the Initialized condition is still the last one in the array. For me it's a bit unusual to guarantee the condition's position, so I adjusted the test to locate the Initialized condition rather than assuming it’s always the last element.

if so, we can simply change the:

if len(image_conds) > 0:

into

if "Initialized" == image_conds[-1].get("type"):

to fix the bug for new change.

As the test case scenario is focusing on invalid_url, we would not check the while loop is timed out or not.
(actually we will never hit timed out in the case)

@brandboat brandboat changed the title Fix incorrect assertion in test_create_with_invalid_url Fix incorrect if-condition in test_create_with_invalid_url Sep 2, 2024
Copy link
Member

@lanfon72 lanfon72 left a comment

Choose a reason for hiding this comment

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

LGTM

@brandboat brandboat merged commit 1605bb2 into harvester:main Sep 2, 2024
4 checks passed
@brandboat brandboat deleted the tests-1173 branch September 6, 2024 08:33
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.

3 participants