From 12cfcb5aedc0df4b23cd37e92b9d3d905e245469 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Wed, 18 Jan 2023 12:54:06 +0100 Subject: [PATCH 1/3] fix(mdns): Make unit test executable with pytest --- .github/workflows/target-test.yml | 72 +++++++------------ ci/clean_build_artifacts.sh | 2 +- components/mdns/examples/pytest_mdns.py | 9 ++- .../mdns/tests/unit_test/CMakeLists.txt | 9 ++- .../mdns/tests/unit_test/main/CMakeLists.txt | 5 ++ .../tests/unit_test/{ => main}/test_mdns.c | 40 +++++++++-- .../mdns/tests/unit_test/pytest_mdns.py | 8 +++ .../mdns/tests/unit_test/sdkconfig.defaults | 2 + 8 files changed, 88 insertions(+), 59 deletions(-) create mode 100644 components/mdns/tests/unit_test/main/CMakeLists.txt rename components/mdns/tests/unit_test/{ => main}/test_mdns.c (86%) create mode 100644 components/mdns/tests/unit_test/pytest_mdns.py create mode 100644 components/mdns/tests/unit_test/sdkconfig.defaults diff --git a/.github/workflows/target-test.yml b/.github/workflows/target-test.yml index adc64de0e..ab36415eb 100644 --- a/.github/workflows/target-test.yml +++ b/.github/workflows/target-test.yml @@ -62,48 +62,30 @@ jobs: matrix: idf_ver: ["latest"] idf_target: ["esp32", "esp32s2", "esp32c3"] - config: ["eth_custom_netif", "eth_def", "eth_no_ipv6", "eth_socket"] + test: [ { app: example, path: "components/mdns/examples" }, { app: unit_test, path: "components/mdns/tests/unit_test" } ] runs-on: ubuntu-20.04 container: espressif/idf:${{ matrix.idf_ver }} - env: - TEST_DIR: components/mdns/examples steps: - name: Checkout esp-protocols uses: actions/checkout@v3 with: submodules: recursive - - name: Build ${{ matrix.example }} with IDF-${{ matrix.idf_ver }} for ${{ matrix.idf_target }} for ${{ matrix.config }} - env: - IDF_TARGET: ${{ matrix.idf_target }} + - name: Build ${{ matrix.test.app }} with IDF-${{ matrix.idf_ver }} for ${{ matrix.idf_target }} shell: bash - working-directory: ${{ env.TEST_DIR }} + working-directory: ${{ matrix.test.path }} run: | + ${IDF_PATH}/install.sh --enable-pytest . ${IDF_PATH}/export.sh - rm -rf sdkconfig sdkconfig.defaults build build_${{ matrix.config }} - cat sdkconfig.ci.${{ matrix.config }} >> sdkconfig.defaults - idf.py set-target ${{ matrix.idf_target }} - idf.py build - mv build build_${{ matrix.config }} - - name: Merge binaries with IDF-${{ matrix.idf_ver }} for ${{ matrix.config }} - working-directory: ${{ env.TEST_DIR }} - env: - IDF_TARGET: ${{ matrix.idf_target }} - shell: bash - run: | - . ${IDF_PATH}/export.sh - cd build_${{ matrix.config }} - esptool.py --chip ${{ matrix.idf_target }} merge_bin --fill-flash-size 4MB -o flash_image.bin @flash_args + python $IDF_PATH/tools/ci/ci_build_apps.py . --target ${{ matrix.idf_target }} -vv --preserve-all --pytest-app + for dir in `ls -d build_*`; do + $GITHUB_WORKSPACE/ci/clean_build_artifacts.sh `pwd`/$dir + zip -qur artifacts.zip $dir + done - uses: actions/upload-artifact@v3 + if: ${{ matrix.idf_target }} == "esp32" with: - name: examples_app_bin_${{ matrix.idf_target }}_${{ matrix.idf_ver }}_${{ matrix.config }} - path: | - ${{ env.TEST_DIR }}/build_${{ matrix.config }}/bootloader/bootloader.bin - ${{ env.TEST_DIR }}/build_${{ matrix.config }}/partition_table/partition-table.bin - ${{ env.TEST_DIR }}/build_${{ matrix.config }}/*.bin - ${{ env.TEST_DIR }}/build_${{ matrix.config }}/*.elf - ${{ env.TEST_DIR }}/build_${{ matrix.config }}/flasher_args.json - ${{ env.TEST_DIR }}/build_${{ matrix.config }}/config/sdkconfig.h - ${{ env.TEST_DIR }}/build_${{ matrix.config }}/config/sdkconfig.json + name: mdns_bin_${{ matrix.idf_target }}_${{ matrix.idf_ver }}_${{ matrix.test.app }} + path: ${{ matrix.test.path }}/artifacts.zip if-no-files-found: error build_asio: @@ -240,14 +222,12 @@ jobs: matrix: idf_ver: ["latest"] idf_target: ["esp32"] - config: ["eth_custom_netif", "eth_def", "eth_no_ipv6", "eth_socket"] - name: Run mDNS Example Test on target + test: [ { app: example, path: "components/mdns/examples" }, { app: unit_test, path: "components/mdns/tests/unit_test" } ] + name: Run mDNS target tests needs: build_mdns runs-on: - self-hosted - ESP32-ETHERNET-KIT - env: - TEST_DIR: components/mdns/examples # Skip running on forks since it won't have access to secrets if: github.repository == 'espressif/esp-protocols' steps: @@ -256,28 +236,27 @@ jobs: - uses: actions/checkout@v3 - uses: actions/download-artifact@v3 with: - name: examples_app_bin_${{ matrix.idf_target }}_${{ matrix.idf_ver }}_${{ matrix.config }} - path: ${{ env.TEST_DIR }}/build_${{ matrix.config }} + name: mdns_bin_${{ matrix.idf_target }}_${{ matrix.idf_ver }}_${{ matrix.test.app }} + path: ${{ matrix.test.path }}/ci/ - name: Install Python packages env: PIP_EXTRA_INDEX_URL: "https://www.piwheels.org/simple" run: | sudo apt-get install -y dnsutils - - name: Download Example Test to target ${{ matrix.config }} - run: | - python -m esptool --chip ${{ matrix.idf_target }} write_flash 0x0 components/mdns/examples/build_${{ matrix.config }}/flash_image.bin - - name: Run Example Test on target ${{ matrix.config }} - working-directory: components/mdns/examples + - name: Run ${{ matrix.test.app }} application on ${{ matrix.idf_target }} + working-directory: ${{ matrix.test.path }} run: | + unzip ci/artifacts.zip -d ci + for dir in `ls -d ci/build_*`; do rm -rf build sdkconfig.defaults - mv build_${{ matrix.config }} build - cat sdkconfig.ci.${{ matrix.config }} >> sdkconfig.defaults - python -m pytest --log-cli-level DEBUG --junit-xml=./examples_results_${{ matrix.idf_target }}_${{ matrix.idf_ver }}_${{ matrix.config }}.xml --target=${{ matrix.idf_target }} + mv $dir build + python -m pytest --log-cli-level DEBUG --junit-xml=./results_${{ matrix.test.app }}_${{ matrix.idf_target }}_${{ matrix.idf_ver }}_${dir#"ci/build_"}.xml --target=${{ matrix.idf_target }} + done - uses: actions/upload-artifact@v3 if: always() with: - name: examples_results_${{ matrix.idf_target }}_${{ matrix.idf_ver }}_${{ matrix.config }} - path: ${{ env.TEST_DIR }}/*.xml + name: results_${{ matrix.test.app }}_${{ matrix.idf_target }}_${{ matrix.idf_ver }}.xml + path: ${{ matrix.test.path }}/*.xml run-target-asio: strategy: @@ -348,7 +327,6 @@ jobs: idf.py set-target ${{ matrix.idf_target }} idf.py build $GITHUB_WORKSPACE/ci/clean_build_artifacts.sh ${GITHUB_WORKSPACE}/${TEST_DIR}/build - ls build - uses: actions/upload-artifact@v3 with: name: modem_target_bin_${{ matrix.idf_target }}_${{ matrix.idf_ver }}_${{ matrix.test.app }} diff --git a/ci/clean_build_artifacts.sh b/ci/clean_build_artifacts.sh index 362fe4d84..ddf9e5592 100755 --- a/ci/clean_build_artifacts.sh +++ b/ci/clean_build_artifacts.sh @@ -4,4 +4,4 @@ # - flasher args # - sdkconfigs (header and json) # (Ignoring the command failure as it refuses to delete nonempty dirs) -find $1 ! -regex ".*/build/[^/]+.\(bin\|elf\)" -a ! -regex ".*\(bootloader\|partition-table\).bin" -a ! -name "flasher_args.json" -a ! -regex ".*/build/config/sdkconfig.\(h\|json\)" -delete || true +find $1 ! -regex ".*/build[^/]*/[^/]+.\(bin\|elf\)" -a ! -regex ".*\(bootloader\|partition-table\).bin" -a ! -name "flasher_args.json" -a ! -regex ".*/build[^/]*/config/sdkconfig.\(h\|json\)" -delete || true diff --git a/components/mdns/examples/pytest_mdns.py b/components/mdns/examples/pytest_mdns.py index fc13d8e77..f13abc149 100644 --- a/components/mdns/examples/pytest_mdns.py +++ b/components/mdns/examples/pytest_mdns.py @@ -1,4 +1,4 @@ -# SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD +# SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD # SPDX-License-Identifier: Unlicense OR CC0-1.0 import re import select @@ -8,8 +8,11 @@ import subprocess import time from threading import Event, Thread -import dpkt -import dpkt.dns +try: + import dpkt + import dpkt.dns +except ImportError: + pass def get_dns_query_for_esp(esp_host): diff --git a/components/mdns/tests/unit_test/CMakeLists.txt b/components/mdns/tests/unit_test/CMakeLists.txt index 5994c440c..450d475e4 100644 --- a/components/mdns/tests/unit_test/CMakeLists.txt +++ b/components/mdns/tests/unit_test/CMakeLists.txt @@ -1,2 +1,7 @@ -idf_component_register(SRC_DIRS "." - PRIV_REQUIRES cmock test_utils mdns) +# This is the project CMakeLists.txt file for the test subproject +cmake_minimum_required(VERSION 3.16) + +set(EXTRA_COMPONENT_DIRS ../.. "$ENV{IDF_PATH}/tools/unit-test-app/components") + +include($ENV{IDF_PATH}/tools/cmake/project.cmake) +project(mdns_test) diff --git a/components/mdns/tests/unit_test/main/CMakeLists.txt b/components/mdns/tests/unit_test/main/CMakeLists.txt new file mode 100644 index 000000000..d04d217e4 --- /dev/null +++ b/components/mdns/tests/unit_test/main/CMakeLists.txt @@ -0,0 +1,5 @@ + +idf_component_register(SRCS "test_mdns.c" + REQUIRES test_utils + INCLUDE_DIRS "." + PRIV_REQUIRES unity mdns) diff --git a/components/mdns/tests/unit_test/test_mdns.c b/components/mdns/tests/unit_test/main/test_mdns.c similarity index 86% rename from components/mdns/tests/unit_test/test_mdns.c rename to components/mdns/tests/unit_test/main/test_mdns.c index fbdbce3ad..5f0f2c9cc 100644 --- a/components/mdns/tests/unit_test/test_mdns.c +++ b/components/mdns/tests/unit_test/main/test_mdns.c @@ -1,13 +1,16 @@ /* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Unlicense OR CC0-1.0 */ -#include "test_utils.h" + #include "mdns.h" #include "esp_event.h" #include "unity.h" +#include "test_utils.h" +#include "unity_fixture.h" +#include "memory_checks.h" #define MDNS_HOSTNAME "test-hostname" #define MDNS_DELEGATE_HOSTNAME "delegate-hostname" @@ -16,6 +19,18 @@ #define MDNS_SERVICE_PROTO "_tcp" #define MDNS_SERVICE_PORT 80 +TEST_GROUP(mdns); + +TEST_SETUP(mdns) +{ + test_utils_record_free_mem(); + TEST_ESP_OK(test_utils_set_leak_level(0, ESP_LEAK_TYPE_CRITICAL, ESP_COMP_LEAK_GENERAL)); +} + +TEST_TEAR_DOWN(mdns) +{ + test_utils_finish_and_evaluate_leaks(32, 64); +} static void yield_to_all_priorities(void) { @@ -27,7 +42,7 @@ static void yield_to_all_priorities(void) } -TEST_CASE("mdns api to fail in invalid state", "[mdns][leaks=64]") +TEST(mdns, api_fails_with_invalid_state) { TEST_ASSERT_NOT_EQUAL(ESP_OK, mdns_init() ); TEST_ASSERT_NOT_EQUAL(ESP_OK, mdns_hostname_set(MDNS_HOSTNAME) ); @@ -35,7 +50,7 @@ TEST_CASE("mdns api to fail in invalid state", "[mdns][leaks=64]") TEST_ASSERT_NOT_EQUAL(ESP_OK, mdns_service_add(MDNS_INSTANCE, MDNS_SERVICE_NAME, MDNS_SERVICE_PROTO, MDNS_SERVICE_PORT, NULL, 0) ); } -TEST_CASE("mdns init and deinit", "[mdns][leaks=64]") +TEST(mdns, init_deinit) { test_case_uses_tcpip(); TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_create_default()); @@ -45,7 +60,7 @@ TEST_CASE("mdns init and deinit", "[mdns][leaks=64]") esp_event_loop_delete_default(); } -TEST_CASE("mdns api return expected err-code and do not leak memory", "[mdns][leaks=64]") +TEST(mdns, api_fails_with_expected_err) { mdns_txt_item_t serviceTxtData[CONFIG_MDNS_MAX_SERVICES] = { {NULL, NULL}, }; @@ -91,7 +106,7 @@ TEST_CASE("mdns api return expected err-code and do not leak memory", "[mdns][le esp_event_loop_delete_default(); } -TEST_CASE("mdns query api return expected err-code and do not leak memory", "[leaks=64]") +TEST(mdns, query_api_fails_with_expected_err) { mdns_result_t *results = NULL; esp_ip6_addr_t addr6; @@ -118,3 +133,16 @@ TEST_CASE("mdns query api return expected err-code and do not leak memory", "[le mdns_free(); esp_event_loop_delete_default(); } + +TEST_GROUP_RUNNER(mdns) +{ + RUN_TEST_CASE(mdns, api_fails_with_invalid_state) + RUN_TEST_CASE(mdns, api_fails_with_expected_err) + RUN_TEST_CASE(mdns, query_api_fails_with_expected_err) + RUN_TEST_CASE(mdns, init_deinit) +} + +void app_main(void) +{ + UNITY_MAIN(mdns); +} diff --git a/components/mdns/tests/unit_test/pytest_mdns.py b/components/mdns/tests/unit_test/pytest_mdns.py new file mode 100644 index 000000000..093daf270 --- /dev/null +++ b/components/mdns/tests/unit_test/pytest_mdns.py @@ -0,0 +1,8 @@ +# SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD +# SPDX-License-Identifier: Apache-2.0 + +from pytest_embedded import Dut + + +def test_lwip(dut: Dut) -> None: + dut.expect_unity_test_output() diff --git a/components/mdns/tests/unit_test/sdkconfig.defaults b/components/mdns/tests/unit_test/sdkconfig.defaults new file mode 100644 index 000000000..168e08d4c --- /dev/null +++ b/components/mdns/tests/unit_test/sdkconfig.defaults @@ -0,0 +1,2 @@ +CONFIG_UNITY_ENABLE_FIXTURE=y +CONFIG_UNITY_ENABLE_IDF_TEST_RUNNER=n From a8339e46180fed69e688c04b6de962635acee767 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Fri, 20 Jan 2023 10:46:41 +0100 Subject: [PATCH 2/3] fix(mdns): Allow setting instance name only after hostname set Closes https://github.com/espressif/esp-protocols/issues/190 --- components/mdns/mdns.c | 2 +- components/mdns/tests/unit_test/main/test_mdns.c | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 1149ff7cf..5d08322f0 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -5398,7 +5398,7 @@ esp_err_t mdns_instance_name_set(const char *instance) if (!_mdns_server) { return ESP_ERR_INVALID_STATE; } - if (_str_null_or_empty(instance) || strlen(instance) > (MDNS_NAME_BUF_LEN - 1)) { + if (_str_null_or_empty(instance) || _mdns_server->hostname == NULL || strlen(instance) > (MDNS_NAME_BUF_LEN - 1)) { return ESP_ERR_INVALID_ARG; } char *new_instance = strndup(instance, MDNS_NAME_BUF_LEN - 1); diff --git a/components/mdns/tests/unit_test/main/test_mdns.c b/components/mdns/tests/unit_test/main/test_mdns.c index 5f0f2c9cc..3ac613c3b 100644 --- a/components/mdns/tests/unit_test/main/test_mdns.c +++ b/components/mdns/tests/unit_test/main/test_mdns.c @@ -115,6 +115,14 @@ TEST(mdns, query_api_fails_with_expected_err) TEST_ASSERT_EQUAL(ESP_OK, esp_event_loop_create_default()); TEST_ASSERT_EQUAL(ESP_OK, mdns_init() ); + // check it is not possible to register a service or set an instance without configuring the hostname + TEST_ASSERT_EQUAL(ESP_ERR_INVALID_ARG, mdns_service_add(MDNS_INSTANCE, MDNS_SERVICE_NAME, MDNS_SERVICE_PROTO, MDNS_SERVICE_PORT, NULL, 0)); + TEST_ASSERT_EQUAL(ESP_ERR_INVALID_ARG, mdns_instance_name_set(MDNS_INSTANCE)); + TEST_ASSERT_EQUAL(ESP_OK, mdns_hostname_set(MDNS_HOSTNAME)); + // hostname is set, now adding a service and instance should succeed + TEST_ASSERT_EQUAL(ESP_OK, mdns_service_add(MDNS_INSTANCE, MDNS_SERVICE_NAME, MDNS_SERVICE_PROTO, MDNS_SERVICE_PORT, NULL, 0)); + TEST_ASSERT_EQUAL(ESP_OK, mdns_instance_name_set(MDNS_INSTANCE)); + TEST_ASSERT_EQUAL(ESP_OK, mdns_query_ptr(MDNS_SERVICE_NAME, MDNS_SERVICE_PROTO, 10, CONFIG_MDNS_MAX_SERVICES, &results) ); mdns_query_results_free(results); From d0c9070715db7a39598693d2f17d69d3fb364958 Mon Sep 17 00:00:00 2001 From: David Cermak Date: Fri, 20 Jan 2023 10:55:16 +0100 Subject: [PATCH 3/3] fix(mdns): Remove strict mode as it's invalid Strict mode was introduced to support "one-shot" queries (described in RFC6762/sec5.1) that are sent by lwip or dig. It was incorrectly assumed that responding to such queries violates the spec, as we have to repeat queries in responces, which is forbidden in RFC6762/sec6. It is however required to repeat query fields according to the Section 6.7. Legacy Unicast Responses: "it MUST repeat the query ID and the question given in the query message." --- components/mdns/Kconfig | 11 ---------- components/mdns/mdns.c | 2 -- .../mdns/private_include/mdns_private.h | 21 ------------------- 3 files changed, 34 deletions(-) diff --git a/components/mdns/Kconfig b/components/mdns/Kconfig index def0d850d..50bab3ba1 100644 --- a/components/mdns/Kconfig +++ b/components/mdns/Kconfig @@ -64,17 +64,6 @@ menu "mDNS" Configures timeout for adding a new mDNS service. Adding a service fails if could not be completed within this time. - config MDNS_STRICT_MODE - bool "mDNS strict mode" - default "n" - help - Configures strict mode. Set this to 1 for the mDNS library to strictly follow the RFC6762: - Currently the only strict feature: Do not repeat original questions in response packets - (defined in RFC6762 sec. 6). - Default configuration is 0, i.e. non-strict mode, since some implementations, - such as lwIP mDNS resolver (used by standard POSIX API like getaddrinfo, gethostbyname) - could not correctly resolve advertised names. - config MDNS_TIMER_PERIOD_MS int "mDNS timer period (ms)" range 10 10000 diff --git a/components/mdns/mdns.c b/components/mdns/mdns.c index 5d08322f0..42bb4f5da 100644 --- a/components/mdns/mdns.c +++ b/components/mdns/mdns.c @@ -1728,7 +1728,6 @@ static void _mdns_create_answer_from_parsed_packet(mdns_parsed_packet_t *parsed_ return; } -#ifdef MDNS_REPEAT_QUERY_IN_RESPONSE if (parsed_packet->src_port != MDNS_SERVICE_PORT && // Repeat the queries only for "One-Shot mDNS queries" (q->type == MDNS_TYPE_ANY || q->type == MDNS_TYPE_A || q->type == MDNS_TYPE_AAAA)) { mdns_out_question_t *out_question = malloc(sizeof(mdns_out_question_t)); @@ -1751,7 +1750,6 @@ static void _mdns_create_answer_from_parsed_packet(mdns_parsed_packet_t *parsed_ out_question->own_dynamic_memory = true; queueToEnd(mdns_out_question_t, packet->questions, out_question); } -#endif // MDNS_REPEAT_QUERY_IN_RESPONSE if (q->unicast) { unicast = true; } diff --git a/components/mdns/private_include/mdns_private.h b/components/mdns/private_include/mdns_private.h index d4fef5f13..0f40110e8 100644 --- a/components/mdns/private_include/mdns_private.h +++ b/components/mdns/private_include/mdns_private.h @@ -21,27 +21,6 @@ #define _mdns_dbg_printf(...) printf(__VA_ARGS__) #endif -/** mDNS strict mode: Set this to 1 for the mDNS library to strictly follow the RFC6762: - * Strict features: - * - to do not set original questions in response packets per RFC6762, sec 6 - * - * The actual configuration is 0, i.e. non-strict mode, since some implementations, - * such as lwIP mdns resolver (used by standard POSIX API like getaddrinfo, gethostbyname) - * could not correctly resolve advertised names. - */ -#ifndef CONFIG_MDNS_STRICT_MODE -#define MDNS_STRICT_MODE 0 -#else -#define MDNS_STRICT_MODE 1 -#endif - -#if !MDNS_STRICT_MODE -/* mDNS responders sometimes repeat queries in responses - * but according to RFC6762, sec 6: Responses MUST NOT contain - * any item in question field */ -#define MDNS_REPEAT_QUERY_IN_RESPONSE 1 -#endif - /** Number of predefined interfaces */ #ifndef CONFIG_MDNS_PREDEF_NETIF_STA #define CONFIG_MDNS_PREDEF_NETIF_STA 0