Skip to content

tests: build non-debug unit tests with autotools, run them #16771

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 13 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Mar 19, 2025

Before this patch, autotools disabled building unit tests for
non-debug-enabled (DEBUGBUILD) builds. runtests skipped running this
combination, though they were built in cmake builds. There seems to be
no technical reason anymore for these restrictions. This patch removes
them, allowing to build and run unit tests for non-debug-enabled builds.

To improve unit test build and run coverage.

  • autotools: do not disable building unit tests for non-debug-enabled
    build. Bringing behavior closer to cmake builds. (There are still
    exceptions in autotools, something for another PR)

  • runtests: run unit tests for non-debug-enabled builds.
    It extends coverage by 50 tests.

  • lib/altsvc.c: fix to include CURL_TIME support in libcurlu, for
    unit tests. It fixes test 1654, and syncs CURL_TIME behavior with
    test 1660 and lib/hsts.c.

Ref: 10a7d05
Ref: fc8e0de #13694
Ref: 99f78cb #16770

@vszakats vszakats marked this pull request as draft March 19, 2025 23:56
@testclutch

This comment was marked as outdated.

@vszakats vszakats changed the title autotools: allow building unittests with debug features disabled (as cmake) autotools: allow building unittests with debug features disabled Mar 20, 2025
@vszakats vszakats marked this pull request as ready for review March 20, 2025 01:07
@vszakats vszakats marked this pull request as draft March 20, 2025 01:10
@vszakats
Copy link
Member Author

vszakats commented Mar 20, 2025

This costs build time. It's +50 programs (in bundle mode it's just +1).

Another take on this is to disable building unittests in cmake non-debug-enabled builds.

There is no technical reason not to build them, but if running tests in this combination doesn't make any sense, it'd be best to skip building it also in cmake to save time.

Example with Cygwin, bundles enabled:

edit: from a test coverage POV building these is pointless, because runtests skips unittest if the build is not debug-enabled. From a build coverage POV, dropping unittests reduces it by disabling unittests in non-debug-enabled cmake jobs, like macos clang-tidy, mingw UWP, mingw32ce, MS-DOS, AmigaOS, and more. Building everything in debug mode would fix it, but that reduces coverage for non-debug builds. Perhaps an extra option to enable building it anyway would solve this. Or, just leaving it as-is and accept that cmake is different here than autotools. The cost of building is much less with cmake, esp when using unity and/or bundles.

@github-actions github-actions bot added the CI Continuous Integration label Mar 20, 2025
Also enable for clang-tidy job explicitly for coverage.

Lost unittest build coverage: mingw UWP, mingw32ce

This saves time for non-debug-enabled builds by not building what's
skipped by test runs anyway. At the same time it reduces bulid coverage
in CI. (Possibly not saving much for most jobs due to unity and bundles,
though it's both libcurlu and unit test binaries.)

Also note:
- http/client programs are also built unconditionally
  (in autotools too), even when pytests aren't configured and run.
Though it passes regardless, so CURL_TIME maybe isn't required,
or passes by chance anyway?
@vszakats
Copy link
Member Author

vszakats commented Mar 20, 2025

Hm, it looks like unittests do not actually require a debug-enabled build, at least according to a CI run with this runtests condition removed.

Except test 1654 that missed the Debug feature label, and another one that also uses CURL_TIME, without requiring Debug, though that one passed anyway. After fixing 1654, CI is green. Historically this condition may have been related to the entangled UNITTESTS, DEBUGBUILD, CURLDEBUG flag use, but that has been fixed earlier in fc8e0de #13694.

I'd go with unlocking this combination in ./configure, letting unittests build there for non-debug-enabled builds (like with CMake), and also let them run to make the built binaries useful. It also means that unittest are now run in cmake non-debug-enabled builds too, which looks like a useful thing for test coverage (+50 tests):

Before:

TESTINFO: "curl lacks unittest support" 53 times (1300, 1302, 1303, 1304, 1305, 1306, 1308, 1309, 1323 and 44 more)
[...]
TESTDONE: 1525 tests out of 1525 reported OK: 100%

https://github.com/curl/curl/actions/runs/13967157858/job/39100108697#step:15:3552

After:

TESTDONE: 1574 tests out of 1574 reported OK: 100%

https://github.com/curl/curl/actions/runs/13969017959/job/39106067593?pr=16771

@vszakats vszakats changed the title autotools: allow building unittests with debug features disabled tests: build non-debug unittests with autotools, make them always run Mar 20, 2025
@vszakats vszakats marked this pull request as ready for review March 20, 2025 13:08
It means test 1660 require the Debug feature flag and a debug-enabled
build to run.

To make this consistent with altsvc.c.
This reverts commit 7d279a5.

Not required, hsts.c includes CURL_TIME support for unitests libcurlu.
This reverts commit da6ce33.

Overridden by the altsvs.c fix.
@vszakats vszakats changed the title tests: build non-debug unittests with autotools, make them always run tests: build non-debug unittests with autotools, and run them Mar 20, 2025
@vszakats vszakats changed the title tests: build non-debug unittests with autotools, and run them tests: build non-debug unit tests with autotools, and run them Mar 20, 2025
@vszakats vszakats changed the title tests: build non-debug unit tests with autotools, and run them tests: build non-debug unit tests with autotools, run them Mar 24, 2025
@vszakats vszakats closed this in c48c491 Mar 24, 2025
@vszakats vszakats deleted the cm-omit-unittest branch March 24, 2025 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build CI Continuous Integration tests
Development

Successfully merging this pull request may close these issues.

2 participants