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

[test] Optimizations, improvements, and unit tests for common/common.go (part 1 of 3) #176

Merged
merged 10 commits into from
May 8, 2023

Conversation

art-tapin
Copy link
Contributor

@art-tapin art-tapin commented Apr 28, 2023

This pull request contains optimizations, improvements, and unit-tests for several functions in the common/common.go file. The changes made are as follows:

  • Refactored the NamespacedNameToObjectKey function to optimize its performance and added unit-tests for it.
  • Simplified the GetEmptySliceIfNil function for improved performance and added unit-tests for it.
  • Added comments for the FetchEnv and GetDefaultIfNil functions.
  • Added unit-tests for the FetchEnv and GetDefaultIfNil functions.

Note that this is only the first of three pull requests, and we have only tested 1/3 of the untested functions in the common/common.go file, so there is more work to be done.

This pull request partly closes issue #167.

Coverage: 59.2% of statements

@art-tapin art-tapin marked this pull request as ready for review April 28, 2023 18:32
@art-tapin art-tapin requested a review from a team as a code owner April 28, 2023 18:32
Copy link
Member

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

Looking good! a few bits to change and others to decide if we want to address them in this PR or not

pkg/common/common.go Show resolved Hide resolved
pkg/common/common.go Show resolved Hide resolved
pkg/common/common_test.go Outdated Show resolved Hide resolved
pkg/common/common_test.go Show resolved Hide resolved
pkg/common/common_test.go Outdated Show resolved Hide resolved
pkg/common/common_test.go Outdated Show resolved Hide resolved
pkg/common/common_test.go Show resolved Hide resolved
+ "sigs.k8s.io/controller-runtime/pkg/client" added as an import
…rformance (#167)

Refactored the NamespacedNameToObjectKey function to improve its performance by switching from using the strings.Split() function to using the strings.IndexRune() function. This new implementation is approximately 12-13 times faster for large datasets than the previous implementation.
@art-tapin art-tapin force-pushed the issue167/test-improvement-for-common-common.go-1 branch from 3ba80e6 to 09331da Compare May 3, 2023 12:49
- empty environment variable name is not expected
- one of the cases was a duplication of an existing one
@art-tapin art-tapin changed the title [test] Optimizations, improvements, and unit tests for common/common.go (part 1 of 4) [test] Optimizations, improvements, and unit tests for common/common.go (part 1 of 3) May 5, 2023
pkg/common/common_test.go Outdated Show resolved Hide resolved
pkg/common/common_test.go Outdated Show resolved Hide resolved
@art-tapin art-tapin requested a review from eguzki May 5, 2023 16:27
Copy link
Member

@didierofrivia didierofrivia left a comment

Choose a reason for hiding this comment

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

🎖️ and extra 🌮 for documenting the functions!

@didierofrivia didierofrivia merged commit 7b0f957 into main May 8, 2023
@didierofrivia didierofrivia deleted the issue167/test-improvement-for-common-common.go-1 branch May 8, 2023 13:16
didierofrivia pushed a commit that referenced this pull request May 17, 2023
…go (part 1 of 3) (#176)

* test: Add unit-tests for FetchEnv (#167)

* refactor: Add comment for FetchEnv() (#167)

* refactor: Add unit-tests for GetDefaultIfNil (#167)

* refactor: Add comment for GetDefaultIfNil (#167)

* test: Add unit-tests for GetEmptySliceIfNil (#167)

* refactor: Add comment for GetEmptySliceIfNil (#167)

* test: Add unit-tests for NamespacedNameToObjectKey (#167)

+ "sigs.k8s.io/controller-runtime/pkg/client" added as an import

* refactor: optimize NamespacedNameToObjectKey function for improved performance (#167)

Refactored the NamespacedNameToObjectKey function to improve its performance by switching from using the strings.Split() function to using the strings.IndexRune() function. This new implementation is approximately 12-13 times faster for large datasets than the previous implementation.

* refactor: delete three meaningless test cases for FetchEnv (#167)

- empty environment variable name is not expected
- one of the cases was a duplication of an existing one

* refactor: get rid two test-cases because of redundancy
didierofrivia pushed a commit that referenced this pull request May 24, 2023
…go (part 1 of 3) (#176)

* test: Add unit-tests for FetchEnv (#167)

* refactor: Add comment for FetchEnv() (#167)

* refactor: Add unit-tests for GetDefaultIfNil (#167)

* refactor: Add comment for GetDefaultIfNil (#167)

* test: Add unit-tests for GetEmptySliceIfNil (#167)

* refactor: Add comment for GetEmptySliceIfNil (#167)

* test: Add unit-tests for NamespacedNameToObjectKey (#167)

+ "sigs.k8s.io/controller-runtime/pkg/client" added as an import

* refactor: optimize NamespacedNameToObjectKey function for improved performance (#167)

Refactored the NamespacedNameToObjectKey function to improve its performance by switching from using the strings.Split() function to using the strings.IndexRune() function. This new implementation is approximately 12-13 times faster for large datasets than the previous implementation.

* refactor: delete three meaningless test cases for FetchEnv (#167)

- empty environment variable name is not expected
- one of the cases was a duplication of an existing one

* refactor: get rid two test-cases because of redundancy
didierofrivia pushed a commit that referenced this pull request May 31, 2023
…go (part 1 of 3) (#176)

* test: Add unit-tests for FetchEnv (#167)

* refactor: Add comment for FetchEnv() (#167)

* refactor: Add unit-tests for GetDefaultIfNil (#167)

* refactor: Add comment for GetDefaultIfNil (#167)

* test: Add unit-tests for GetEmptySliceIfNil (#167)

* refactor: Add comment for GetEmptySliceIfNil (#167)

* test: Add unit-tests for NamespacedNameToObjectKey (#167)

+ "sigs.k8s.io/controller-runtime/pkg/client" added as an import

* refactor: optimize NamespacedNameToObjectKey function for improved performance (#167)

Refactored the NamespacedNameToObjectKey function to improve its performance by switching from using the strings.Split() function to using the strings.IndexRune() function. This new implementation is approximately 12-13 times faster for large datasets than the previous implementation.

* refactor: delete three meaningless test cases for FetchEnv (#167)

- empty environment variable name is not expected
- one of the cases was a duplication of an existing one

* refactor: get rid two test-cases because of redundancy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants