Skip to content

Conversation

@dijidiji
Copy link

@dijidiji dijidiji commented Aug 6, 2025

Description

This adds tests for the camera functions in camera.cpp. Duplicate of #119 but without the tag changes.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Testing Checklist

  • Tested with skunit_tests

I've verified that the tests run and assertions pass as expected. I've also run code coverage which reports 80% coverage for camera.cpp

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have requested a review from ... on the Pull Request

Copy link

@Broccoli811 Broccoli811 left a comment

Choose a reason for hiding this comment

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

The new test file for the camera module looks solid. It covers a broad range of key functionalities for a camera function, and it is well documented and readable to me.

All tests passed successfully in my environment, and the expected results match the documented behavior in camera.cpp

A suggestion that could be useful is to add edge case tests for when the camera is moved to negative/unreasonable coordinates or when the rectangles lie exactly on the screen border. This may ensure behavior is consistent in those scenarios.

Also, you may want to update the company board card on your additions/tests and potentially reference the camera.cpp document, it may sound trivial to some, but it doesn't hurt to add more context to the card for the next reviewer :)

Overall, great implementation!

Copy link

@JPF2209 JPF2209 left a comment

Choose a reason for hiding this comment

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

Type of Change

  • This piece of code correctly identifies the changes made being adding additional tests

Code Readability

  • The code is very understandable and clear in what it needs to be comparing it to the other code.

Maintainability

  • As this is in the same style as the other code, it's quite maintainable being simple while doing the job.

Code Simplicity

  • This code is simple in it's execution following established design patterns and best practices

Edge Cases

  • The code deals with edge cases simply since the inner functionality of the test code manages this

Test Thoroughness

  • For all the key tests in this type of scenario, this covers it all and the code has been tested in multiple environments and it works.

Backward Compatibility

  • The code doesn't break existing functionality in the way it works.

Performance Considerations

  • Performance is usually more of a concern with the function itself but this works out well for performance.

Security Concerns

  • Ensure to fulfilll branch protection policy

Dependencies

  • There are no new added dependencies.

Documentation

  • The documentation provided is through and simple to follow at the same time

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants