Skip to content

tests: always make bundles, adapt build and tests #17590

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

Closed
wants to merge 309 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jun 11, 2025

Make test bundles the default. Drop non-bundle build mode.
Also do all the optimizations and tidy-ups this allows, simpler builds,
less bundle exceptions, streamlined build mechanics.

Also rework the init/deinit macro magic for unit tests. The new method
allows using unique init/deinit function names, and calling them with
arguments. This is in turn makes it possible to reduce the use of global
variables.

Note this drop existing build options -DCURL_TEST_BUNDLES= from cmake
and --enable-test-bundles / --disable-test-bundles from autotools.

Also:

  • rename test entry functions to have unique names: test_<testname>
    This removes the last exception that was handled in the generator.
  • fix make dist to not miss test sources with test bundles enabled.
  • sync and merge tests/mk-bundle.pl into scripts/mk-unity.pl.
  • mk-unity.pl: add --embed option and use it when CURL_CLANG_TIDY=ON
    to ensure that clang-tidy does not miss external test C sources.
    (because clang-tidy ignores code that's #included.)
  • tests/unit: drop no-op setup/stop functions.
  • tests: reduce symbol scopes, global macros, other fixes and tidy-ups.
  • tool1621: fix to run, also fix it to pass.
  • sockfilt: fix Windows compiler warning in certain unity include order,
    by explicitly including warnless.h.

Follow-up to 6897aeb #17468


w/o ws: https://github.com/curl/curl/pull/17590/files?w=1

  • perhaps first drop all build-time duplicated libtests sources. (Also maybe apply that to the two sources that I cloned in 6897aeb tests: drop mk-bundle exceptions #17468: lib587, lib645.) It would simplify this PR. → libtests: stop building the sames source multiple times #17598
  • fix breaking some autotools magic that included the test .c source in the distro before this patch, but no longer with bundles enabled. I suspect bundle never worked for dist, but went unnoticed because it wasn't CI tested.
    Also check unity for the same issue.
  • try dropping non-bundle build logic.
  • try dropping mk-bundle script. [dropped one of the two.]
    While the script is adding some build complexity, overall it seems to save some manual labour/annoyance when adding/deleting scripts.
    Also, the script is also used for unity anyway, and the script is required to embed sources for clang-tidy. The latter isn't something
    solvable without it, unless clang-tidy gets fixed to not ignore #included C sources.
  • make entry functions static.
  • give entry functions explicit unique names.
  • caveat: clang-tidy excludes included .c sources from its checks. Fixed by adding an 'embed' mode and enabling it when building with CURL_CLANG_TIDY=ON.
  • drop remaining mk-bundle script? It'll add more manual management, but simplifies the build mechanics.
    mk-bundle also has a "unity" feature for the misc test source, with autotools. [FUTURE PR]
    update: snag: make dist is tricky in this case, and adds an extra manual "accounting" job: add to the bundle file twice, then add to Makefile.inc too.
  • it would seem to make sense to drop a whole bunch of per-test #includes to make everything shorter and more efficient. [FUTURE: this can be done as we go. There isn't much to save in terms of build perf, but might be useful to make the test snippets simpler, shorted.]
  • and/or rework or add mk-bundle option to include the files instead of #include-ing them. To keep them friendly with clang-tidy. [DONE]
  • is there any downside/upside of just always embedding, insead of #including? [→ let's keep #include-ing by default for now] update: Yes there is: the compiler messages refer to the generated source, so the filenames and line numbers become useless, the only way to figure out where the issue is is by looking at the (hopefully included) code snippet.
  • issue: unit_setup(), unit_stop() are referenced from a macro. Making these names unique for each test is still a head-scratcher. Perhaps just removing this macro magic may work? It'd also allow to make some statics local and not stomp on the global namespace. They are an init block with error handling and a deinit block with predefined goto target, used by helper macros.
#define UNITTEST_START                          \
  CURLcode test(char *arg)                      \
  {                                             \
    (void)arg;                                  \
    if(unit_setup()) {                          \
      fail("unit_setup() FAILURE");             \
    }                                           \
    else {

#define UNITTEST_STOP                           \
    goto unit_test_abort; /* avoid warning */   \
unit_test_abort:                                \
    unit_stop();                                \
  }                                             \
  return (CURLcode)unitfail;                    \
  }

@vszakats vszakats added the tests label Jun 11, 2025
@vszakats vszakats marked this pull request as draft June 11, 2025 09:43
@github-actions github-actions bot added the CI Continuous Integration label Jun 11, 2025
@vszakats vszakats force-pushed the always-bundle branch 5 times, most recently from c7d3a75 to 02bdd3d Compare June 12, 2025 22:17
@vszakats vszakats marked this pull request as ready for review June 12, 2025 22:34
@vszakats vszakats changed the title tests: always build and use bundles, adapt build and tests tests: always make bundles, adapt build and tests Jun 14, 2025
@vszakats vszakats closed this in 2c27a67 Jun 14, 2025
@vszakats vszakats deleted the always-bundle branch June 14, 2025 19:09
@vszakats vszakats mentioned this pull request Jun 15, 2025
vszakats added a commit that referenced this pull request Jun 15, 2025
Follow-up to 2c27a67 #17590
Follow-up to df1ff17 #17418

Closes #17624
vszakats added a commit that referenced this pull request Jun 15, 2025
vszakats added a commit that referenced this pull request Jun 15, 2025
Instead of relying on CMake's built-in unity feature, use `mk-unity.pl`,
as already done with autotools. It simplified the build, shortens logs
and makes debugging easier because of the fewer build variations.
It also allows testing / fixing with cmake and those automatically apply
to autotools builds too. cmake builds can be much-much faster, esp.
when working the builds themselves.

It also enables "unity" in old cmake versions. Basically every test
target is a single generated .c source.

Also:
- drop a `lib` unity workaround for libtests with autotools after fixing
  the issue in libtests itself. It drops a few exceptions and makes
  libcurl build faster (in autotools unity).
- fix another `lib` autotools unity issue and drop the workaround for it
  from `mk-unity.pl`. `srcdir` was missing from the header path.
- simplify `mk-unity.pl` command-lines, drop exclusions.

Follow-up to 2c27a67 #17590

Closes #17628
vszakats pushed a commit that referenced this pull request Jun 20, 2025
Since all code fits within that, it is more convenient.

Co-authored-by: Viktor Szakats
Follow-up to 2c27a67 #17590

Closes #17623
vszakats added a commit that referenced this pull request Jun 21, 2025
Derive it from `$BUNDLE` instead. autotools seems to be already relying
on `$BUNDLE_SRC` being equal to `$BUNDLE.c`. (I haven't realized this
before aaebb45.)

Also drop redundant `nodist_<target>_SOURCE` lines in tunits and units.

Follow-up to aaebb45 #17688
Follow-up to 2c27a67 #17590

Closes #17692
denandz pushed a commit to denandz/curl that referenced this pull request Jun 21, 2025
Make test bundles the default. Drop non-bundle build mode.
Also do all the optimizations and tidy-ups this allows, simpler builds,
less bundle exceptions, streamlined build mechanics.

Also rework the init/deinit macro magic for unit tests. The new method
allows using unique init/deinit function names, and calling them with
arguments. This is in turn makes it possible to reduce the use of global
variables.

Note this drop existing build options `-DCURL_TEST_BUNDLES=` from cmake
and `--enable-test-bundles` / `--disable-test-bundles` from autotools.

Also:
- rename test entry functions to have unique names: `test_<testname>`
  This removes the last exception that was handled in the generator.
- fix `make dist` to not miss test sources with test bundles enabled.
- sync and merge `tests/mk-bundle.pl` into `scripts/mk-unity.pl`.
- mk-unity.pl: add `--embed` option and use it when `CURL_CLANG_TIDY=ON`
  to ensure that `clang-tidy` does not miss external test C sources.
  (because `clang-tidy` ignores code that's #included.)
- tests/unit: drop no-op setup/stop functions.
- tests: reduce symbol scopes, global macros, other fixes and tidy-ups.
- tool1621: fix to run, also fix it to pass.
- sockfilt: fix Windows compiler warning in certain unity include order,
  by explicitly including `warnless.h`.

Follow-up to 6897aeb curl#17468

Closes curl#17590
denandz pushed a commit to denandz/curl that referenced this pull request Jun 21, 2025
denandz pushed a commit to denandz/curl that referenced this pull request Jun 21, 2025
denandz pushed a commit to denandz/curl that referenced this pull request Jun 21, 2025
Instead of relying on CMake's built-in unity feature, use `mk-unity.pl`,
as already done with autotools. It simplified the build, shortens logs
and makes debugging easier because of the fewer build variations.
It also allows testing / fixing with cmake and those automatically apply
to autotools builds too. cmake builds can be much-much faster, esp.
when working the builds themselves.

It also enables "unity" in old cmake versions. Basically every test
target is a single generated .c source.

Also:
- drop a `lib` unity workaround for libtests with autotools after fixing
  the issue in libtests itself. It drops a few exceptions and makes
  libcurl build faster (in autotools unity).
- fix another `lib` autotools unity issue and drop the workaround for it
  from `mk-unity.pl`. `srcdir` was missing from the header path.
- simplify `mk-unity.pl` command-lines, drop exclusions.

Follow-up to 2c27a67 curl#17590

Closes curl#17628
denandz pushed a commit to denandz/curl that referenced this pull request Jun 21, 2025
Since all code fits within that, it is more convenient.

Co-authored-by: Viktor Szakats
Follow-up to 2c27a67 curl#17590

Closes curl#17623
denandz pushed a commit to denandz/curl that referenced this pull request Jun 21, 2025
Derive it from `$BUNDLE` instead. autotools seems to be already relying
on `$BUNDLE_SRC` being equal to `$BUNDLE.c`. (I haven't realized this
before aaebb45.)

Also drop redundant `nodist_<target>_SOURCE` lines in tunits and units.

Follow-up to aaebb45 curl#17688
Follow-up to 2c27a67 curl#17590

Closes curl#17692
vszakats added a commit that referenced this pull request Jun 22, 2025
Before this patch the code relied on re-initializing `TEST_HANG_TIMEOUT`
macro before compiling each test, to allow them each to override it to
a custom value for single tests. Thie required re-including `test.h`
into each test.

After this patch this macro becomes a global, immutable, default. Tests
which want to override it can now use alternate macros that do accept
a custom timeout. The only test currently affected is lib1501.

Follow-up to 2c27a67 #17590

Closes #17702
@vszakats vszakats mentioned this pull request Jun 23, 2025
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration cmdline tool tests
Development

Successfully merging this pull request may close these issues.

1 participant