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

CARMA Cloud ambassador integration #200

Merged
merged 132 commits into from
May 3, 2024

Conversation

kruegersp
Copy link
Contributor

@kruegersp kruegersp commented Feb 16, 2024

PR Details

Description

This PR primarily focuses on merging a branch into develop, which encompasses the integration testing of CARMA Cloud with CDASim. The purpose of this integration test was to assess the communication between CARMA Cloud and CDASim, ensuring that:

All components operate correctly within CDASim without encountering any significant problems.
CARMA Cloud successfully registers with Ambassador.
CARMA Cloud receives and processes time synchronization messages.

Related GitHub Issue

Related Jira Key

CXC-10

Motivation and Context

How Has This Been Tested?

Types of changes

  • Defect fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that cause existing functionality to change)

Checklist:

  • I have added any new packages to the sonar-scanner.properties file
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@kruegersp kruegersp self-assigned this Feb 16, 2024
*/
public class CarmaCloudRegistrationReceiver implements Runnable
{
private static final int LISTEN_PORT = 1617; // which port for CARMA Cloud?
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still relevant?

private StringBuilder messageBuf = new StringBuilder();
private HttpServer oSrvr;

class TimeSyncHandler implements HttpHandler {
Copy link
Contributor

@paulbourelly999 paulbourelly999 May 2, 2024

Choose a reason for hiding this comment

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

Nit: We should probably leave a comment for the unit test to remove the HTTP Server and use mocks instead. Using an actual HttpServer for a unit test makes the unit test harder to maintain and likely causes the unit test to require more resources (time and processing). Not high priority right now but a TODO comment will help us track this technical debt.

Copy link
Contributor

@paulbourelly999 paulbourelly999 left a comment

Choose a reason for hiding this comment

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

Left some small comments.

Copy link
Contributor

@paulbourelly999 paulbourelly999 left a comment

Choose a reason for hiding this comment

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

Please remove comment/unused unit tests. Then I am good with this PR.

Copy link

sonarcloud bot commented May 3, 2024

@paulbourelly999 paulbourelly999 merged commit caac2ff into develop May 3, 2024
3 checks passed
@paulbourelly999 paulbourelly999 deleted the feature/carma-cloud-integration branch May 3, 2024 16:03
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