Skip to content

CI: be explicit when skipping unit tests #2002

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

CI: be explicit when skipping unit tests #2002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bcdbbea
CI: be explicit when skipping unit tests
kinkie Feb 24, 2025
25bc860
Source format
kinkie Feb 24, 2025
288e278
Emulate SKIP for squid.conf test
kinkie Feb 25, 2025
e31d084
Apply suggestions from code review
kinkie Feb 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,8 +1018,8 @@ tests_testString_LDFLAGS = $(LIBADD_DL)

## Tests of fs/*

if ENABLE_FS_ROCK
check_PROGRAMS += tests/testRock
if ENABLE_FS_ROCK
tests_testRock_SOURCES = \
$(DELAY_POOL_SOURCE) \
$(UNLINKDSOURCE) \
Expand Down Expand Up @@ -1180,14 +1180,15 @@ tests_testRock_LDADD = \
$(XTRA_LIBS)
tests_testRock_LDFLAGS = $(AM_CPPFLAGS) $(LIBADD_DL)
else
tests_testRock_SOURCES = tests/testSkipped.cc
EXTRA_DIST += \
tests/testRock.cc \
tests/testStoreSupport.cc \
tests/testStoreSupport.h
endif

if ENABLE_FS_UFS
check_PROGRAMS += tests/testUfs
if ENABLE_FS_UFS
tests_testUfs_SOURCES = \
$(DELAY_POOL_SOURCE) \
$(UNLINKDSOURCE) \
Expand Down Expand Up @@ -1356,6 +1357,7 @@ tests_testUfs_LDADD = \
$(XTRA_LIBS)
tests_testUfs_LDFLAGS = $(LIBADD_DL)
else
tests_testUfs_SOURCES = tests/testSkipped.cc
EXTRA_DIST += \
tests/testUfs.cc
endif
Expand Down Expand Up @@ -1696,8 +1698,8 @@ tests_testDiskIO_LDFLAGS = $(LIBADD_DL)

## Tests of auth/*

if ENABLE_AUTH
check_PROGRAMS += tests/testACLMaxUserIP
if ENABLE_AUTH
tests_testACLMaxUserIP_SOURCES = \
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid extra work/noise and/or inconsistencies, ./configure-level decisions should not be second-guessed during "make": If X was disabled by ./configure, it should disappear from "make" view (to the extent such disappearance is practical -- we have to obey automake rules, for example, of course). There are valid exceptions from this rule of thumb, but we should not add exceptional Makefile treatment for the three cases targeted by this PR, two cases arguably missed by this PR (git grep skip-unless-autoconf-defines), and tempt reviewers with a precedent to demand similar exceptional Makefile treatment for new test targets IMO.

BTW, at least two of the cases targeted by this PR have preexisting problems. Solving those problems is likely to result in complete ENABLE_FS_UFS and ENABLE_FS_ROCK removal! I am not an authentication expert, but I suspect that ENABLE_AUTH should be removed as well. If you have time to work on those removals (one at a time, in the order they are mentioned in this paragraph), please do so.

If skip-unless-autoconf-defines code should print SKIP and return exit code 77, please consider making those changes (instead).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

git grep skip-unless-autoconf-defines

Thanks for letting me know, adding these.
The text in that script reinforces my proposal to let SKIPped tests pass through in test-builds.sh: historically it was deemed important enough to notify the developer of tests being skipped that the script emits WARNINGs when skipping tests..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If skip-unless-autoconf-defines code should print SKIP and return exit code 77, please consider making those changes (instead).

The problem with skip-unless-autoconf-defines is that it doesn't use Automake's test harness, but it kind-of-sort-of emulates its output.
I'm extending it to emulate it more closely

Copy link
Contributor

Choose a reason for hiding this comment

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

Alex: we should not add exceptional Makefile treatment for the three cases targeted by this PR ... and tempt reviewers with a precedent to demand similar exceptional Makefile treatment for new test targets IMO.

I still think we should not do the above.

Alex: If skip-unless-autoconf-defines code should print SKIP and return exit code 77, please consider making those changes (instead).

I still support working on those skip-unless-autoconf-defines enhancements in this PR, but please do not ignore the last word in that sentence -- "instead".

Would you be willing to remove src/Makefile.am changes from this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am happy to make including UFS and AUTH unconditional (in followup PRs, not a s a prerequisite to this).
Rock is at the moment a different story. It requires SHM, which is not currently working on MacOS, and I suppose it's never been tested on MinGW or Windows either, has it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rock is at the moment a different story. It requires SHM

Rock cache_dirs do not require shared memory. They do not use shared memory in non-SMP Squids.

