-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improves test_edgesec.c
#453
Conversation
Use `#include <threads.h>` (aka ISO C11 standard threads) instead of `#include <pthreads.h>` in `test_edgesec.c` Using C11 standard threads caught a double-free error, so it's already better!
Currently, the eloop threads are never actually stopped. We call `edge_eloop_terminate()`, but this is not threadsafe. To fix this, we can use a threadsafe atomic from `<stdatomic.h>`, which is constantly checked by eloop, which then automatically closes the eloop when the atomic is set. This also allows us to call `thrd_join()` on the threads and check their return code.
Currently, in `test_edgesec`, if the AP thread has an error, `fail_msg()` is called, which normally fails the CMocka test with the given error message. However, [CMocka **is not thread safe**][1]. Instead, we can make the AP Thread store the error message in a buffer, and call `thrd_exit(EXIT_FAILURE)` to exit the thread with an error exit code. We can then check for this thread exitcode later on, when we call `thrd_join()`, e.g. in `teardown_edgesec_test()`. [1]: https://api.cmocka.org/
Add a test that tests whether the AP server thread fails correctly. Previously, our test code wasn't correctly failing on errors. This should also help fix flakey code-coverage results in `test_edgesec.c`, see #435. This test is pretty slow, as we need to wait 10 seconds for `writeread_domain_data_str()` to timeout. In the future, we should find a way to speed up this test (maybe by using a different function to send data to the AP server socket). Fixes: #435
GitHub Actions is a bit slow, and so the test sometimes takes more than 15 seconds to run.
Codecov Report
@@ Coverage Diff @@
## main #453 +/- ##
==========================================
+ Coverage 53.08% 53.24% +0.16%
==========================================
Files 144 144
Lines 19871 19937 +66
==========================================
+ Hits 10548 10616 +68
+ Misses 9323 9321 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Instead of using `writeread_domain_data_str`, and waiting for it to timeout due to a lack of a response, we can just manually create a socket and use `write_domain_data_s()`, to avoid even trying to read a response. This causes our test to be much faster (5x faster).
When polling for the server in `test_edgesec`, poll every 100µs instead of only once every 1s. This speeds up the `test_edgesec` test by about 20000% (200x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Indeed we will have to replace the timeouts eventually.
Yah, even 1 second is quite a lot when I'm running the tests 1000s of times when Plus, think about all of the CO2 we'd save by using less of Microsoft's/GitHub's server time 🤣 !! |
Improves
test_edgesec.c
by making the following changes:test(edgesec): use
threads.h
in test_edgesecUse
#include <threads.h>
(aka ISO C11 standard threads) instead of#include <pthreads.h>
intest_edgesec.c
Using C11 standard threads caught a double-free error, so it's already
better!
test(edgesec): stop eloop threads gracefully
Currently, the eloop threads are never actually stopped. We call
edge_eloop_terminate()
, but this is not thread-safe.To fix this, we can use a threadsafe atomic from
<stdatomic.h>
, which is constantly checked by eloop, which then automatically closes the eloop when the atomic is set.This also allows us to call
thrd_join()
on the threads and check their return code.Basically, before this PR, calling
edge_eloop_terminate()
from the main test thread did nothing.test(edgesec): store error msg from ap thread
Currently, in
test_edgesec
, if the AP thread has an error,fail_msg()
is called, which normally fails the CMocka test with the given error message. However, CMocka is not thread safe.Instead, we can make the AP Thread store the error message in a buffer, and call
thrd_exit(EXIT_FAILURE)
to exit the thread with an error exit code.We can then check for this thread exitcode later on, when we call
thrd_join()
, e.g. inteardown_edgesec_test()
.test(edgesec): test ap server thread failures
Add a test that tests whether the AP server thread fails correctly.
Previously, our test code wasn't correctly failing on errors.
This should also help fix flakey code-coverage results in
test_edgesec.c
, seetests/test_edgesec.c
code-coverage is inconsistent #435.This test is pretty slow, as we need to wait 10 seconds for
writeread_domain_data_str()
to timeout. In the future, we should find a way to speed up this test (maybe by using a different function to send data to the AP server socket).@mereacre, if you've got any idea how we can easily replace this line, without having to wait 10 seconds for a timeout, let me know!edgesec/tests/test_edgesec.c
Lines 376 to 381 in 43f5b52