On Wed, 2023-10-18 at 11:33 +0200, Pablo Neira Ayuso wrote: > On Wed, Oct 18, 2023 at 12:33:29AM +0200, Thomas Haller wrote: > > Some tests cannot pass, for example due to missing kernel features > > or > > kernel bugs. The tests make an educated guess (feature detection), > > whether the failure is due to that, and marks the test as SKIP > > (exit > > 77). The problem is, the test might guess wrong and hide a real > > issue. > > > > Add support for a NFT_TEST_FAIL_ON_SKIP_EXCEPT= regex to help with > > this. > > Together with NFT_TEST_FAIL_ON_SKIP=y is enabled, test names that > > match > > the regex are allowed to be skipped. Unexpected skips are treated > > as > > fatal. This allows to maintain a list of tests that are known to be > > skipped. > > > > You can think of this as some sort of XFAIL/XPASS mechanism. The > > difference is that usually XFAIL is part of the test. Here the > > failure > > happens due to external conditions, as such you need to configure > > NFT_TEST_FAIL_ON_SKIP_EXCEPT= per kernel. Also, usually XFAIL is > > about > > failing tests, while this is about tests that are marked to be > > skipped. > > But we mark them as skipped due to a heuristic, and those we want > > to > > manually keep track off. > > > > Why is NFT_TEST_FAIL_ON_SKIP_EXCEPT= useful and why doesn't it just > > work > > automatically? It does work automatically (see use case 1 below). > > Trust > > the automatism to the right thing, and don't bother. This is when > > you > > don't trust the automatism and you curate a list of tests that are > > known > > to be be skipped, but no others (see use case 3 below). In > > particular, > > it's for running CI on a downstream kernel, where we expect a > > static > > list of skipped tests and where we want to find any changes. > > > > Consider this: there are three use case for running the tests. This patch, and the 3 use cases are about how to treat SKIPs (and the potential that a SKIP might happen due to a wrong reason, and how to ensure this doesn't hide an actual bug). Your reply makes me think that you consider this only relevant for the "eval-exit-code" patches. It's not (although it could nicely work together and solve some concerns). SKIPs already exist. This patch is independent of that other patches or whatever the outcome of those is. Unless you think, that the current SKIP mechanism (feature detection/probing) in master is 100% reliable, and there is no need to worry about SKIP hiding a bug. Then just consider yourself to be in use-case 1 (which means to trust the automatism and not to care). Use case 3 is enabled by this patch allows with the new NFT_TEST_FAIL_ON_SKIP_EXCEPT option. And it's the result of Florian saying: >>> No, its not my use case. >>> >>> The use case is to make sure that the frankenkernels that I am in >>> charge of do not miss any important bug fixes. >>> >>> This is the reason for the feature probing, "skip" should tell me >>> that I can safely ignore it because the feature is not present. >>> >>> I could built a list of "expected failures", but that will >>> mask real actual regressions. > > > > 1) The contributor, who wants to run the test suite. The tests > > make a > > best effort to pass and when the test detects that the failure is > > likely due to the kernel, then it will skip the test. This is the > > default. > > This is not a good default, it is us that are running this tests > infrastructure, we are the target users. This is the current default already, and what was introduced with the recent additions of SKIP. An effort from Florian. > This contributor should be running latest kernel either from nf.git > and nf-next.git It means running the test suite on distro kernels is not a supported use case. I thought, Florian said that he does exactly that? It also means, you cannot PASS the test suite on $DISTRO, after you build the kernel and the corresponding nftables packages. E.g. Fedora does not enable OSF module. OSF tests cannot pass. They are consequently SKIP on Fedora. Nothing wrong with running the test suite against Fedora kernel. Why would I be required to recompile another kernel, unless I want to test a more recent kernel patch? > > > 2) The maintainer, who runs latest kernel and expects that all > > tests are > > passing. Any SKIP is likely a bug. This is achieved by setting > > NFT_TEST_FAIL_ON_SKIP=y. > > I don't want to remember to set this on, I am running this in my > test machines all the time. echo "export NFT_TEST_FAIL_ON_SKIP=y" >> ~/.bashrc Also, there are only are reasonably small number of options that the test suite has. See "tests/shell/run-tests.sh --help". The majority of users don't need to care, the the default aims to do the thing they want (use-case 1). Feel free not to care either. Then use-cases 2 and use-cases 3 are just not yours. This doesn't mean you are not a "maintainer", it just means you trust the SKIP automatism or keep track of SKIPs via other means. I think for a core developer, it would be useful to be aware and use some of those options. There is quite useful stuff there. > > > 3) The downstream maintainer, who test a distro kernel that is > > known to > > lack certain features. They know that a selected few tests should > > be > > skipped, but most tests should pass. By setting > > NFT_TEST_FAIL_ON_SKIP=y > > and NFT_TEST_FAIL_ON_SKIP_EXCEPT= they can specify which are > > expected to > > be skipped. The regex is kernel/environment specific, and must be > > actively curated. > > Such downstream maintainer should be really concerned about the test > failure and track the issue to make sure the fix gets to their > downstream kernel. None of the 3 use cases allow any "failures". Of course failures must be avoided at all cost, because a failing test suite looses a lot of it's benefits. There must be only PASS and some SKIP. - Use case 1 is to not care about SKIPs and trust that they don't hide a bug. - Use case 2 is about not allowing any SKIPs at all. All tests must PASS. NFT_TEST_FAIL_ON_SKIP=y ensures that. You'll probably need to build your own kernel for this. - Use case 3 is about allowing only a selected list of SKIPs (and get an error when the SKIP/PASS state of a test is not as expected). NFT_TEST_FAIL_ON_SKIP_EXCEPT= is the "selected list". > > > BONUS) actually, cases 2) and 3) optimally run automated CI > > tests. > > Then the test environment is defined with a particular kernel > > variant > > and changes slowly over time. NFT_TEST_FAIL_ON_SKIP_EXCEPT= > > allows > > to pick up any unexpected changes of the skipped/pass behavior of > > tests. > > > > If a test matches the regex but passes, this is also flagged as a > > failure ([XPASS]). The user is required to maintain an accurate > > list of > > XFAIL tests. > > > > Example: > > > > $ NFT_TEST_FAIL_ON_SKIP=y \ > > NFT_TEST_FAIL_ON_SKIP_EXCEPT='^(tests/shell/testcases/nft- > > f/0017ct_timeout_obj_0|tests/shell/testcases/listing/0020flowtable_ > > 0)$' \ > > ./tests/shell/run-tests.sh \ > > tests/shell/testcases/nft-f/0017ct_timeout_obj_0 \ > > tests/shell/testcases/cache/0006_cache_table_flush \ > > tests/shell/testcases/listing/0013objects_0 \ > > tests/shell/testcases/listing/0020flowtable_0 > > ... > > I: [SKIPPED] 1/4 tests/shell/testcases/nft- > > f/0017ct_timeout_obj_0 > > I: [OK] 2/4 > > tests/shell/testcases/cache/0006_cache_table_flush > > W: [FAIL-SKIP] 3/4 tests/shell/testcases/listing/0013objects_0 > > W: [XPASS] 4/4 > > tests/shell/testcases/listing/0020flowtable_0 > > > > This list of XFAIL tests is maintainable, because on a particular > > downstream kernel, the number of tests known to be skipped is small > > and > > relatively static. Also, you can generate the list easily (followed > > by > > manual verification!) via > > > > $ NFT_TEST_FAIL_ON_SKIP=n ./tests/shell/run-tests.sh -k > > $ export NFT_TEST_FAIL_ON_SKIP_EXCEPT="$(cat /tmp/nft- > > test.latest.*/skip-if-fail-except)" > > $ NFT_TEST_FAIL_ON_SKIP=y ./tests/shell/run-tests.sh > > > > Signed-off-by: Thomas Haller <thaller@xxxxxxxxxx> > > --- > > Sorry for the overly long commit message. I hope it can be useful > > and > > speak in favor of the patch (and not against it). > > > > This is related to the "eval-exit-code" patch, as it's about how to > > handle tests that are SKIPed. But it's relevant for any skipped > > test, > > and not tied to that work. > > > > tests/shell/helpers/test-wrapper.sh | 41 +++++++++++++++++++++ > > tests/shell/run-tests.sh | 55 ++++++++++++++++++++++--- > > ---- > > 2 files changed, 83 insertions(+), 13 deletions(-)