SMP Squids use shared memory (with or without rock cache_dirs).

tests/testACLMaxUserIP.cc
nodist_tests_testACLMaxUserIP_SOURCES = \
Expand Down Expand Up @@ -1743,6 +1745,7 @@ tests_testACLMaxUserIP_LDADD = \
$(XTRA_LIBS)
tests_testACLMaxUserIP_LDFLAGS = $(LIBADD_DL)
else
tests_testACLMaxUserIP_SOURCES = tests/testSkipped.cc
EXTRA_DIST += \
tests/testACLMaxUserIP.cc
endif
Expand Down
22 changes: 22 additions & 0 deletions src/tests/testSkipped.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright (C) 1996-2023 The Squid Software Foundation and contributors
*
* Squid software is distributed under GPLv2+ license and includes
* contributions from numerous individuals and organizations.
* Please see the COPYING and CONTRIBUTORS files for details.
*/

/*
* this is a dummy unit test file, meant to be used when a feature
* is not available to be tested.
*/

#include "squid.h"

int
main() {
// use this magic return code to inform Automake cfgaux/test-driver
// that a test was skipped
return 77;
}

22 changes: 13 additions & 9 deletions test-suite/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,23 +124,27 @@ squid-conf-tests: $(srcdir)/test-squid-conf.sh $(top_builddir)/src/squid.conf.de
exit 1; \
fi; \
done; \
failed=0; \
result=0; \
cfglist="$(top_builddir)/src/squid.conf.default $(srcdir)/squidconf/*.conf"; \
rm -f $@ || $(TRUE); \
for cfg in $$cfglist ; do \
$(srcdir)/test-squid-conf.sh $(top_builddir) $(sbindir) $$cfg || \
{ echo "FAIL: squid.conf test: $$cfg" | \
$(srcdir)/test-squid-conf.sh $(top_builddir) $(sbindir) $$cfg; result=$$?; \
case $$result in \
0) echo "PASS: squid.conf test: $$cfg" | \
sed s%$(top_builddir)/src/%% | \
sed s%$(srcdir)/squidconf/%% ; \
failed=1; break; \
}; \
if test "$$failed" -eq 0; then \
echo "PASS: squid.conf test: $$cfg" | \
continue ;; \
77) echo "SKIP: squid.conf test: $$cfg" | \
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder what it would take to stop emulating automake test driver here and actually reuse it? Do you know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder what it would take to stop emulating automake test driver here and actually reuse it? Do you know?

From what I know, each test would have to be its own script, not an iteration in a for loop in Makefile.am

Copy link
Contributor

@rousskov rousskov Mar 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hm... We are able to use cfgaux/test-driver to testHeaders. These Squid configuration tests feel rather similar:

testHeaders: $(SOURCES) $(noinst_HEADERS) $(EXTRA_DIST) ...

squid-conf-tests: ... $(srcdir)/squidconf/*

With a few adjustments, we may even be able to move to something like this:

- squid-conf-tests: ... $(srcdir)/squidconf/*
+ squid-conf-tests: ... $(EXTRA_DIST)

The current squid-conf-tests script gets more complex with every modification, and we are writing it in a Makefile! Do you think a conversion to use cfgaux/test-driver for squid-conf-tests is worth trying?

sed s%$(top_builddir)/src/%% | \
sed s%$(srcdir)/squidconf/%% ; \
else break; fi; \
continue ;; \
*) echo "FAIL: squid.conf test: $$cfg" | \
sed s%$(top_builddir)/src/%% | \
sed s%$(srcdir)/squidconf/%% ; \
exit $$result ;; \
esac; \
done; \
if test "$$failed" -eq 0; then cp $(TRUE) $@ ; else exit 1; fi
if test "$$result" -eq 0; then cp $(TRUE) $@ ; else exit 1; fi

CLEANFILES += \
squid-conf-tests \
Expand Down
4 changes: 2 additions & 2 deletions test-suite/test-squid-conf.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ then
if grep -q "# *undef *\b$defineName\b" $autoconfHeader
then
echo "$here: WARNING: Skipping $configFile test because $defineName is not defined in $autoconfHeader";
exit 0;
exit 77
fi

if ! grep -q "# *define *\b$defineName\b" $autoconfHeader
Expand All @@ -230,7 +230,7 @@ then
if ! grep -q "# *define *\b$defineName *$defineValue\b" $autoconfHeader
then
echo "$here: WARNING: Skipping $configFile test because $defineName is not $defineValue in $autoconfHeader";
exit 0;
exit 77
fi
fi

Expand Down