-
Notifications
You must be signed in to change notification settings - Fork 81
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
test: Update getRegionsWithCaps function to also accept a list of plans that must be available in the identified region #555
test: Update getRegionsWithCaps function to also accept a list of plans that must be available in the identified region #555
Conversation
@@ -191,8 +204,29 @@ func getRegionsWithCaps(t *testing.T, client *linodego.Client, capabilities []st | |||
return true | |||
} | |||
|
|||
// Function to check if a region has the required plans available | |||
regionHasPlans := func(regionID string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of closures!
Returns: | ||
- string values representing the IDs of regions that meet the given criteria. | ||
*/ | ||
func getRegionsWithCaps(t *testing.T, client *linodego.Client, capabilities, plans []string) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: We can considerably reduce the number of iterations we need to run by aggregating all availabilities into a map hashing on (region, plan):
regionsAvailabilities, err := client.ListRegionsAvailability(context.Background(), nil)
require.NoError(t, err)
type availKey struct {
Region string
Plan string
}
availMap := make(map[availKey]linodego.RegionAvailability, len(regionsAvailabilities))
for _, avail := range regionsAvailabilities {
availMap[availKey{Region: avail.Region, Plan: avail.Plan}] = avail
}
// ...
regionHasPlans := func(regionID string) bool {
for _, plan := range plans {
if avail, ok := availMap[availKey{Region: regionID, Plan: plan}]; !ok || !avail.Available {
return false
}
}
return true
}
Since this is just for tests I don't think it's a big deal either way; just thought I'd share another approach 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great suggestion! I didn't think of putting it into a hash map but this will for sure be faster than my previous solution 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests works well locally, nice work!
@@ -191,8 +214,22 @@ func getRegionsWithCaps(t *testing.T, client *linodego.Client, capabilities []st | |||
return true | |||
} | |||
|
|||
// Function to check if a region has the required plans available | |||
regionHasPlans := func(regionID string) bool { | |||
if len(plans) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still left this check in to avoid any unnecessary map lookups when there are no plans to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great once fixtures are re-run, nice work!
📝 Description
A helper function has been updated for the logic to resolve integration test regions that also accepts a list of plans which must be available in the resolved region
Note: the function takes list value of plans as an optional parameter
✔️ How to Test
make testint
📷 Preview
If applicable, include a screenshot or code snippet of this change. Otherwise, please remove this section.