Test runner improvements by dag-erling · Pull Request #21708 · curl/curl

dag-erling · 2026-05-21T09:27:46Z

Allow the path to curl-config to be specified on the command line
Allow the log directory to be specified on the command line
Factor out the list of Perl libtest scripts

* Add a -cc option to runtests which lets the user specify the path to curl-config. * Move $CURLCONFIG to the global configuration and make it usable in test scripts. * Modify the curl-config test cases (1013, 1014, 1022, 1023) to use %CURLCONFIG instead of a hard-coded path. This is a small step on the way to making the test suite installable.

vszakats · 2026-05-21T09:34:13Z

+
+TESTS_PL = \
+  test307.pl \
+  test610.pl                       test613.pl \


Suggested change

test610.pl test613.pl \

test610.pl \

test613.pl \

maybe put each in its own line? (not likely to have very many of these scripts in the future.

I was following the established style, which is one line per decade.

It's the exception, and not really decades everywhere. I think if we change, one per line is easier, also for diffs. It's also very unlikely the number of these scripts grow significantly in the future.

vszakats · 2026-05-21T09:39:57Z


 EXTRA_DIST = CMakeLists.txt $(FIRST_C) $(FIRST_H) $(UTILS_C) $(UTILS_H) $(TESTS_C) \
-  test307.pl test610.pl test613.pl test1013.pl test1022.pl mk-lib1521.pl
+  $(TESTS_PL) mk-lib1521.pl


What is the advantage of moving this list to Makefile.inc?

Makefile.inc can be ingested by other scripts or makefiles and now has a complete list of test scripts and programs.

The only makefile this is ingested in in Makefile.am, thus I don't see a compelling
reason to move this (then it'd also need to be documented in the comment at each
ingestion place, even if not used). It may be split off into its own variable within
Makefile.am, if that helps, but not a big fan of doing that in general.

edit: if this is meant to support running tests outside of the normal automake/cmake
framework, with 3rd-party tooling I suggest a local solution that doesn't need curl make
changes like this, because things without obvious internal uses are difficult to reason
about and maintain. E.g. a 3rd-party script may find tests/libcurl -name 'test*.pl' and
use that list to figure out test*.pl.

Adding TESTS_PL makes the libtest build more consistent and maintainable, independently of my out-of-tree use. Currently the Perl test scripts are only listed in EXTRA_DISTS in Makefile.am, which has a very specific meaning; now there is a list of Perl test scripts in Makefile.inc, which is automatically included in EXTRA_DISTS. This means the procedure for adding a Perl test script is now the same as for adding a C test program: add the file, then edit Makefile.inc.

Fwiw, I intend to eventually submit a PR that makes the tests installable, but it's not quite ready yet.

What this looks like to me is creating a public interface for something that is not meant to be public.

I'm not a fan of this change. It's also unrelated to the other changes in this PR and its goal is also unrelated to anything curl tests need internally.

If it will make sense in context of a future PR I believe it's should be changed and reviewed there.

Think of it as refactoring. It improves the readability and maintainability of the libtest build.

Strong disagree. IMO it does the opposite.

vszakats · 2026-05-21T09:41:03Z

In what use-case is it useful to override curl-config or the log directory?

dag-erling · 2026-05-21T11:44:00Z

In what use-case is it useful to override curl-config or the log directory?

I have a client who wants to include the cURL tests in their integration test suite. This means installing the tests and running them without the source tree present and without write access to the directory where the tests are located. With this change I can run ./runtests.pl -c /usr/bin/curl -vc /usr/bin/curl -cc /usr/bin/curl-config -ld /tmp/curl.log and most tests run just fine.

Add a TESTS_PL variable to tests/libtest/Makefile.inc which lists the handful of library tests that are actually Perl scripts instead of C programs. These were previously only listed in EXTRA_DIST in the Makefile.

vszakats · 2026-05-21T13:23:46Z

In what use-case is it useful to override curl-config or the log directory?

I have a client who wants to include the cURL tests in their integration test suite. This means installing the tests and running them without the source tree present and without write access to the directory where the tests are located. With this change I can run ./runtests.pl -c /usr/bin/curl -vc /usr/bin/curl -cc /usr/bin/curl-config -ld /tmp/curl.log and most tests run just fine.

I wonder if including this information in the PR text would better explain
why these option are added, for future readers?

dag-erling · 2026-05-21T14:29:39Z

I have a client who wants to include the cURL tests in their integration test suite. This means installing the tests and running them without the source tree present and without write access to the directory where the tests are located. With this change I can run ./runtests.pl -c /usr/bin/curl -vc /usr/bin/curl -cc /usr/bin/curl-config -ld /tmp/curl.log and most tests run just fine.

I wonder if including this information in the PR text would better explain why these option are added, for future readers?

It was mentioned in the commit message. I assumed reviewers read those.

vszakats · 2026-05-21T15:21:39Z

I have a client who wants to include the cURL tests in their integration test suite. This means installing the tests and running them without the source tree present and without write access to the directory where the tests are located. With this change I can run ./runtests.pl -c /usr/bin/curl -vc /usr/bin/curl -cc /usr/bin/curl-config -ld /tmp/curl.log and most tests run just fine.

I wonder if including this information in the PR text would better explain why these option are added, for future readers?

It was mentioned in the commit message. I assumed reviewers read those.

It doesn't? It says what the commit does, but not the why.

dag-erling · 2026-05-21T15:39:04Z

It doesn't? It says what the commit does, but not the why.

“This is a small step on the way to making the test suite installable.”

vszakats · 2026-05-21T20:36:56Z

It doesn't? It says what the commit does, but not the why.

“This is a small step on the way to making the test suite installable.”

I mean the PR message, that gets merged, if a PR is accepted.
Sub-commit messages are rarely read and not merged.

To me though this doesn't explain it, because I don't grok what
"making the test suite installable" means, and whether the project
wants this (we merged commits to the opposite effect in recent
weeks). This'd probably need a discussion before making preparation
steps towards it.

Adding runtests.pl options that the project itself doesn't use is
probably fine, and running it separately is also fine, but an
explanation is important IMO, since curl itself doesn't want to
override curl-config or the log directory, yet it needs to keep
maintaining these options if merged.

github-actions Bot added the tests label May 21, 2026

dag-erling added 2 commits May 21, 2026 11:29

runtests: Make the log directory configurable

1540f50

dag-erling force-pushed the des/tests-curlconfig branch from 4d7a143 to c125e20 Compare May 21, 2026 09:29

vszakats reviewed May 21, 2026

View reviewed changes

tests: Explicitly list Perl test scripts

98aa1e4

Add a TESTS_PL variable to tests/libtest/Makefile.inc which lists the handful of library tests that are actually Perl scripts instead of C programs. These were previously only listed in EXTRA_DIST in the Makefile.

dag-erling force-pushed the des/tests-curlconfig branch from c125e20 to 98aa1e4 Compare May 21, 2026 13:04

Uh oh!

Conversation

dag-erling commented May 21, 2026

Uh oh!

vszakats May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vszakats May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vszakats commented May 21, 2026

Uh oh!

dag-erling commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vszakats commented May 21, 2026

Uh oh!

dag-erling commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vszakats commented May 21, 2026

Uh oh!

dag-erling commented May 21, 2026

Uh oh!

vszakats commented May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

vszakats May 21, 2026 •

edited

Loading

vszakats May 21, 2026 •

edited

Loading

dag-erling commented May 21, 2026 •

edited

Loading

dag-erling commented May 21, 2026 •

edited

Loading