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

Availability: Skip bad configurations #3502

Merged
merged 3 commits into from
Nov 14, 2024
Merged

Conversation

kamalca
Copy link
Collaborator

@kamalca kamalca commented Nov 9, 2024

Mismatches in Availability Set requirements and capabilities may be due to test case requirements, so we can skip the test instead of failing.

@kamalca
Copy link
Collaborator Author

kamalca commented Nov 11, 2024

@squirrelsc Any suggestions on how to resolve 'Availability.on_before_deployment' is too complex flake error?

I considered making a common function, called something like assert_or_skip(condition: Any, message: str) that would work like an assert statement but raise a SkippedException instead of an assert error. This would remove a level of logic branching and resolve the error.

What are your thoughts? There are complex interactions between ultradisk, availability types, and maximize capability, so it is not easy to break this logic into small pieces.

@kamalca kamalca force-pushed the kameroncarr/availability branch 2 times, most recently from eb9e778 to 86eca00 Compare November 11, 2024 19:37
@squirrelsc
Copy link
Member

@squirrelsc Any suggestions on how to resolve 'Availability.on_before_deployment' is too complex flake error?

I considered making a common function, called something like assert_or_skip(condition: Any, message: str) that would work like an assert statement but raise a SkippedException instead of an assert error. This would remove a level of logic branching and resolve the error.

What are your thoughts? There are complex interactions between ultradisk, availability types, and maximize capability, so it is not easy to break this logic into small pieces.

I thought I replied, but lost. Instead of the assert_or_skip, I suggest splitting a meaningful sub-method for this method. The sub-method may be called like _check_configuration_conflict, and move all related content here. The assert_and_skip is easy to misuse, because most skip should happen in test case level. Also, there are many assert methods, like is_true, is_empty, and so on. It's not worth to wrap all of them.

Mismatches in Availability Set requirements and capabilities may be due to test case requirements, so we can skip the test instead of failing.
Break up on_before_deployment to reduce the complexity of the function.
@squirrelsc
Copy link
Member

@LiliDeng LGTM

Make it more clear that supported availability types can also be affected by test case requirements.
@LiliDeng LiliDeng merged commit 4ec8c8d into main Nov 14, 2024
45 checks passed
@LiliDeng LiliDeng deleted the kameroncarr/availability branch November 14, 2024 01:51
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.

4 participants