You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "PsiACE (via GitHub)" <gi...@apache.org> on 2023/03/15 17:09:47 UTC
[GitHub] [incubator-opendal] PsiACE opened a new pull request, #1632: refactor(bindings/c): unit tests with cmocka
PsiACE opened a new pull request, #1632:
URL: https://github.com/apache/incubator-opendal/pull/1632
I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/
## Summary
[cmocka](https://cmocka.org/) is an elegant unit testing framework for C with support for mock objects. Licensed under Apache-2.0.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #1632: refactor(bindings/c): unit tests with cmocka
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #1632:
URL: https://github.com/apache/incubator-opendal/pull/1632#issuecomment-1470431027
Maybe a simple C program is enough? And I expect we don't need do complex unit tests at bindings level.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #1632: refactor(bindings/c): unit tests with cmocka
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #1632:
URL: https://github.com/apache/incubator-opendal/pull/1632#issuecomment-1470429645
🥺, I don't like introducing things that need extra setup...
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #1632: feat(bindings/c): basic read and write op, a dirty way
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #1632:
URL: https://github.com/apache/incubator-opendal/pull/1632#discussion_r1138012224
##########
bindings/c/cbindgen.toml:
##########
Review Comment:
Do we still need this file now that `build.rs` has been introduced?
##########
bindings/c/include/opendal.h:
##########
@@ -17,25 +17,33 @@
* under the License.
*/
+#ifndef __OPENDAL_H__
+#define __OPENDAL_H__
-#ifndef _OPENDAL_H
-#define _OPENDAL_H
+/* Do not modify. This code was autogenerated by bindgen. */
-#include <stdint.h>
-#include <stddef.h>
+#include <stdarg.h>
#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <stdlib.h>
-#ifdef __cplusplus
-extern "C" {
-#endif // __cplusplus
+typedef struct BlockingOperator BlockingOperator;
Review Comment:
I have no knowledge about async/blocking in the C ecosystem. How do they handle naming conventions?
##########
bindings/c/include/opendal.h:
##########
@@ -17,25 +17,33 @@
* under the License.
*/
+#ifndef __OPENDAL_H__
+#define __OPENDAL_H__
-#ifndef _OPENDAL_H
-#define _OPENDAL_H
+/* Do not modify. This code was autogenerated by bindgen. */
-#include <stdint.h>
-#include <stddef.h>
+#include <stdarg.h>
#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <stdlib.h>
-#ifdef __cplusplus
-extern "C" {
-#endif // __cplusplus
+typedef struct BlockingOperator BlockingOperator;
-/*
- Hello, OpenDAL!
+typedef struct DynArray {
+ uint8_t *array;
+ size_t length;
+} DynArray;
+
+/**
+ * Hello, OpenDAL!
*/
void hello_opendal(void);
-#ifdef __cplusplus
-} // extern "C"
-#endif // __cplusplus
+struct BlockingOperator *blocking_op_create(void);
+
+struct DynArray blocking_read(const struct BlockingOperator *op, const char *path);
+
+void blocking_write(const struct BlockingOperator *op, const char *path, const struct DynArray *bs);
Review Comment:
Is it common for C to use `DynArray` to write data?
https://github.com/awslabs/aws-c-s3/blob/main/include/aws/s3/s3_client.h#L103-L105
##########
bindings/c/tests/test.c:
##########
@@ -18,7 +18,34 @@
*/
#include "opendal.h"
-int main(int argc, char *argv[]) {
- hello_opendal();
- return 0;
+#include <stdio.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+static void test_hello_opendal(void **state)
+{
+ (void)state; /* unused */
+
+ hello_opendal();
+}
+
+static void test_blocking_op(void **state)
+{
+ (void)state; /* unused */
+
+ BlockingOperator *op = blocking_op_create();
+ DynArray wbs = {"hello, world!", 13};
+ blocking_write(op, "opendal", &wbs);
+ DynArray rbs = blocking_read(op, "opendal");
+ printf("%s\n", rbs);
}
+
+int main(int argc, char **argv)
+{
+
+ const struct CMUnitTest tests[] = {
+ cmocka_unit_test(test_hello_opendal),
Review Comment:
I still don't see the necessity of introducing cmocka. Should we consider removing it now?
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo closed pull request #1632: feat(bindings/c): basic read and write op, a dirty way
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo closed pull request #1632: feat(bindings/c): basic read and write op, a dirty way
URL: https://github.com/apache/incubator-opendal/pull/1632
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #1632: refactor(bindings/c): unit tests with cmocka
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #1632:
URL: https://github.com/apache/incubator-opendal/pull/1632#issuecomment-1470440737
Yes, I do think testing is important. But we can do this without introducing any other deps.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] PsiACE commented on a diff in pull request #1632: feat(bindings/c): basic read and write op, a dirty way
Posted by "PsiACE (via GitHub)" <gi...@apache.org>.
PsiACE commented on code in PR #1632:
URL: https://github.com/apache/incubator-opendal/pull/1632#discussion_r1138233398
##########
bindings/c/tests/test.c:
##########
@@ -18,7 +18,34 @@
*/
#include "opendal.h"
-int main(int argc, char *argv[]) {
- hello_opendal();
- return 0;
+#include <stdio.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+static void test_hello_opendal(void **state)
+{
+ (void)state; /* unused */
+
+ hello_opendal();
+}
+
+static void test_blocking_op(void **state)
+{
+ (void)state; /* unused */
+
+ BlockingOperator *op = blocking_op_create();
+ DynArray wbs = {"hello, world!", 13};
+ blocking_write(op, "opendal", &wbs);
+ DynArray rbs = blocking_read(op, "opendal");
+ printf("%s\n", rbs);
}
+
+int main(int argc, char **argv)
+{
+
+ const struct CMUnitTest tests[] = {
+ cmocka_unit_test(test_hello_opendal),
Review Comment:
I would remove it, and unless we have something complicated, there are totally simpler ways to do it.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] messense commented on pull request #1632: feat(bindings/c): basic read and write op, a dirty way
Posted by "messense (via GitHub)" <gi...@apache.org>.
messense commented on PR #1632:
URL: https://github.com/apache/incubator-opendal/pull/1632#issuecomment-1471103254
IMHO, you can use `rust-bindgen` to generate Rust bindings for the C-API and write tests in Rust instead so you can just run `cargo test`. 😂
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] Xuanwo commented on pull request #1632: feat(bindings/c): basic read and write op, a dirty way
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #1632:
URL: https://github.com/apache/incubator-opendal/pull/1632#issuecomment-1474817916
Thank you for your effort. I will close this PR to give you more space to continue your work. You are welcome to reopen this PR when you are ready for reviewing. Thank you once again!
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-opendal] PsiACE commented on pull request #1632: refactor(bindings/c): unit tests with cmocka
Posted by "PsiACE (via GitHub)" <gi...@apache.org>.
PsiACE commented on PR #1632:
URL: https://github.com/apache/incubator-opendal/pull/1632#issuecomment-1470437643
> pleading_face, I don't like introducing things that need extra setup...
There are indeed some unit testing libraries that are head-only. I just want to avoid copying files directly.
Let me reconsider it during the day, in any case, I think testing (may be simple) is necessary.
--
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: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org