Feature/509 remove cpputests #711

pnoltes · 2024-01-01T18:56:46Z

Intro

This PR removes the usage of Cpputest. Cpputest was used for the deprecated linked list, array_list tests, hash map, and IP utils.

Linked List

This PR removes the linked list implementation and the linked list Cpputests. Linked list was still used in the shell TUI history, which has now been refactored to use the array list instead.

Deprecated Array List

The header, implementation, and Cpputests for the deprecated array list API (API without the celix_ prefix) have been removed. Additionally, any usage of the deprecated array list API has been updated to use celix_array_list. Note that the removal of the deprecated array list usage accounts for the majority of the changed files.

IP Utils

The Cpputests for IP utils and their implementation have been refactored to GTests with an updated implementation. This includes enhanced error handling and corresponding tests. It's important to note that IP utils are currently not used in the Celix framework or bundles. However, they could be valuable for remote service implementations, specifically the celix_utils_findIpInSubnet function, which can identify the appropriate IP for listening based on a subnet CIDR notation.

Deprecated Hash Map

The Cpputests for the deprecated hash map (hash map API without the celix_ prefix) have been refactored to GTests and are maintained as much as possible in their original form.

Also refactor shell tui history to use array list instead of linked list.

Also improves error handling

# Conflicts: # .github/workflows/coverity-scan.yml # .github/workflows/macos.yml # .github/workflows/ubuntu.yml # bundles/pubsub/examples/pubsub/pubsub_websocket/private/include/pubsub_websocket_private.h # bundles/pubsub/examples/pubsub/pubsub_websocket/private/src/ps_websocket_activator.c # bundles/pubsub/examples/pubsub/pubsub_websocket/private/src/pubsub_websocket_example.c # bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_handler.c # bundles/pubsub/pubsub_admin_zmq/src/pubsub_zmq_admin.c # bundles/pubsub/pubsub_admin_zmq/src/pubsub_zmq_topic_receiver.c # bundles/pubsub/pubsub_discovery/src/pubsub_discovery_impl.c # bundles/pubsub/pubsub_utils/src/pubsub_utils.c

Also fix invalid array create in dm_dependency_manager_impl.c and add missing status initialize in service_registry.c.

PengZheng

It seems that this PR can be merged without bringing about any new issues except for some incorrect indentations.
That is, all remarks I gave above apply to existing issues.
But I do want to have a brief discussion about them before the actual merge happens.

I saw lots of arrayList replaced by celix_arrayList, but only saw very few #include "celix_array_list.h", which suggests that we depend heavily on "transtive includes". Transtive includes often increase accidental complexities. Removing an include from a public header file may break a lot of downstream builds, thus lead to unhappy users. I suggest we follow John Lakos' advice to forbid transtive includes "except to access a public base class of a type whose definition is included directly".

The current ip_utils is not very satisfatctory. In addition to the drawbacks mentioned below, the current implementation lacks support for ipv6. Considering IPv6 link-local address combined with Bonjour provides real zero configuration networking experience, IPv6 support is critical for IOT devices.

PengZheng · 2024-01-02T03:20:31Z

bundles/remote_services/discovery_common/include/endpoint_discovery_poller.h

