Skip to content
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

Test OpenWRT in CI #234

Merged
merged 8 commits into from
Sep 2, 2022
Merged

Test OpenWRT in CI #234

merged 8 commits into from
Sep 2, 2022

Conversation

aloisklink
Copy link
Contributor

Runs the OpenWRT tests in CI.

I had to slightly play around with some stuff to get OpenWRT tests working again.

Firstly, I had to add LD_LIBRARY_PATH="${sourceDir}/build/${presetName}/lib/ubox/lib", see ee360eb to get OpenWRT tests working properly, since even though we set RPATH on our tests, since the tests link to libuci, libuci is the one that needs RPATH to libubox.

This meant I had to modify the CI to use the presets listed in CMakePresets.json (see ci: use buildDir from CMake presets), then I had to add a preset for openwrt that set LD_LIBRARY_PATH (see ci: add test config for openwrt)

Then, in order to fix the tests, I needed to:

  • test: add __wrap_uwrt_init_context test lib
    The __wrap_uwrt_init_context test library can be used to wrap uwrt_init_context() so that it loads from the ./test/data/uci folder.

  • tests(hostapd): fix WITH_HOSTAPD_UCI tests
    The run_ap_process() function doesn't call kill_process() when WITH_HOSTAPD_UCI is defined.
    Additionally, no hostapd_conf_file is written, instead UCI handles writing the configuration file.

  • test: fix dnsmasq tests when using UCI mode
    When running dnsmasq in UCI mode:

    • The dnsmasq_conf_path property is ignored, as UCI decides where to store the configuration
    • kill_process() is not run when running run_dhcp_process()
    • signal_dhcp_process() does nothing

    Additionally, we need to make sure that dhcp_conf.bridge_prefix is correctly set, otherwise you'll get a bunch of undefined memory, and your tests will work 95% of the time, and fail 5% of the time. (I think it took me about an hour to debug this)

  • refactor: simplify uwrt_create_interface code
    Some simple refactoring, just to avoid constantly doing uwrt_set_property().

  • fix: set ifname in uwrt_create_interface()
    The ifname field wasn't being set in uwrt_create_interface, which was causing the uci_wrt.c tests to fail.

Currently, we're hardcoding the build directory `./build` when
building in CI, as the `cmake --install` command does not
support loading the build directory from a cmake-preset.

However, when running OpenWRT tests, the cmake-preset sets a
LD_LIBRARY_PATH that points to the build directory, so we
can no longer use `./build`.
Tests now work for the OpenWRT preset, even on standard Linux,
as long as the LD_LIBRARY_PATH env points towards the libubox.so
shared library file.
The __wrap_uwrt_init_context test library can be used to wrap
uwrt_init_context() so that it loads from the ./test/data/uci
folder.
The `run_ap_process()` function doesn't call `kill_process()`
when WITH_HOSTAPD_UCI is defined.

Additionally, no hostapd_conf_file is written, instead UCI handles
writing the configuration file.
When running dnsmasq in UCI mode:
  - The dnsmasq_conf_path property is ignored, as UCI
    decides where to store the configuration
  - `kill_process()` is not run when running `run_dhcp_process()`
  - `signal_dhcp_process()` does nothing

Additionally, we need to make sure that `dhcp_conf.bridge_prefix` is
correctly set, otherwise you'll get a bunch of undefined memory,
and your tests will work 95% of the time, and fail 5% of the time.
(I think it took me about an hour to debug this)
Add `uwrt_set_property()` calls into a loop.

This should also increase our code-coverage, since we'll
have less lines of code.
The ifname field wasn't being set in uwrt_create_interface,
which was causing the uci_wrt.c tests to fail.
@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #234 (e920668) into main (af591bc) will decrease coverage by 0.98%.
The diff coverage is 90.47%.

@@            Coverage Diff             @@
##             main     #234      +/-   ##
==========================================
- Coverage   45.63%   44.64%   -0.99%     
==========================================
  Files         107      110       +3     
  Lines       14953    16393    +1440     
==========================================
+ Hits         6824     7319     +495     
- Misses       8129     9074     +945     
Impacted Files Coverage Δ
src/utils/uci_wrt.c 24.95% <82.60%> (ø)
tests/ap/test_hostapd.c 94.91% <100.00%> (+1.81%) ⬆️
tests/dhcp/test_dnsmasq.c 93.50% <100.00%> (+0.04%) ⬆️
tests/utils/__wrap_uwrt_init_context.c 100.00% <100.00%> (ø)
src/dhcp/dnsmasq.c 64.04% <0.00%> (-3.23%) ⬇️
src/firewall/firewall_service.c 2.61% <0.00%> (-1.23%) ⬇️
tests/utils/test_uci_wrt.c 100.00% <0.00%> (ø)
src/utils/iface.c 19.78% <0.00%> (+1.59%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@mereacre mereacre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work.

@mereacre mereacre merged commit d7c726f into main Sep 2, 2022
@mereacre mereacre deleted the test-openwrt branch September 2, 2022 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants