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

Deflake TestVolumeCreateClusterOpts #3641

Merged

Conversation

photra
Copy link
Contributor

@photra photra commented May 31, 2022

- What I did
Fixes #3636

- How I did it
Added sorting logic to test to ensure consistent ordering of Secrets

- How to verify it
Run the full test suite multiple times to validate this fix

- Description for the changelog
Deflaked TestVolumeCreateClusterOpts

- A picture of a cute animal (not mandatory but encouraged)
image

@codecov-commenter
Copy link

codecov-commenter commented May 31, 2022

Codecov Report

Merging #3641 (215efa0) into master (84b86e2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3641      +/-   ##
==========================================
+ Coverage   59.01%   59.02%   +0.01%     
==========================================
  Files         287      289       +2     
  Lines       24622    24628       +6     
==========================================
+ Hits        14530    14537       +7     
  Misses       9218     9218              
+ Partials      874      873       -1     

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks!

Left two suggestions / thoughts; it looks like the PR has some unrelated commits (probably from the change you were working on); could you remove those from this PR?

secrets := body.ClusterVolumeSpec.Secrets
sort.SliceStable(secrets, func(i, j int) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps remove the intermediate variable to prevent confusing the secrets argument from the other secrets variable;

Suggested change
secrets := body.ClusterVolumeSpec.Secrets
sort.SliceStable(secrets, func(i, j int) bool {
sort.SliceStable(body.ClusterVolumeSpec.Secrets, func(i, j int) bool {

Copy link
Member

Choose a reason for hiding this comment

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

We should probably also update the runCreate() function to also sort these before making the request;

for key, secret := range options.secrets.GetAll() {
volOpts.ClusterVolumeSpec.Secrets = append(
volOpts.ClusterVolumeSpec.Secrets,
volume.Secret{
Key: key,
Secret: secret,
},
)
}

Wondering if we should add a GetSlice() (or something like that) method on MapOpt, which would return a sorted slice of the map;

cli/opts/opts.go

Lines 177 to 180 in c780f7c

// GetAll returns the values of MapOpts as a map.
func (opts *MapOpts) GetAll() map[string]string {
return opts.values
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update the runCreate() function as well!

As for getSlice(), I think it's a good idea. It will be useful in similar testing situations and also prevent flakes in the future. Should we add it to this PR?

@photra photra force-pushed the 3636-deflake-testvolumecreateclusteropts branch 5 times, most recently from 5a669b0 to 98903b3 Compare May 31, 2022 12:09
@photra
Copy link
Contributor Author

photra commented May 31, 2022

@thaJeztah the unrelated commits have been removed!

Let me know if you would like the getSlice function implemented as mentioned in the review above!

Add Secret sorting prior to request to prevent flakiness in CI

Signed-off-by: Phong Tran <[email protected]>

Co-authored-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Phong Tran <[email protected]>
@thaJeztah thaJeztah force-pushed the 3636-deflake-testvolumecreateclusteropts branch from 215efa0 to 1d85b4d Compare June 3, 2022 09:54
@thaJeztah thaJeztah added this to the 22.06.0 milestone Jun 3, 2022
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I did a quick squash of the commits; will merge once CI completes 👍

@thaJeztah thaJeztah merged commit 9cda6ce into docker:master Jun 3, 2022
@thaJeztah
Copy link
Member

done! thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test: TestVolumeCreateClusterOpts
3 participants