@@ -55,6 +55,6 @@ celix_status_t endpointDiscoveryPoller_destroy(endpoint_discovery_poller_t *poll
 celix_status_t endpointDiscoveryPoller_addDiscoveryEndpoint(endpoint_discovery_poller_t *poller, char *url);
 celix_status_t endpointDiscoveryPoller_removeDiscoveryEndpoint(endpoint_discovery_poller_t *poller, char *url);

-celix_status_t endpointDiscoveryPoller_getDiscoveryEndpoints(endpoint_discovery_poller_t *poller, array_list_pt urls);
+celix_status_t endpointDiscoveryPoller_getDiscoveryEndpoints(endpoint_discovery_poller_t *poller, celix_array_list_t* urls);


Missing #include "celix_array_list.h".

bundles/remote_services/discovery_common/src/endpoint_discovery_poller.c

libs/utils/src/ip_utils.c

libs/utils/src/hash_map_private.h

pnoltes · 2024-01-04T10:30:37Z

The current ip_utils is not very satisfatctory. In addition to the drawbacks mentioned below, the current implementation lacks support for ipv6. Considering IPv6 link-local address combined with Bonjour provides real zero configuration networking experience, IPv6 support is critical for IOT devices.

Good point, then I will remove the ip utils in this branch. If this is needed again the ip utils in commit id 1a67dabc5b87f0c8250ce4351df310873be3b0b9 can be restored.

pnoltes · 2024-01-04T10:45:40Z

I saw lots of arrayList replaced by celix_arrayList, but only saw very few #include "celix_array_list.h", which suggests that we depend heavily on "transtive includes". Transtive includes often increase accidental complexities. Removing an include from a public header file may break a lot of downstream builds, thus lead to unhappy users. I suggest we follow John Lakos' advice to forbid transtive includes "except to access a public base class of a type whose definition is included directly".

Also a good point. I do think this is more relevant for C++ code then for C code, but non the less also good to do for our C code.
If we forbid transtive dependencies, IMO we have to use forward declaration. So instead of including celix_array_list.h if a function returns or accepts a celix array list, we only need the following declaration:
`typedef struct celix_array_list celix_array_list_t;
I think it is then wise to define a guideline for this, something like:

For a C object, the following header guideline should be followed:

The public api for a C object can can be found in a header named: celix_<object_snake_name>.h
The opaque typedefs (forward declaration) can be found in a header named: celix_<object_name_name>_type.h. This header ideally should only contain opaque typedefs, but can also contain concrete struct definition if needed (e.g. celix_properties_entry_t or celix_properties_value_type_e, etc)
If needed all private functions or structs (only used inside the library which provided the public api) can be found in a header named: celix_<object_snake_name>_private.h
If needed all internal functions or structs (only used inside the Apache Celix project, but not meant as public API), can be found in a header named: ``celix_<object_snake_name>_internal.h`

I also think this should be addressed in a separate ticket, if you agree I will make a ticket for this.

PengZheng · 2024-01-04T11:10:26Z

I also think this should be addressed in a separate ticket, if you agree I will make a ticket for this.

Yes, we can address this later.

# Conflicts: # CHANGES.md

# Conflicts: # CHANGES.md # bundles/remote_services/rsa_spi/include/endpoint_description.h # bundles/remote_services/rsa_spi/include/endpoint_listener.h # libs/framework/include/celix_api.h # libs/framework/include_deprecated/bundle_context.h # libs/framework/include_deprecated/dm_component.h # libs/framework/src/service_registry.c # libs/utils/include_deprecated/celix_utils_api.h

pnoltes · 2024-01-04T13:58:27Z

I also think this should be addressed in a separate ticket, if you agree I will make a ticket for this.

Yes, we can address this later.

Created #715 for this

pnoltes added 15 commits December 23, 2023 17:11

Remove cpputest config, cpputest usage and linked list

8af7bff

Also refactor shell tui history to use array list instead of linked list.

Remove cpputest from build config

6a4d2a9

Remove cpputests for array and linked list

577551e

Remove deprecated array_list and its usage

daa0060

Refactor hash_map cpputests to gtests

5d97401

Refactor ip utils and ip utils tests

d1f7d10

Also improves error handling

Remove cpputest from build config

8d113a3

Improve error handling of IP utils

01b3af1

Replace array_list with celix_array_list in rsa shm v2

61c641a

Replace array_list with celix_array_list in conan exmpl test

a851bf9

Separate celix err ei test to prevent unintentionally init of tss

f2317ed

Fix memleak in celix_utils_findIpInSubnet when calling getifaddrs

4bb8990

Fix invalid snprintf

1f8dea8

Also fix invalid array create in dm_dependency_manager_impl.c and add missing status initialize in service_registry.c.

Replace EXPECT_EQ for pointer compare

1a67dab

pnoltes mentioned this pull request Jan 1, 2024

Feature/coverity fix #709

Closed

PengZheng reviewed Jan 2, 2024

View reviewed changes

pnoltes added 6 commits January 4, 2024 14:12

Merge branch 'master' into feature/509-remove-cpputests

4de66c3

# Conflicts: # CHANGES.md

Remove ip utils

b4b26a3

Remove unused linked_list source files

7a9bae9

Refactor endpointDiscoveryPoller_poll for indent and error handling

cd8bfd5

Remove unneeded export macros in hash_map_private.h

36d8c40

pnoltes requested a review from PengZheng January 4, 2024 13:58

PengZheng approved these changes Jan 4, 2024

View reviewed changes

pnoltes merged commit 7259275 into master Jan 4, 2024
28 checks passed

pnoltes deleted the feature/509-remove-cpputests branch January 4, 2024 14:43

PengZheng mentioned this pull request Feb 2, 2024

Refactor CppUTest tests to gtest #96

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/509 remove cpputests #711

Feature/509 remove cpputests #711

pnoltes commented Jan 1, 2024

PengZheng left a comment •

edited

Loading

PengZheng Jan 2, 2024

pnoltes commented Jan 4, 2024

pnoltes commented Jan 4, 2024

PengZheng commented Jan 4, 2024

pnoltes commented Jan 4, 2024

Feature/509 remove cpputests #711

Feature/509 remove cpputests #711

Conversation

pnoltes commented Jan 1, 2024

Intro

Linked List

Deprecated Array List

IP Utils

Deprecated Hash Map

PengZheng left a comment • edited Loading

Choose a reason for hiding this comment

PengZheng Jan 2, 2024

Choose a reason for hiding this comment

pnoltes commented Jan 4, 2024

pnoltes commented Jan 4, 2024

PengZheng commented Jan 4, 2024

pnoltes commented Jan 4, 2024

PengZheng left a comment •

edited

Loading