You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by "xuzhenbao (via GitHub)" <gi...@apache.org> on 2023/11/23 13:13:31 UTC
[PR] Add unit test for dfi (celix)
xuzhenbao opened a new pull request, #691:
URL: https://github.com/apache/celix/pull/691
Add unit test for `DFI`, and fix some bugs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit test for dfi (celix)
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on code in PR #691:
URL: https://github.com/apache/celix/pull/691#discussion_r1404110849
##########
libs/dfi/src/dyn_type.c:
##########
@@ -294,6 +296,14 @@ static int dynType_parseComplex(FILE *stream, dyn_type *type) {
if (status == OK) {
TAILQ_FOREACH(entry, &type->complex.entriesHead, entries) {
count +=1;
+ struct complex_type_entry *entry2 = NULL;
Review Comment:
This is far from enough for runtime verification. If descriptors are generated from service header, then we need not to worry about this, since the compiler will check it for us.
##########
libs/dfi/src/json_rpc.c:
##########
@@ -230,7 +230,7 @@ int jsonRpc_call(dyn_interface_type *intf, void *service, const char *request, c
} else {
json_object_set_new_nocheck(payload, "e", json_integer(funcCallStatus));
}
- response = json_dumps(payload, JSON_DECODE_ANY);
+ response = json_dumps(payload, JSON_COMPACT | JSON_ENCODE_ANY);//Should use JSON_COMPACT, it can reduce the size of the JSON string.
Review Comment:
Nice.
##########
libs/dfi/src/dyn_function.c:
##########
@@ -93,7 +93,7 @@ int dynFunction_parse(FILE *descriptor, struct types_head *refTypes, dyn_functio
int dynFunction_parseWithStr(const char *descriptor, struct types_head *refTypes, dyn_function_type **out) {
int status = OK;
- FILE *stream = fmemopen((char *)descriptor, strlen(descriptor), "r");
+ FILE *stream = fmemopen((char *)descriptor, strlen(descriptor) + 1, "r");
Review Comment:
Nice catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit test for dfi (celix)
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on code in PR #691:
URL: https://github.com/apache/celix/pull/691#discussion_r1404110849
##########
libs/dfi/src/dyn_type.c:
##########
@@ -294,6 +296,14 @@ static int dynType_parseComplex(FILE *stream, dyn_type *type) {
if (status == OK) {
TAILQ_FOREACH(entry, &type->complex.entriesHead, entries) {
count +=1;
+ struct complex_type_entry *entry2 = NULL;
Review Comment:
This is far from enough for runtime verification. If descriptors are generated from service header, then we need not to worry about this, since the compiler will check it for us. By the way, O(N^2) complexity is terrible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit test for dfi (celix)
Posted by "PengZheng (via GitHub)" <gi...@apache.org>.
PengZheng commented on code in PR #691:
URL: https://github.com/apache/celix/pull/691#discussion_r1404110849
##########
libs/dfi/src/dyn_type.c:
##########
@@ -294,6 +296,14 @@ static int dynType_parseComplex(FILE *stream, dyn_type *type) {
if (status == OK) {
TAILQ_FOREACH(entry, &type->complex.entriesHead, entries) {
count +=1;
+ struct complex_type_entry *entry2 = NULL;
Review Comment:
This is far from enough for runtime verification. If descriptors are generated from service header, then we need not to worry about this, since the compiler will check it for us. By the way, O(N) complexity is terrible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit test for dfi (celix)
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #691:
URL: https://github.com/apache/celix/pull/691#issuecomment-1824428541
## [Codecov](https://app.codecov.io/gh/apache/celix/pull/691?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `1 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`e9642b9`)](https://app.codecov.io/gh/apache/celix/commit/e9642b94a9487e74b174732a39812d7f4fa2a75e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 83.14% compared to head [(`fb4b737`)](https://app.codecov.io/gh/apache/celix/pull/691?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 83.48%.
> :exclamation: Current head fb4b737 differs from pull request most recent head 4762101. Consider uploading reports for the commit 4762101 to get more accurate results
| [Files](https://app.codecov.io/gh/apache/celix/pull/691?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [libs/dfi/src/dyn\_avpr\_type.c](https://app.codecov.io/gh/apache/celix/pull/691?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGlicy9kZmkvc3JjL2R5bl9hdnByX3R5cGUuYw==) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/celix/pull/691?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #691 +/- ##
==========================================
+ Coverage 83.14% 83.48% +0.33%
==========================================
Files 254 254
Lines 32935 32957 +22
==========================================
+ Hits 27384 27513 +129
+ Misses 5551 5444 -107
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/celix/pull/691?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit test for dfi (celix)
Posted by "xuzhenbao (via GitHub)" <gi...@apache.org>.
xuzhenbao merged PR #691:
URL: https://github.com/apache/celix/pull/691
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit test for dfi (celix)
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes commented on code in PR #691:
URL: https://github.com/apache/celix/pull/691#discussion_r1408143026
##########
libs/dfi/src/dyn_function.c:
##########
@@ -93,7 +93,7 @@ int dynFunction_parse(FILE *descriptor, struct types_head *refTypes, dyn_functio
int dynFunction_parseWithStr(const char *descriptor, struct types_head *refTypes, dyn_function_type **out) {
int status = OK;
- FILE *stream = fmemopen((char *)descriptor, strlen(descriptor), "r");
+ FILE *stream = fmemopen((char *)descriptor, strlen(descriptor) + 1, "r");
Review Comment:
yes, nice catch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add unit test for dfi (celix)
Posted by "pnoltes (via GitHub)" <gi...@apache.org>.
pnoltes commented on PR #691:
URL: https://github.com/apache/celix/pull/691#issuecomment-1828525439
Good to see some additional test for libdfi. I will try to review this PR this week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@celix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org