You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2020/02/13 20:57:36 UTC
[GitHub] [qpid-dispatch] nicob87 opened a new pull request #684: WIP: (Work
In Progress) DISPATCH-1568: using doctest
nicob87 opened a new pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684
- Demo using the library that Jiri Daněk suggested in the issue, it seems to be a very nice option for several reasons, it is an evolution of catch2, compile time significantly smaller, support nested "subcases" which simplifyes setup and teardown, it is a single header file, and others.
see: https://github.com/onqtam/doctest
- jira: https://issues.apache.org/jira/browse/DISPATCH-1568
- Commit history will be fixed. a lot of junk for now.
- To run the demo, just:
```
[nbrignon@localhost build (nb-doctest)]$ cmake ..
...
[nbrignon@localhost build (nb-doctest)]$ make c_unittest
...
```
- and then we can execute the binary directly:
```
[nbrignon@localhost build (nb-doctest)]$ ./tests/c_unittest
[doctest] doctest version is "2.3.6"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases: 5 | 5 passed | 0 failed | 0 skipped
[doctest] assertions: 15 | 15 passed | 0 failed |
[doctest] Status: SUCCESS!
```
- or debug it:
```
[nbrignon@localhost build (nb-doctest)]$ gdb ./tests/c_unittest
...
(gdb) b safe_snprintf
Breakpoint 1 at 0x408330
(gdb) run
...
Breakpoint 1, safe_snprintf (str=str@entry=0x7fffffffcd00 " \313\371\367\377\177", size=size@entry=19, format=format@entry=0x4228d2 "something")
at /home/nbrignon/qpid-dispatch/src/router_core/terminus.c:89
89 size_t safe_snprintf(char *str, size_t size, const char *format, ...) {
```
- or execute it through ctest:
```
nbrignon@localhost build (nb-doctest)]$ ctest -VV -R c_unittest
...
test 1
Start 1: c_unittest
1: Test command: /home/nbrignon/qpid-dispatch/build/tests/c_unittest
1: Test timeout computed to be: 600
1: [doctest] doctest version is "2.3.6"
1: [doctest] run with "--help" for options
1: ===============================================================================
1: [doctest] test cases: 5 | 5 passed | 0 failed | 0 skipped
1: [doctest] assertions: 15 | 15 passed | 0 failed |
1: [doctest] Status: SUCCESS!
1/1 Test #1: c_unittest ....................... Passed 0.01 sec
The following tests passed:
c_unittest
100% tests passed, 0 tests failed out of 1
Total Test time (real) = 0.01 sec
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] nicob87 commented on a change in pull request #684:
WIP: (Work In Progress) DISPATCH-1568: using doctest
Posted by GitBox <gi...@apache.org>.
nicob87 commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r381018665
##########
File path: tests/test_terminus_mocks.c
##########
@@ -0,0 +1,32 @@
+#include <strings.h>
+#include <stdio.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdarg.h>
+#include <inttypes.h>
+#include <limits.h>
Review comment:
only string.h, stdbool.h and stdarg.h are needed here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jdanekrh commented on a change in pull request
#684: WIP: (Work In Progress) DISPATCH-1568: using doctest
Posted by GitBox <gi...@apache.org>.
jdanekrh commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r380251166
##########
File path: tests/c_unittests_main.cpp
##########
@@ -0,0 +1,2 @@
+#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN
+#include "qdr_doctest.h"
Review comment:
```suggestion
#include "doctest.h"
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jdanekrh commented on a change in pull request
#684: WIP: (Work In Progress) DISPATCH-1568: using doctest
Posted by GitBox <gi...@apache.org>.
jdanekrh commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r380251166
##########
File path: tests/c_unittests_main.cpp
##########
@@ -0,0 +1,2 @@
+#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN
+#include "qdr_doctest.h"
Review comment:
```suggestion
#include "doctest.h"
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] nicob87 commented on a change in pull request #684:
WIP: (Work In Progress) DISPATCH-1568: using doctest
Posted by GitBox <gi...@apache.org>.
nicob87 commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r381033409
##########
File path: src/router_core/terminus.c
##########
@@ -75,25 +76,47 @@ void qdr_terminus_free(qdr_terminus_t *term)
free_qdr_terminus_t(term);
}
+#ifdef TESTING
+__attribute__((weak))
+int mocked_vsnprintf(char *str, size_t size, const char *format, ...){return -1;}
+#endif
// DISPATCH-1461: snprintf() is evil - it returns >= size on overflow. This
// wrapper will never return >= size, even if truncated. This makes it safe to
// do pointer & length arithmetic without overflowing the destination buffer in
// qdr_terminus_format()
-//
-static inline int safe_snprintf(char *str, size_t size, const char *format, ...)
-{
+// not static to be used unit-tested
+size_t safe_snprintf(char *str, size_t size, const char *format, ...) {
+ // max size allowed must be INT_MAX (since vsnprintf reutrns an int)
+ if (size == 0 || size > INT_MAX) {
+ //TODO log a warning somewhere?
+ return 0;
+ }
+ int max_possible_return_value = (int)(size - 1);
va_list ap;
va_start(ap, format);
+#ifdef TESTING
+ int rc = mocked_vsnprintf(str, size, format, ap);
Review comment:
Hi @jdanekrh, Awesome review! Thanks.
About the preprocessor, yes, I agree that code in this pr is polluted and it is possible to do it cleaner, with a preprocessor "seam". More over that I do not like using preprocessor at all. And also I do not like to compile twice just to be able to use the preprocessor. BUT I was looking for a way to do not pollute the production "binary". I didn't know a couple of tricks explained in your documentation.
I think it is possible to do something like what is explained in [2] (Run-time function interception) with the addition here we are mixing c and c++. It looks like it may work :-P.
Will give it a try tomorrow or one of this days.
Regards!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jdanekrh commented on a change in pull request
#684: WIP: (Work In Progress) DISPATCH-1568: using doctest
Posted by GitBox <gi...@apache.org>.
jdanekrh commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r380251798
##########
File path: tests/c_unittests_main.cpp
##########
@@ -0,0 +1,2 @@
+#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN
+#include "qdr_doctest.h"
Review comment:
aha, right. I misunderstood what you are doing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] jdanekrh commented on a change in pull request
#684: WIP: (Work In Progress) DISPATCH-1568: using doctest
Posted by GitBox <gi...@apache.org>.
jdanekrh commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r380620641
##########
File path: src/router_core/terminus.c
##########
@@ -75,25 +76,47 @@ void qdr_terminus_free(qdr_terminus_t *term)
free_qdr_terminus_t(term);
}
+#ifdef TESTING
+__attribute__((weak))
+int mocked_vsnprintf(char *str, size_t size, const char *format, ...){return -1;}
+#endif
// DISPATCH-1461: snprintf() is evil - it returns >= size on overflow. This
// wrapper will never return >= size, even if truncated. This makes it safe to
// do pointer & length arithmetic without overflowing the destination buffer in
// qdr_terminus_format()
-//
-static inline int safe_snprintf(char *str, size_t size, const char *format, ...)
-{
+// not static to be used unit-tested
+size_t safe_snprintf(char *str, size_t size, const char *format, ...) {
+ // max size allowed must be INT_MAX (since vsnprintf reutrns an int)
+ if (size == 0 || size > INT_MAX) {
+ //TODO log a warning somewhere?
+ return 0;
+ }
+ int max_possible_return_value = (int)(size - 1);
va_list ap;
va_start(ap, format);
+#ifdef TESTING
+ int rc = mocked_vsnprintf(str, size, format, ap);
Review comment:
I was thinking about this and I don't like this much. I understand that in C, there isn't many options to replace vsnprintf calls in tests, but this "pollutes" the production code a lot.
In the C++ world, they seem to call these techniques "seams", [1] [2] [3]. Dispatch already uses the linker seam in two places, to mock `qd_server_timeout` and `qd_timer_now`.
I generally like using the linker better, it seems cleaner to me.
[1] http://www.informit.com/articles/article.aspx?p=359417&seqNum=3
[2] https://accu.org/index.php/journals/1927 Refactoring Towards Seams in C++
[3] https://cute-test.com/guides/mocking-with-cute/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] nicob87 commented on a change in pull request #684:
WIP: (Work In Progress) DISPATCH-1568: using doctest
Posted by GitBox <gi...@apache.org>.
nicob87 commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r381369258
##########
File path: tests/test_terminus_mocks.c
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+#define _GNU_SOURCE //needed for dlfcn.h to contain RTLD_NEXT
+#include <stdbool.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <dlfcn.h>
+
+bool mocked = false;
+int mocked_rc = 0;
+
+void mock(bool m) {
+ mocked = m;
+}
+void mocked_value(int v)
+{
+ mocked_rc = v;
+}
+
+/* Even without any "#include" compiler complaing about unmatching with stdio.h
+ * vsnprintf. */
+/*-Werror=builtin-declaration-mismatch*/
+#define ARGUMENTS char *str, size_t size, const char *format, ...
Review comment:
this will change
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-dispatch] nicob87 commented on a change in pull request #684:
WIP: (Work In Progress) DISPATCH-1568: using doctest
Posted by GitBox <gi...@apache.org>.
nicob87 commented on a change in pull request #684: WIP: (Work In Progress) DISPATCH-1568: using doctest
URL: https://github.com/apache/qpid-dispatch/pull/684#discussion_r381019059
##########
File path: src/router_core/terminus.c
##########
@@ -75,25 +76,47 @@ void qdr_terminus_free(qdr_terminus_t *term)
free_qdr_terminus_t(term);
}
+#ifdef TESTING
+__attribute__((weak))
Review comment:
no need to make it weak since this goes to a shared library. __attribute__((noinline)) probably is a good idea
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org