-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fixed the init_test by comparing #58
Conversation
Signed-off-by: 7h3-3mp7y-m4n <[email protected]>
after running the |
Thanks, @7h3-3mp7y-m4n! We have been seeing random issue in the CI with this unit tests, like here for example:https://github.com/score-spec/score-k8s/actions/runs/11355384862/job/31584604052:
When getting this error, just So if this is fixing this issue, this would be great! I would love your review on this, @delca85, any feedback/comment on this? |
main_init_test.go
Outdated
@@ -150,7 +151,17 @@ func TestInitWithProvisioners(t *testing.T) { | |||
provs, err := loader.LoadProvisionersFromDirectory(filepath.Join(td, ".score-k8s"), loader.DefaultSuffix) | |||
assert.NoError(t, err) | |||
if assert.Greater(t, len(provs), 2) { | |||
assert.Equal(t, "template://two", provs[0].Uri()) | |||
assert.Equal(t, "template://one", provs[1].Uri()) | |||
expectedProvisioners := map[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.
@7h3-3mp7y-m4n sorry for being annoying, what do you think about this way to check that the expected provisioners are there without checking the order but leveraging slices
functions:
expectedProvisionerUris := []string{"template://one", "template://two"}
for _, expectedUri := range expectedProvisionerUris {
assert.True(t, slices.ContainsFunc(provs, func(p provisioners.Provisioner) bool {
return p.Uri() == expectedUri
}), fmt.Sprintf("Expected provisioner '%s' not found", expectedUri))
}
?
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.
No no @delca85 you are not annoying, TBH I like your approach, I wrote it this way cause I was debugging at the same time. I added a map because of its fast lookup ... As we are dealing with a small number of inputs cause it's a test we can definitely use your approach, and it's more flexible and has fewer lines .. Thanks!
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.
Cool, thank you @7h3-3mp7y-m4n !
Signed-off-by: 7h3-3mp7y-m4n <[email protected]>
improved the test by adding the desired changes
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.
LGTM, thanks for the changes!
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.
Thanks @7h3-3mp7y-m4n, for fixing this random issue we got in CI/tests!
And thanks, @delca85, as always, for your prescriptive suggestions and reviews, much appreciated!
LGTM
Fixed the main_init_test.go
Description
What does this PR do?
It just fixed the uncertainty error we get while testing
Types of changes
Checklist: