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

[22317] Test service node listener #62

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Javgilavi
Copy link
Contributor

This PR add the blackbox tests for service node listener and the minimum needed implementation.

@Javgilavi Javgilavi requested a review from Mario-DL January 15, 2025 16:11
@Javgilavi Javgilavi changed the title Test service node listener [22317] Test service node listener Jan 16, 2025
Signed-off-by: Javier Gil Aviles <[email protected]>
@Javgilavi Javgilavi force-pushed the test/service_node_listener branch from c0460e3 to 4de5353 Compare January 16, 2025 08:48
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

This test suite will gracefully assert the feature's correct behavior, good work.
Leaving some suggestions though

/**
* @brief This method sends a request to node via service for changing its configuration.
* @param [in] req configuration request, contain which node and configuration file
* @param [out] res response from the node with its new configuration
Copy link
Member

Choose a reason for hiding this comment

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

With the last update, this line should be removed and specify the return instead

Copy link
Member

Choose a reason for hiding this comment

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

I think this suggestion was not applied, I am missing the return in the doxygen comment

sustainml_cpp/test/blackbox/api/ManagedNodeService.hpp Outdated Show resolved Hide resolved
sustainml_cpp/test/blackbox/api/TaskInjector.hpp Outdated Show resolved Hide resolved
sustainml_cpp/test/blackbox/api/TaskReceiver.hpp Outdated Show resolved Hide resolved
sustainml_cpp/test/blackbox/api/ManagedNodeService.hpp Outdated Show resolved Hide resolved
@Mario-DL
Copy link
Member

Additionally, we should uncrustify the types.cpp (which is not generated code) and OrchestratorNode.hpp as the ci reports code divergences on them

Signed-off-by: Javier Gil Aviles <[email protected]>
@Javgilavi Javgilavi requested a review from Mario-DL January 22, 2025 15:58
Signed-off-by: Javier Gil Aviles <[email protected]>
@Javgilavi Javgilavi force-pushed the test/service_node_listener branch from 4a18663 to d73e7f1 Compare January 23, 2025 16:06
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.

2 participants