CI: be explicit when skipping unit tests

[kinkie]

The Automake test framework uses a specific
return value from tests to signify they have
been skipped. Use it.

[kinkie]

As a followup, we might want to stack on top of PR #2001 so that
skipped tests are displayed in test-builds.sh summary
(skipped tests regex: "^SKIP: ")
Sample output:

SKIP: tests/testRock
SKIP: tests/testUfs
SKIP: tests/testACLMaxUserIP

[rousskov]

As a followup, we might want to stack on top of PR #2001 so that skipped tests are displayed in test-builds.sh summary (skipped tests regex: "^SKIP: ")

I would be against that followup change: test-builds.sh summary goal is exposing/highlighting problems. In this context, a skipped test is not a problem IMO -- it was intentionally skipped for a valid reason (if it were a problem it would have been reported as FAIL, ERROR, or some such). FWIW, TAP specs say that "harnesses must not treat failing SKIP test points as a test failure".

[rousskov]

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).

[kinkie]

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..

[kinkie]

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

[rousskov]

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?

[kinkie]

As a followup, we might want to stack on top of PR #2001 so that skipped tests are displayed in test-builds.sh summary (skipped tests regex: "^SKIP: ")

I would be against that followup change: test-builds.sh summary goal is exposing/highlighting problems. In this context, a skipped test is not a problem IMO -- it was intentionally skipped for a valid reason (if it were a problem it would have been reported as FAIL, ERROR, or some such). FWIW, TAP specs say that "harnesses must not treat failing SKIP test points as a test failure".

I only partly agree to

test-builds.sh summary goal is exposing/highlighting problems

I would express the definition as
test-builds.sh summary goal is exposing/highlighting information worth of notice

By this definition, knowing that some feature was not tested is noteworthy, and it's easier to do it here than spelunking through a lot of ./configure output . Of the tests that are currently conditionally built, only testRock has system dependencies, the others have to be intentionally disabled - and I agree with you that it might be worthwhile to make enabling them unconditional.

In other words, I believe my intent was not clear. I do not mean to second-guess configure, but to highlight information which in the general case might not be intentional

[kinkie]

Output from a test run:

$ grep SKIP *.log
// ...
btlayer-04-noauth-everything.log:# SKIP:  0
btlayer-04-noauth-everything.log:# SKIP:  0
btlayer-04-noauth-everything.log:SKIP: tests/testACLMaxUserIP
btlayer-04-noauth-everything.log:# SKIP:  1
btlayer-04-noauth-everything.log:# SKIP:  0
btlayer-04-noauth-everything.log:# SKIP:  0
btlayer-04-noauth-everything.log:SKIP: squid.conf test: bug4832.conf

[rousskov]

As a followup, we might want to stack on top of PR #2001 so that skipped tests are displayed in test-builds.sh summary (skipped tests regex: "^SKIP: ")

I would be against that followup change: test-builds.sh summary goal is exposing/highlighting problems. In this context, a skipped test is not a problem IMO -- it was intentionally skipped for a valid reason (if it were a problem it would have been reported as FAIL, ERROR, or some such). FWIW, TAP specs say that "harnesses must not treat failing SKIP test points as a test failure".

I only partly agree to

test-builds.sh summary goal is exposing/highlighting problems

I would express the definition as test-builds.sh summary goal is exposing/highlighting information worth of notice

I do not recommend using "information worth of notice" definition because it triggers endless fights of what is worth noting and/or results in excessive noise that effectively cancels any attempts at "highlighting" (because a lot of things end up worth noting for someone in some use cases). For many years, we have experienced both symptoms with cache.log, especially at debugging levels 0 and 1!

• "Highlighting nothing" is trivial but throws the baby out with the bathwater.
• "Highlighting everything" is easy but drowns the proverbial baby in the bathwater.
• "Highlighting problems" is far from perfect (we need to detail what should be considered a "problem" in this context) but could be a reasonable initial target between those two extremes.
• "Highlighting information worth highlighting" does not get us closer to defining what should be highlighted.

By this definition, knowing that some feature was not tested is noteworthy, and it's easier to do it here than spelunking through a lot of ./configure output . Of the tests that are currently conditionally built, only testRock has system dependencies, the others have to be intentionally disabled - and I agree with you that it might be worthwhile to make enabling them unconditional.

At the risk of extending what should be an out-of-scope discussion, I suspect that rock cache_dirs have the same set of "system dependencies" as ufs cache_dirs; both rock and ufs cache_dirs should probably be built unconditionally. Ufs may be effectively built unconditionally already. Other than the current existence of (problematic IIRC) ./configure tests and "size of executable" arguments (outweighed by that other, more important considerations), do you know of any specific reason not to build both unconditionally?

In other words, I believe my intent was not clear. I do not mean to second-guess configure, but to highlight information which in the general case might not be intentional

FWIW, that intent was fairly clear to me AFAICT. While somebody might incorrectly disable a Squid feature during ./configure, adding conditional compilation to src/Makefile.am just to tell them that they have disabled a Squid feature is not worth it. And listing all disabled features in top CI output creates too much noise (and associated work; this PR changes just scratch the surface of that idea).

[rousskov]

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?

[rousskov]

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