Merge lp:~stefan.goetz-deactivatedaccount/hipl/unittests into lp:hipl
- unittests
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 5030 |
Proposed branch: | lp:~stefan.goetz-deactivatedaccount/hipl/unittests |
Merge into: | lp:hipl |
Diff against target: |
676 lines (+588/-2) 8 files modified
INSTALL (+3/-2) Makefile.am (+15/-0) configure.ac (+4/-0) doc/HACKING (+116/-0) test/README.UNITTESTS (+5/-0) test/check_lib_core.c (+55/-0) test/lib/core/hit.c (+195/-0) test/lib/core/straddr.c (+195/-0) |
To merge this branch: | bzr merge lp:~stefan.goetz-deactivatedaccount/hipl/unittests |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Miika Komu | Approve | ||
Stefan Götz (community) | Approve | ||
Review via email: mp+38672@code.launchpad.net |
Commit message
Description of the change
This branch introduces the unit test framework 'check' (http://
It was recently discussed on hipl-dev how useful unit tests were. I added a small number of tests of very simple HIPL functions to the testing suite and found several code bugs as well as inconsistencies between documentation and implementation. So I think there is at least some value to unit tests. So the question is whether this framework should be adopted in HIPL and whether developers are willing to embrace unit testing, maybe for new code?
Miika Komu (miika-iki) wrote : | # |
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
Hi!
> Great! I think this is a welcome addition for everyone.
Yay :-) Do you think it would make sense to have a policy around unit tests?
Something like 'You must not commit added code that is not covered by unit
tests'? It sounds appealing for the code quality but it's also a bit of work,
and there would definitely need to be some form of consensus on the topic, I
guess :)
> I would hope someone else reviews the code too.
A note I had been forgetting: the sample tests fail all over the place, not
because they are buggy but because they find actual implementation problems. I'm
having another branch in the pipes that would take care of these issues in the
hipl code and make all unit tests succeed.
> a. fail_unless(
> b. use a thread to kick hipd up, fail_unless(
> c. fail_unless(
It might work, although I'm not too sure how gracefully check handles threading.
Maybe one simply needs to try. Would such a test need to be run with root
privileges?
Stefan
--
Stefan Götz, Ph.D. Student
Distributed Systems Group
Chair for Computer Science IV, RWTH Aachen, Germany
http://
Miika Komu (miika-iki) wrote : | # |
Hi,
On 10/18/2010 10:52 AM, Stefan Götz wrote:
> Hi!
>
>> Great! I think this is a welcome addition for everyone.
>
> Yay :-) Do you think it would make sense to have a policy around unit tests?
> Something like 'You must not commit added code that is not covered by unit
> tests'? It sounds appealing for the code quality but it's also a bit of work,
> and there would definitely need to be some form of consensus on the topic, I
> guess :)
not a bad idea!
>> I would hope someone else reviews the code too.
>
> A note I had been forgetting: the sample tests fail all over the place, not
> because they are buggy but because they find actual implementation problems. I'm
> having another branch in the pipes that would take care of these issues in the
> hipl code and make all unit tests succeed.
Thanks.
>> a. fail_unless(
>> b. use a thread to kick hipd up, fail_unless(
>> c. fail_unless(
>
> It might work, although I'm not too sure how gracefully check handles threading.
> Maybe one simply needs to try. Would such a test need to be run with root
> privileges?
Alternatives (a) and (c) could be implemented without threads. All of
the alternatives could be implemented without root privileges if the raw
sockets would be disabled (i.e. hipconf nat plain-udp).
René Hummen (rene-hummen) wrote : | # |
Your code adds 'check' as a dependency for HIPL on all platforms. I suggest to implement an option in configure.ac that allows configuring 'check' functionality with AC_CHECK_LIB and to surround all occurrences of testing implementations by an if HIP_UNITTEST ... endif in the Makefile.am.
This will remove the dependency for default compilations.
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
Hi René!
> Your code adds 'check' as a dependency for HIPL on all platforms. I suggest to implement an option in configure.ac [...]
Hm, please note that only 'make check' has this dependency, i.e., when someone
wants to run the unit tests anyway. So someone who just wants to build the HIPL
binaries from source via 'make' does not depend on check on any platform.
Is it really useful to have a configure option to disable the unit tests, even
if one can simply choose not to run them in the first place?
But you reminded me that the INSTALL file should be updated to note that 'check'
is a recommended dependency for developers.
Stefan
- 4999. By Stefan Götz
-
- added line at end of file for good style
- 5000. By Stefan Götz
-
- added a warning that it is ok for the unit tests to use forward declarations but everyone else should use header files
- 5001. By Stefan Götz
-
- added a brief semi-explanation why certain forward declarations are necessary
- 5002. By Stefan Götz
-
- emit a warning at configure time if the check library needed for unit tests is not found
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
Hi Rene!
> surround all occurrences of testing
> implementations by an if HIP_UNITTEST ... endif in the Makefile.am.
So I guess what you are asking for is a ./configure command line option that allows one to exclude unit tests and thus the dependency on check from a build?
Stefan
- 5003. By Stefan Götz
-
Prevent test units from being compiled and run when the check library is not available
- added the Automake conditional HIP_UNITTESTS
- made the inclusion of the unit tests depend on HIP_UNITTESS in Makefile.am - 5004. By Stefan Götz
-
Documented how to unit test for an assertion to occur.
- 5005. By Stefan Götz
-
Documented the check library as another optional dependency
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
I think I addressed all concerns raised here. This branch currently merges into the trunk without conflicts. It's ready to go in, from my side.
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
Merge blocker #1:
On 10/19/2010 02:49 PM, Christof Mroz wrote:
> There should be an explicit statement about how to test static functions in
> HACKING, I think. Since tests routines are located in separate files, these
> can't be called so easily. You may say that testing a non-public interface is
> against the "spirit" of unit testing, but a quick google search reveals its
> demand nevertheless.
> I can think of at least one concrete coverage in HIPL that can't trivially be
> "induced" by high-level functions even though the function is referentially
> transparent by nature.
>
> Either way, there should be a consistent and documented policy.
I agree. I will take care of that today or tomorrow.
Merge blocker #2:
(Rene Hummen:) Makefile.am unnecessarily adds the linker option '-lcheck', needs to be removed.
- 5006. By Stefan Götz
-
Clean up automake configuration for unit tests:
- removed unnecessary explicit -lcheck switch in Makefile.am
- used the correct configure variable to decide whether libcheck was found and to automatically generate the -lcheck switchThis fixes merge blocker #2
- 5007. By Stefan Götz
-
Documented a policy and a how-to on testing static functions.
- added the policy
- clarified the test organisation to be able to refer to it in the policyThis should remove merge blocker #1.
Stefan Götz (stefan.goetz-deactivatedaccount) wrote : | # |
Revisions 5006 and 5007 address all remaining merge blockers. The branch applies cleanly to the current trunk revision 5028. If there aren't any other issues, I herewith re-request merging this branch to trunk :-)
Miika Komu (miika-iki) wrote : | # |
Seems good to me.
- 5008. By Stefan Götz
-
Handle absence of libcheck correctly
Symptom of bug: The configure script detects the absence of libcheck but running 'make check' tries to compile the unit tests nevertheless and fails.
Cause of bug: configure.ac converts the result of the configure-time test for libcheck into a make-visible variable by testing whether the auto-generated test variable ac_cv_lib_check_suite_ create is empty, based on the assumption that is empty if libcheck is absent, and that it is non-empty if libcheck is present. However, ac_cv_lib_ check_suite_ create is never empty and contains either 'no' or 'yes'.
Bug fix: Test for the variable value 'yes' instead of any non-empty value to determine that libcheck is present.
Preview Diff
1 | === modified file 'INSTALL' |
2 | --- INSTALL 2010-10-11 17:00:58 +0000 |
3 | +++ INSTALL 2010-10-20 08:48:52 +0000 |
4 | @@ -27,7 +27,8 @@ |
5 | Make and gcc. openssl, iptables, libcap and libconfig are required complete |
6 | with development headers. For Perl, Socket6, IO::Socket::INET6, Net::IP and |
7 | Net::DNS modules are required. You can optionally install xmlto to |
8 | -build the HOWTO and doxygen to build the code documentation. |
9 | +build the HOWTO and doxygen to build the code documentation. Installing the |
10 | +optional check library (http://check.sourceforge.net/) enables unit tests. |
11 | |
12 | On Ubuntu, the following command(s) should solve the dependencies: |
13 | |
14 | @@ -35,7 +36,7 @@ |
15 | iptables-dev libcap-dev libnet-ip-perl libnet-dns-perl \ |
16 | libsocket6-perl libio-socket-inet6-perl |
17 | |
18 | - Optionally: aptitude install pax miredo bzr xmlto doxygen |
19 | + Optionally: aptitude install pax miredo bzr xmlto doxygen check |
20 | |
21 | On Fedora, the following command(s) should solve the dependencies: |
22 | |
23 | |
24 | === modified file 'Makefile.am' |
25 | --- Makefile.am 2010-10-01 17:59:52 +0000 |
26 | +++ Makefile.am 2010-10-20 08:48:52 +0000 |
27 | @@ -38,6 +38,16 @@ |
28 | lib_LTLIBRARIES += lib/modularization/libmod.la |
29 | |
30 | |
31 | +### tests ### |
32 | +TESTS = |
33 | +check_PROGRAMS = |
34 | + |
35 | +if HIP_UNITTESTS |
36 | +TESTS += check_lib_core |
37 | +check_PROGRAMS += check_lib_core |
38 | +endif |
39 | + |
40 | + |
41 | ### source declarations ### |
42 | |
43 | test_auth_performance_SOURCES = test/auth_performance.c |
44 | @@ -151,6 +161,10 @@ |
45 | |
46 | lib_modularization_libmod_la_SOURCES = lib/modularization/lmod.c |
47 | |
48 | +check_lib_core_SOURCES = test/check_lib_core.c \ |
49 | + test/lib/core/hit.c \ |
50 | + test/lib/core/straddr.c |
51 | + |
52 | # Initialize LDADD lists empty, because modules might add entries to LDADD. The |
53 | # module LDADDs need to be included before the standard LDADDs, because modules |
54 | # can depend on the core code. |
55 | @@ -166,6 +180,7 @@ |
56 | |
57 | ### library dependencies ### |
58 | |
59 | +check_lib_core_LDADD = lib/core/libhipcore.la |
60 | firewall_hipfw_LDADD += lib/core/libhipcore.la |
61 | hipd_hipd_LDADD += lib/modularization/libmod.la \ |
62 | lib/core/libhipcore.la |
63 | |
64 | === modified file 'configure.ac' |
65 | --- configure.ac 2010-10-05 16:45:22 +0000 |
66 | +++ configure.ac 2010-10-20 08:48:52 +0000 |
67 | @@ -39,6 +39,10 @@ |
68 | AH_TEMPLATE(HAVE_EC_CRYPTO, [Defined to 1 if ec is enabled.]),) |
69 | # We need the math lib in the registration extension. |
70 | AC_CHECK_LIB(m, pow,, AC_MSG_ERROR(Math lib not found)) |
71 | +# The unit tests depend on 'check' (http://check.sourceforge.net/) |
72 | +AC_CHECK_LIB(check, suite_create,, |
73 | + AC_MSG_WARN(libcheck (http://check.sourceforge.net/) not found: HIPL unit tests are not available),) |
74 | +AM_CONDITIONAL(HIP_UNITTESTS, test x"$ac_cv_lib_check_suite_create" = xyes) |
75 | |
76 | # If no --prefix option is passed to configure, $prefix is empty. But we want |
77 | # to expand $sysconfdir, which defaults to ${prefix}/etc. So set $prefix. |
78 | |
79 | === modified file 'doc/HACKING' |
80 | --- doc/HACKING 2010-10-18 16:16:10 +0000 |
81 | +++ doc/HACKING 2010-10-20 08:48:52 +0000 |
82 | @@ -684,6 +684,122 @@ |
83 | |
84 | Modify the numbers depending on your hardware to suit your needs. |
85 | |
86 | +UNIT TESTS |
87 | +========== |
88 | +You can (and are recommended to) use unit testing to improve code quality. |
89 | + |
90 | +The command |
91 | + |
92 | +#> make check |
93 | + |
94 | +runs the existing unit tests. |
95 | +HIPL uses the check framework (http://check.sourceforge.net/) for unit tests. |
96 | +It is available in source and binary form, e.g. the Debian package 'check'. |
97 | + |
98 | +What To Test? |
99 | +------------- |
100 | +Unit tests check whether implementations conform to their specification (i.e., |
101 | +their documentation). Some rules of thumb for writing tests are: |
102 | + |
103 | +- test the documented aspects of a function: does it behave as expected? does |
104 | + it catch invalid input as documented? |
105 | + |
106 | +- test undocumented aspects of a function: how does it handle invalid input |
107 | + (e.g., NULL pointers, empty strings, negative integers) that the compiler |
108 | + cannot catch? |
109 | + |
110 | +Test coverage: At the very least, tests should ensure that a function can |
111 | +handle valid and invalid input as documented. Ideally, they also test corner |
112 | +cases and unusual input and state. Obviously, unit tests are limited |
113 | +in testing whether a function fully behaves as documented because that can be |
114 | +very complex, slow, or impossible on a local system. |
115 | + |
116 | +Unit Tests Organizition |
117 | +----------------------- |
118 | +The unit tests are organized as follows: |
119 | + |
120 | +- all test code resides in the test directory. The directories below test |
121 | + mirror those in the main hipl directory to organize the tests in the same |
122 | + layout. |
123 | + |
124 | +- test programs: There is one test program per hipl component. At the time of |
125 | + this writing, the only test program is check_lib_core for all tests that |
126 | + relate to the files from lib/core. By convention, each test program is |
127 | + called check_<component name> where <component name> is the name of the |
128 | + directory that contains the component. Test programs are defined in |
129 | + Makefile.am. |
130 | + |
131 | +- test suites: Each test program consists of several test suites, one for each |
132 | + implementation file to test. By convention, the tests of the test suite for |
133 | + lib/core/hit.c are contained in test/lib/core/hit.c, which exports them as a |
134 | + suite to the check_lib_core test program. |
135 | + |
136 | +- tests: Each test suite consists of one or more tests that test a specific |
137 | + aspect of a function from the implementation. For example, the test function |
138 | + test_hip_convert_hit_to_str_valid() in test/lib/core/hit.c tests whether the |
139 | + function hip_convert_hit_to_str() from lib/core/hit.c works as expected with |
140 | + valid input parameters. By convention, the test function is called |
141 | + test_<function under test>_<test aspect>(). |
142 | + |
143 | +Adding Unit Tests |
144 | +----------------- |
145 | +To add a unit test for the function foo() in the file hipd/bar.c, follow these |
146 | +steps: |
147 | + |
148 | +* If it does not exist, create a test program for the hipd component: |
149 | + - copy an existing test program implementation (test/check_*.c) to |
150 | + test/check_hipd.c |
151 | + - remove the existing "extern Suite" and "srunner_add_suite()" statements |
152 | + - in Makefile.am: |
153 | + * add the check_hipd test program to TESTS |
154 | + * add the check_hipd test program to check_PROGRAMS |
155 | + * add the file test/check_hipd.c to check_hipd_SOURCES |
156 | + * add the check library from the check framework and all libraries your |
157 | + test program depends on (usually the same as the original component hipd) |
158 | + to check_hipd_LDADD |
159 | + - create the directory test/hipd for your test suites |
160 | + - re-run the automake tool chain |
161 | +* If it does not exist, create the test suite for the file hipd/bar.c: |
162 | + - copy an existing test suite (test/hipd/*.c) to test/hipd/bar.c. |
163 | + - in test/hipd/bar.c, make sure you include the test target's header file |
164 | + (#include "hipd/bar.h" in this example) |
165 | + - in test/hipd/bar.c, remove all tests and tcase_add_test() statements |
166 | + - in test/hipd/bar.c, adapt the name of the test suite in the invocation of |
167 | + suite_create() to "hipd/bar" |
168 | + - add test/hipd/bar.c to check_hipd_SOURCES in Makefile.am |
169 | + - re-run the automake tool chain |
170 | +* Create the test case: |
171 | + - implement the test function called test_foo_<test aspect>() in |
172 | + test/hipd/bar.c (see other test suites and the check documentation for |
173 | + details) |
174 | + - register the test function to the test suite by adding a corresponding |
175 | + tcase_add_test() function call to hipd_bar() in test/hipd/bar.c. |
176 | + If you'd like to test for an HIP_ASSERT() to occur, use |
177 | + tcase_add_exit_test(). |
178 | + - run "make check" |
179 | + |
180 | +Testing Static Functions |
181 | +------------------------ |
182 | +Usually, unit tests are not intended to apply to static functions and doing so |
183 | +is difficult because these functions are not accessible by test programs |
184 | +from outside of the files that define them. However, static functions |
185 | +are commonplace in HIPL and often form relevant test targets. Developers are |
186 | +thus encouraged to apply unit tests to static functions as well as to |
187 | +non-static functions with external visibility. |
188 | + |
189 | +There are two approaches to making static functions visible within the test |
190 | +programs (adopting the names from the organization example above): |
191 | + |
192 | +a) Instead of including hipd/bar.h, include the implementation file hipd/bar.c |
193 | + directly. While not an elegant solution, it is the simplest approach that |
194 | + should work in most cases. |
195 | + |
196 | +b) For the test target static foo() create the wrapper foo_test_wrapper() that |
197 | + is not static. Declare all such wrappers in hipd/bar_test_wrappers.h and |
198 | + include that file instead of hipd/bar.h. While more effort, this is cleaner |
199 | + than solution a) and should work where a) prooves problematic. |
200 | + |
201 | + |
202 | DEBUGGING |
203 | ========= |
204 | |
205 | |
206 | === added file 'test/README.UNITTESTS' |
207 | --- test/README.UNITTESTS 1970-01-01 00:00:00 +0000 |
208 | +++ test/README.UNITTESTS 2010-10-20 08:48:52 +0000 |
209 | @@ -0,0 +1,5 @@ |
210 | +Run the unit tests via |
211 | + |
212 | +#> make check |
213 | + |
214 | +For more information, see doc/HACKING |
215 | |
216 | === added file 'test/check_lib_core.c' |
217 | --- test/check_lib_core.c 1970-01-01 00:00:00 +0000 |
218 | +++ test/check_lib_core.c 2010-10-20 08:48:52 +0000 |
219 | @@ -0,0 +1,55 @@ |
220 | +/* |
221 | + * Copyright (c) 2010 Aalto University and RWTH Aachen University. |
222 | + * |
223 | + * Permission is hereby granted, free of charge, to any person |
224 | + * obtaining a copy of this software and associated documentation |
225 | + * files (the "Software"), to deal in the Software without |
226 | + * restriction, including without limitation the rights to use, |
227 | + * copy, modify, merge, publish, distribute, sublicense, and/or sell |
228 | + * copies of the Software, and to permit persons to whom the |
229 | + * Software is furnished to do so, subject to the following |
230 | + * conditions: |
231 | + * |
232 | + * The above copyright notice and this permission notice shall be |
233 | + * included in all copies or substantial portions of the Software. |
234 | + * |
235 | + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, |
236 | + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES |
237 | + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND |
238 | + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT |
239 | + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, |
240 | + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING |
241 | + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR |
242 | + * OTHER DEALINGS IN THE SOFTWARE. |
243 | + */ |
244 | + |
245 | +/** |
246 | + * @file |
247 | + * @brief Unit tests of lib/core (see doc/HACKING on unit tests). |
248 | + * @author Stefan Goetz <stefan.goetz@cs.rwth-aachen.de> |
249 | + */ |
250 | +#include <stdlib.h> |
251 | +#include <check.h> |
252 | + |
253 | +// Import test suite functions from their respective C files via forward |
254 | +// declarations. |
255 | +// Since each test C file exports only one such function which is only used |
256 | +// right here, a dedicated header file for each of them adds unnecessary file |
257 | +// clutter in this particular case of unit tests. |
258 | +// Do not adopt this HFAS (header-file-avoidance-scheme) (TM) in HIPL production |
259 | +// code as header files are generally a good idea, just not here. |
260 | +extern Suite *lib_core_hit (void); |
261 | +extern Suite *lib_core_straddr (void); |
262 | + |
263 | +int main(void) |
264 | +{ |
265 | + int number_failed; |
266 | + SRunner *sr = srunner_create(lib_core_hit()); |
267 | + srunner_add_suite(sr, lib_core_straddr()); |
268 | + |
269 | + srunner_run_all(sr, CK_NORMAL); |
270 | + number_failed = srunner_ntests_failed(sr); |
271 | + srunner_free(sr); |
272 | + return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE; |
273 | +} |
274 | + |
275 | |
276 | === added directory 'test/lib' |
277 | === added directory 'test/lib/core' |
278 | === added file 'test/lib/core/hit.c' |
279 | --- test/lib/core/hit.c 1970-01-01 00:00:00 +0000 |
280 | +++ test/lib/core/hit.c 2010-10-20 08:48:52 +0000 |
281 | @@ -0,0 +1,195 @@ |
282 | +/* |
283 | + * Copyright (c) 2010 Aalto University and RWTH Aachen University. |
284 | + * |
285 | + * Permission is hereby granted, free of charge, to any person |
286 | + * obtaining a copy of this software and associated documentation |
287 | + * files (the "Software"), to deal in the Software without |
288 | + * restriction, including without limitation the rights to use, |
289 | + * copy, modify, merge, publish, distribute, sublicense, and/or sell |
290 | + * copies of the Software, and to permit persons to whom the |
291 | + * Software is furnished to do so, subject to the following |
292 | + * conditions: |
293 | + * |
294 | + * The above copyright notice and this permission notice shall be |
295 | + * included in all copies or substantial portions of the Software. |
296 | + * |
297 | + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, |
298 | + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES |
299 | + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND |
300 | + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT |
301 | + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, |
302 | + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING |
303 | + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR |
304 | + * OTHER DEALINGS IN THE SOFTWARE. |
305 | + */ |
306 | + |
307 | +/** |
308 | + * @file |
309 | + * @author Stefan Goetz <stefan.goetz@cs.rwth-aachen.de> |
310 | + */ |
311 | +#include <check.h> |
312 | +#include <stdlib.h> |
313 | +#include <string.h> |
314 | +#include <stdio.h> |
315 | +#include "lib/core/hit.h" |
316 | + |
317 | +START_TEST(test_hip_convert_hit_to_str_valid) |
318 | +{ |
319 | + char buf[64]; |
320 | + hip_hit_t hit; |
321 | + fail_unless(hip_convert_hit_to_str(&hit, "", buf) == 0, NULL); |
322 | +} |
323 | +END_TEST |
324 | + |
325 | +START_TEST(test_hip_convert_hit_to_str_null_hit) |
326 | +{ |
327 | + char buf[64]; |
328 | + hip_convert_hit_to_str(NULL, "", buf); |
329 | +} |
330 | +END_TEST |
331 | + |
332 | +START_TEST(test_hip_convert_hit_to_str_null_buf) |
333 | +{ |
334 | + hip_hit_t hit; |
335 | + fail_unless(hip_convert_hit_to_str(&hit, "", NULL) == 1, NULL); |
336 | +} |
337 | +END_TEST |
338 | + |
339 | +START_TEST(test_hip_convert_hit_to_str_null_prefix) |
340 | +{ |
341 | + char buf[64]; |
342 | + hip_hit_t hit; |
343 | + fail_unless(hip_convert_hit_to_str(&hit, NULL, buf) == 0, NULL); |
344 | +} |
345 | +END_TEST |
346 | + |
347 | +START_TEST(test_hip_convert_hit_to_str_bounds) |
348 | +{ |
349 | + const char suffix[] = "SFX"; |
350 | + const unsigned int BEFORE_LEN = 30; |
351 | + const unsigned int HIT_LEN = 39; // 16 bytes -> 32 hex chars + 7 ':'s |
352 | + const unsigned int SUFFIX_LEN = sizeof(suffix); // includes null char |
353 | + const unsigned int AFTER_LEN = 30; |
354 | + char buf[BEFORE_LEN + HIT_LEN + SUFFIX_LEN + AFTER_LEN] = { 1 }; |
355 | + char ones[BEFORE_LEN + HIT_LEN + SUFFIX_LEN + AFTER_LEN] = { 1 }; |
356 | + hip_hit_t hit; |
357 | + memset(&hit.s6_addr, 0x22, sizeof(hit.s6_addr)); |
358 | + |
359 | + // write the HIT string into the middle of the buffer |
360 | + fail_unless(hip_convert_hit_to_str(&hit, suffix, buf + BEFORE_LEN) == 0, NULL); |
361 | + // is the buffer before the HIT untouched? |
362 | + fail_unless(memcmp(buf, ones, BEFORE_LEN) == 0, NULL); |
363 | + // is the first part of the HIT correct? |
364 | + fail_unless(*(buf + BEFORE_LEN) == '2', NULL); |
365 | + // is the last part of the HIT correct? |
366 | + fail_unless(*(buf + BEFORE_LEN + HIT_LEN - 1) == '2', NULL); |
367 | + // is the suffix correct including the terminating null character? |
368 | + fail_unless(memcmp(buf + BEFORE_LEN + HIT_LEN, suffix, SUFFIX_LEN) == 0, NULL); |
369 | + // is the buffer after the suffix untouched? |
370 | + fail_unless(memcmp(buf + BEFORE_LEN + HIT_LEN + SUFFIX_LEN, ones, AFTER_LEN) == 0, NULL); |
371 | +} |
372 | +END_TEST |
373 | + |
374 | +START_TEST(test_hip_hit_is_bigger_bigger) |
375 | +{ |
376 | + const hip_hit_t bigger = IN6ADDR_LOOPBACK_INIT; |
377 | + const hip_hit_t smaller = IN6ADDR_ANY_INIT; |
378 | + fail_unless(hip_hit_is_bigger(&bigger, &smaller) == 1, NULL); |
379 | +} |
380 | +END_TEST |
381 | + |
382 | +START_TEST(test_hip_hit_is_bigger_equal_smaller) |
383 | +{ |
384 | + const hip_hit_t bigger = IN6ADDR_LOOPBACK_INIT; |
385 | + const hip_hit_t smaller = IN6ADDR_ANY_INIT; |
386 | + fail_unless(hip_hit_is_bigger(&smaller, &bigger) == 0, NULL); |
387 | + fail_unless(hip_hit_is_bigger(&bigger, &bigger) == 0, NULL); |
388 | +} |
389 | +END_TEST |
390 | + |
391 | +START_TEST(test_hip_hit_is_bigger_first_null) |
392 | +{ |
393 | + hip_hit_t hit; |
394 | + hip_hit_is_bigger(NULL, &hit); |
395 | +} |
396 | +END_TEST |
397 | + |
398 | +START_TEST(test_hip_hit_is_bigger_second_null) |
399 | +{ |
400 | + hip_hit_t hit; |
401 | + hip_hit_is_bigger(&hit, NULL); |
402 | +} |
403 | +END_TEST |
404 | + |
405 | +START_TEST(test_hip_hit_are_equal_equality) |
406 | +{ |
407 | + const hip_hit_t hit1 = IN6ADDR_LOOPBACK_INIT; |
408 | + const hip_hit_t hit2 = IN6ADDR_LOOPBACK_INIT; |
409 | + fail_unless(hip_hit_are_equal(&hit1, &hit2) == 1, NULL); |
410 | +} |
411 | +END_TEST |
412 | + |
413 | +START_TEST(test_hip_hit_are_equal_inequality) |
414 | +{ |
415 | + const hip_hit_t bigger = IN6ADDR_LOOPBACK_INIT; |
416 | + const hip_hit_t smaller = IN6ADDR_ANY_INIT; |
417 | + fail_unless(hip_hit_are_equal(&bigger, &smaller) == 1, NULL); |
418 | +} |
419 | +END_TEST |
420 | + |
421 | +START_TEST(test_hip_hit_are_equal_first_null) |
422 | +{ |
423 | + hip_hit_t hit; |
424 | + hip_hit_are_equal(NULL, &hit); |
425 | +} |
426 | +END_TEST |
427 | + |
428 | +START_TEST(test_hip_hit_are_equal_second_null) |
429 | +{ |
430 | + hip_hit_t hit; |
431 | + hip_hit_are_equal(&hit, NULL); |
432 | +} |
433 | +END_TEST |
434 | + |
435 | +START_TEST(test_hip_hash_hit_valid) |
436 | +{ |
437 | + const hip_hit_t hit = IN6ADDR_ANY_INIT; |
438 | + hip_hash_hit(&hit); |
439 | +} |
440 | +END_TEST |
441 | + |
442 | +START_TEST(test_hip_hash_hit_null) |
443 | +{ |
444 | + hip_hash_hit(NULL); |
445 | +} |
446 | +END_TEST |
447 | + |
448 | +// For unknown reasons, this file does not compile with the following, |
449 | +// seemingly useless forward declaration |
450 | +Suite *lib_core_hit(void); |
451 | + |
452 | +Suite *lib_core_hit(void) |
453 | +{ |
454 | + Suite *s = suite_create("lib/core/hit"); |
455 | + |
456 | + TCase *tc_core = tcase_create("Core"); |
457 | + tcase_add_test(tc_core, test_hip_convert_hit_to_str_valid); |
458 | + tcase_add_exit_test(tc_core, test_hip_convert_hit_to_str_null_hit, 1); |
459 | + tcase_add_test(tc_core, test_hip_convert_hit_to_str_null_buf); |
460 | + tcase_add_test(tc_core, test_hip_convert_hit_to_str_null_prefix); |
461 | + tcase_add_test(tc_core, test_hip_convert_hit_to_str_bounds); |
462 | + tcase_add_test(tc_core, test_hip_hit_is_bigger_bigger); |
463 | + tcase_add_test(tc_core, test_hip_hit_is_bigger_equal_smaller); |
464 | + tcase_add_exit_test(tc_core, test_hip_hit_is_bigger_first_null, 1); |
465 | + tcase_add_exit_test(tc_core, test_hip_hit_is_bigger_second_null, 1); |
466 | + tcase_add_test(tc_core, test_hip_hit_are_equal_equality); |
467 | + tcase_add_test(tc_core, test_hip_hit_are_equal_inequality); |
468 | + tcase_add_exit_test(tc_core, test_hip_hit_are_equal_first_null, 1); |
469 | + tcase_add_exit_test(tc_core, test_hip_hit_are_equal_second_null, 1); |
470 | + tcase_add_test(tc_core, test_hip_hash_hit_valid); |
471 | + tcase_add_exit_test(tc_core, test_hip_hash_hit_null, 1); |
472 | + suite_add_tcase(s, tc_core); |
473 | + |
474 | + return s; |
475 | +} |
476 | + |
477 | |
478 | === added file 'test/lib/core/straddr.c' |
479 | --- test/lib/core/straddr.c 1970-01-01 00:00:00 +0000 |
480 | +++ test/lib/core/straddr.c 2010-10-20 08:48:52 +0000 |
481 | @@ -0,0 +1,195 @@ |
482 | +/* |
483 | + * Copyright (c) 2010 Aalto University and RWTH Aachen University. |
484 | + * |
485 | + * Permission is hereby granted, free of charge, to any person |
486 | + * obtaining a copy of this software and associated documentation |
487 | + * files (the "Software"), to deal in the Software without |
488 | + * restriction, including without limitation the rights to use, |
489 | + * copy, modify, merge, publish, distribute, sublicense, and/or sell |
490 | + * copies of the Software, and to permit persons to whom the |
491 | + * Software is furnished to do so, subject to the following |
492 | + * conditions: |
493 | + * |
494 | + * The above copyright notice and this permission notice shall be |
495 | + * included in all copies or substantial portions of the Software. |
496 | + * |
497 | + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, |
498 | + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES |
499 | + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND |
500 | + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT |
501 | + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, |
502 | + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING |
503 | + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR |
504 | + * OTHER DEALINGS IN THE SOFTWARE. |
505 | + */ |
506 | + |
507 | +/** |
508 | + * @file |
509 | + * @author Stefan Goetz <stefan.goetz@cs.rwth-aachen.de> |
510 | + */ |
511 | +#include <check.h> |
512 | +#include <stdlib.h> // free() |
513 | +#include "lib/core/straddr.h" |
514 | + |
515 | +START_TEST(test_convert_string_to_address_v4_valid) |
516 | +{ |
517 | + const char *str = "127.0.0.1"; |
518 | + struct in_addr ip; |
519 | + |
520 | + fail_unless(convert_string_to_address_v4(str, &ip) == 0, NULL); |
521 | +} |
522 | +END_TEST |
523 | + |
524 | +START_TEST(test_convert_string_to_address_v4_null_str) |
525 | +{ |
526 | + struct in_addr ip; |
527 | + |
528 | + fail_unless(convert_string_to_address_v4(NULL, &ip) < 0, NULL); |
529 | +} |
530 | +END_TEST |
531 | + |
532 | +START_TEST(test_convert_string_to_address_v4_null_addr) |
533 | +{ |
534 | + const char *str = "127.0.0.1"; |
535 | + |
536 | + fail_unless(convert_string_to_address_v4(str, NULL) < 0, NULL); |
537 | +} |
538 | +END_TEST |
539 | + |
540 | +START_TEST(test_convert_string_to_address_v4_invalid) |
541 | +{ |
542 | + const char *str = " 127.0.0.1"; |
543 | + struct in_addr ip; |
544 | + |
545 | + fail_unless(convert_string_to_address_v4(str, &ip) < 0, NULL); |
546 | +} |
547 | +END_TEST |
548 | + |
549 | +START_TEST(test_convert_string_to_address_valid) |
550 | +{ |
551 | + const char *str = "fe80::215:58ff:fe29:9c36"; |
552 | + struct in6_addr ip; |
553 | + |
554 | + fail_unless(convert_string_to_address(str, &ip) == 0, NULL); |
555 | +} |
556 | +END_TEST |
557 | + |
558 | +START_TEST(test_convert_string_to_address_null_str) |
559 | +{ |
560 | + struct in6_addr ip; |
561 | + |
562 | + fail_unless(convert_string_to_address(NULL, &ip) < 0, NULL); |
563 | +} |
564 | +END_TEST |
565 | + |
566 | +START_TEST(test_convert_string_to_address_null_addr) |
567 | +{ |
568 | + const char *str = "fe80::215:58ff:fe29:9c36"; |
569 | + |
570 | + fail_unless(convert_string_to_address(str, NULL) < 0, NULL); |
571 | +} |
572 | +END_TEST |
573 | + |
574 | +START_TEST(test_convert_string_to_address_invalid) |
575 | +{ |
576 | + const char *str = " fe80::215:58ff:fe29:9c36"; |
577 | + struct in6_addr ip; |
578 | + |
579 | + fail_unless(convert_string_to_address(str, &ip) < 0, NULL); |
580 | +} |
581 | +END_TEST |
582 | + |
583 | +START_TEST(test_hip_string_to_lowercase_valid) |
584 | +{ |
585 | + char to[128] = { 1 }; |
586 | + char ones[128] = { 1 }; |
587 | + const char from[] = "TesT"; |
588 | + const size_t count = sizeof(from) - 1; |
589 | + const unsigned int offset = 32; |
590 | + |
591 | + fail_unless(hip_string_to_lowercase(to + offset, from, count) == 0, NULL); |
592 | + // was from correctly converted to lower case? |
593 | + fail_unless(memcmp(to + offset, "test", count) == 0, NULL); |
594 | + // is the beginning of to still intact? |
595 | + fail_unless(memcmp(to, ones, offset) == 0, NULL); |
596 | + // is the rest of to still intact? |
597 | + fail_unless(memcmp(to + offset + count, ones, offset) == 0, NULL); |
598 | +} |
599 | +END_TEST |
600 | + |
601 | +START_TEST(test_hip_string_is_digit_valid) |
602 | +{ |
603 | + fail_unless(hip_string_is_digit("123456789") == 0, NULL); |
604 | + fail_unless(hip_string_is_digit("abc") < 0, NULL); |
605 | +} |
606 | +END_TEST |
607 | + |
608 | +START_TEST(test_hip_string_is_digit_null) |
609 | +{ |
610 | + fail_unless(hip_string_is_digit(NULL) < 0, NULL); |
611 | +} |
612 | +END_TEST |
613 | + |
614 | +START_TEST(test_hip_string_is_digit_empty) |
615 | +{ |
616 | + fail_unless(hip_string_is_digit("") < 0, NULL); |
617 | +} |
618 | +END_TEST |
619 | + |
620 | +START_TEST(test_base64_encode_valid) |
621 | +{ |
622 | + const char b64[] = "VGVzdA=="; |
623 | + unsigned char buf[] = "Test"; |
624 | + unsigned int len = sizeof(buf) - 1; // do not include null character as per doc |
625 | + unsigned char *result = NULL; |
626 | + |
627 | + fail_unless((result = base64_encode(buf, len)) != NULL, NULL); |
628 | + fail_unless(strcmp((char*)result, b64) == 0, NULL); |
629 | + free(result); // note it's not documented that we need to free the returned memory |
630 | +} |
631 | +END_TEST |
632 | + |
633 | +START_TEST(test_base64_encode_null_buf) |
634 | +{ |
635 | + fail_unless(base64_encode(NULL, 42) == NULL, NULL); |
636 | +} |
637 | +END_TEST |
638 | + |
639 | +START_TEST(test_base64_encode_empty_buf) |
640 | +{ |
641 | + unsigned char buf[] = ""; |
642 | + unsigned char *result = NULL; |
643 | + |
644 | + fail_unless((result = base64_encode(buf, 0)) != NULL, NULL); |
645 | + fail_unless(strlen((char *)result) == 0, NULL); |
646 | +} |
647 | +END_TEST |
648 | + |
649 | +// For unknown reasons, this file does not compile with the following, |
650 | +// seemingly useless forward declaration |
651 | +Suite *lib_core_straddr(void); |
652 | + |
653 | +Suite *lib_core_straddr(void) |
654 | +{ |
655 | + Suite *s = suite_create("lib/core/straddr"); |
656 | + |
657 | + TCase *tc_core = tcase_create("Core"); |
658 | + tcase_add_test(tc_core, test_convert_string_to_address_v4_valid); |
659 | + tcase_add_test(tc_core, test_convert_string_to_address_v4_null_str); |
660 | + tcase_add_test(tc_core, test_convert_string_to_address_v4_null_addr); |
661 | + tcase_add_test(tc_core, test_convert_string_to_address_v4_invalid); |
662 | + tcase_add_test(tc_core, test_convert_string_to_address_valid); |
663 | + tcase_add_test(tc_core, test_convert_string_to_address_null_str); |
664 | + tcase_add_test(tc_core, test_convert_string_to_address_null_addr); |
665 | + tcase_add_test(tc_core, test_convert_string_to_address_invalid); |
666 | + tcase_add_test(tc_core, test_hip_string_to_lowercase_valid); |
667 | + tcase_add_test(tc_core, test_hip_string_is_digit_valid); |
668 | + tcase_add_test(tc_core, test_hip_string_is_digit_null); |
669 | + tcase_add_test(tc_core, test_hip_string_is_digit_empty); |
670 | + tcase_add_test(tc_core, test_base64_encode_valid); |
671 | + tcase_add_test(tc_core, test_base64_encode_null_buf); |
672 | + tcase_add_test(tc_core, test_base64_encode_empty_buf); |
673 | + suite_add_tcase(s, tc_core); |
674 | + |
675 | + return s; |
676 | +} |
Great! I think this is a welcome addition for everyone. While the changes seem fine to me, I would hope someone else reviews the code too.
What about testing old code? Would it be ok the abuse the unit test framework for benign system tests too? We could test the hipd through loopback using e.g. one of the ways described below:
a. fail_unless( hipd_main( LOOPBACK_ TEST_FLAG) , ..) hip_map_ id_to_addr( ..), ..) and fail_unless(ICMPv6 fails as in modules/ heartbeat/ hipd/heartbeat. c) hip_send_ i1(..), ..) and fail_unless( hip_receive_ r1(..), ..), etc
b. use a thread to kick hipd up, fail_unless(
c. fail_unless(