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

SortUtils.h is missing tests #32420

Closed
tcarmelveilleux opened this issue Mar 4, 2024 · 0 comments · Fixed by #32421
Closed

SortUtils.h is missing tests #32420

tcarmelveilleux opened this issue Mar 4, 2024 · 0 comments · Fixed by #32421
Assignees
Labels
quality Software implementation quality tests

Comments

@tcarmelveilleux
Copy link
Contributor

SortUtils.h is used by DNS-SD, session eviction and Network Commissioning. The sorts claim to be stable, but there are zero unit tests.

The SortUtils.h methods should be unit-tested.

@tcarmelveilleux tcarmelveilleux added tests quality Software implementation quality labels Mar 4, 2024
@tcarmelveilleux tcarmelveilleux self-assigned this Mar 4, 2024
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Mar 4, 2024
- SortUtils.h has no tests. While it is apparently trivial,
  we should never have code that has no tests
- BubbleSort could crash (never-ending loop due to integer overflow)
  when input size was < 2. Its use is removed and replaced by InsertionSort
  which is known to be faster on average. A better solution is to move
  to using STL sorts in the future, but this requires analysis of flash
  costs and STL usage, given lambdas are used with sorts many places.

Fixes project-chip#32420

Testing done:
- Added missing unit tests.
- All integration tests still pass.
andy31415 added a commit that referenced this issue Mar 5, 2024
* Add tests to lib/support/SortUtils.h

- SortUtils.h has no tests. While it is apparently trivial,
  we should never have code that has no tests
- BubbleSort could crash (never-ending loop due to integer overflow)
  when input size was < 2. Its use is removed and replaced by InsertionSort
  which is known to be faster on average. A better solution is to move
  to using STL sorts in the future, but this requires analysis of flash
  costs and STL usage, given lambdas are used with sorts many places.

Fixes #32420

Testing done:
- Added missing unit tests.
- All integration tests still pass.

* Restyled by clang-format

* Fix build error on Android

* Add constness to operator==

---------

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Andrei Litvin <[email protected]>
erwinpan1 pushed a commit to erwinpan1/connectedhomeip that referenced this issue Mar 7, 2024
* Add tests to lib/support/SortUtils.h

- SortUtils.h has no tests. While it is apparently trivial,
  we should never have code that has no tests
- BubbleSort could crash (never-ending loop due to integer overflow)
  when input size was < 2. Its use is removed and replaced by InsertionSort
  which is known to be faster on average. A better solution is to move
  to using STL sorts in the future, but this requires analysis of flash
  costs and STL usage, given lambdas are used with sorts many places.

Fixes project-chip#32420

Testing done:
- Added missing unit tests.
- All integration tests still pass.

* Restyled by clang-format

* Fix build error on Android

* Add constness to operator==

---------

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Andrei Litvin <[email protected]>
huangxuyong pushed a commit to huangxuyong/connectedhomeip that referenced this issue Mar 19, 2024
* Add tests to lib/support/SortUtils.h

- SortUtils.h has no tests. While it is apparently trivial,
  we should never have code that has no tests
- BubbleSort could crash (never-ending loop due to integer overflow)
  when input size was < 2. Its use is removed and replaced by InsertionSort
  which is known to be faster on average. A better solution is to move
  to using STL sorts in the future, but this requires analysis of flash
  costs and STL usage, given lambdas are used with sorts many places.

Fixes project-chip#32420

Testing done:
- Added missing unit tests.
- All integration tests still pass.

* Restyled by clang-format

* Fix build error on Android

* Add constness to operator==

---------

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Andrei Litvin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quality Software implementation quality tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant