Messages in this thread | | | Subject | Re: [PATCH v2 6/8] selftests/harness: Refactor XFAIL into SKIP | From | Ralph Campbell <> | Date | Mon, 13 Jul 2020 17:50:34 -0700 |
| |
On 7/13/20 5:13 PM, Kees Cook wrote: > On Mon, Jul 13, 2020 at 12:08:08PM -0700, Ralph Campbell wrote: >> >> On 6/22/20 11:16 AM, Kees Cook wrote: >>> Plumb the old XFAIL result into a TAP SKIP. >>> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> --- >>> tools/testing/selftests/kselftest_harness.h | 64 ++++++++++++++----- >>> tools/testing/selftests/seccomp/seccomp_bpf.c | 8 +-- >>> 2 files changed, 52 insertions(+), 20 deletions(-) >>> >>> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h >>> index f8f7e47c739a..b519765904a6 100644 >>> --- a/tools/testing/selftests/kselftest_harness.h >>> +++ b/tools/testing/selftests/kselftest_harness.h >>> @@ -112,22 +112,22 @@ >>> __FILE__, __LINE__, _metadata->name, ##__VA_ARGS__) >>> /** >>> - * XFAIL(statement, fmt, ...) >>> + * SKIP(statement, fmt, ...) >>> * >>> - * @statement: statement to run after reporting XFAIL >>> + * @statement: statement to run after reporting SKIP >>> * @fmt: format string >>> * @...: optional arguments >>> * >>> - * This forces a "pass" after reporting a failure with an XFAIL prefix, >>> + * This forces a "pass" after reporting why something is being skipped >>> * and runs "statement", which is usually "return" or "goto skip". >>> */ >>> -#define XFAIL(statement, fmt, ...) do { \ >>> +#define SKIP(statement, fmt, ...) do { \ >>> if (TH_LOG_ENABLED) { \ >>> - fprintf(TH_LOG_STREAM, "# XFAIL " fmt "\n", \ >>> + fprintf(TH_LOG_STREAM, "# SKIP " fmt "\n", \ >>> ##__VA_ARGS__); \ >>> } \ >>> - /* TODO: find a way to pass xfail to test runner process. */ \ >>> _metadata->passed = 1; \ >>> + _metadata->skip = 1; \ >>> _metadata->trigger = 0; \ >>> statement; \ >>> } while (0) >>> @@ -777,6 +777,7 @@ struct __test_metadata { >>> struct __fixture_metadata *fixture; >>> int termsig; >>> int passed; >>> + int skip; /* did SKIP get used? */ >>> int trigger; /* extra handler after the evaluation */ >>> int timeout; /* seconds to wait for test timeout */ >>> bool timed_out; /* did this test timeout instead of exiting? */ >>> @@ -866,17 +867,31 @@ void __wait_for_test(struct __test_metadata *t) >>> fprintf(TH_LOG_STREAM, >>> "# %s: Test terminated by timeout\n", t->name); >>> } else if (WIFEXITED(status)) { >>> - t->passed = t->termsig == -1 ? !WEXITSTATUS(status) : 0; >>> if (t->termsig != -1) { >>> + t->passed = 0; >>> fprintf(TH_LOG_STREAM, >>> "# %s: Test exited normally instead of by signal (code: %d)\n", >>> t->name, >>> WEXITSTATUS(status)); >>> - } else if (!t->passed) { >>> - fprintf(TH_LOG_STREAM, >>> - "# %s: Test failed at step #%d\n", >>> - t->name, >>> - WEXITSTATUS(status)); >>> + } else { >>> + switch (WEXITSTATUS(status)) { >>> + /* Success */ >>> + case 0: >>> + t->passed = 1; >>> + break; >>> + /* SKIP */ >>> + case 255: >>> + t->passed = 1; >>> + t->skip = 1; >>> + break; >>> + /* Other failure, assume step report. */ >>> + default: >>> + t->passed = 0; >>> + fprintf(TH_LOG_STREAM, >>> + "# %s: Test failed at step #%d\n", >>> + t->name, >>> + WEXITSTATUS(status)); >>> + } >>> } >>> } else if (WIFSIGNALED(status)) { >>> t->passed = 0; >>> @@ -906,6 +921,7 @@ void __run_test(struct __fixture_metadata *f, >>> { >>> /* reset test struct */ >>> t->passed = 1; >>> + t->skip = 0; >>> t->trigger = 0; >>> t->step = 0; >>> t->no_print = 0; >>> @@ -918,15 +934,31 @@ void __run_test(struct __fixture_metadata *f, >>> t->passed = 0; >>> } else if (t->pid == 0) { >>> t->fn(t, variant); >>> - /* return the step that failed or 0 */ >>> - _exit(t->passed ? 0 : t->step); >>> + /* Make sure step doesn't get lost in reporting */ >>> + if (t->step >= 255) { >>> + ksft_print_msg("Too many test steps (%u)!?\n", t->step); >>> + t->step = 254; >>> + } >> >> I noticed that this message is now appearing in the HMM self tests. >> I haven't quite tracked down why ->steps should be 255 after running >> the first test. I did notice that ASSERT*() calls __INC_STEP() but >> that doesn't explain it. >> Separately, maybe __INC_STEP() should check for < 254 instead of < 255? >> >> Set CONFIG_HMM_TESTS=m, build and install kernel and modules. >> cd tools/testing/selftests/vm >> make >> ./test_hmm.sh smoke >> Running smoke test. Note, this test provides basic coverage. >> [ 106.803476] memmap_init_zone_device initialised 65536 pages in 7ms >> [ 106.810141] added new 256 MB chunk (total 1 chunks, 256 MB) PFNs [0x3ffff0000 0x400000000) >> [ 106.823703] memmap_init_zone_device initialised 65536 pages in 4ms >> [ 106.829968] added new 256 MB chunk (total 1 chunks, 256 MB) PFNs [0x3fffe0000 0x3ffff0000) >> [ 106.838655] HMM test module loaded. This is only for testing HMM. >> TAP version 13 >> 1..20 >> # Starting 20 tests from 3 test cases. >> # RUN hmm.open_close ... >> # OK hmm.open_close >> ok 1 hmm.open_close >> # RUN hmm.anon_read ... >> # Too many test steps (255)!? >> # OK hmm.anon_read > > Oooh: > > #define NTIMES 256 > > Yes, that's a lot of steps. :) > > I agree,__ INC_STEP() needs adjustment, though it should be 253. Does > this work for you? > > diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h > index 935029d4fb21..4f78e4805633 100644 > --- a/tools/testing/selftests/kselftest_harness.h > +++ b/tools/testing/selftests/kselftest_harness.h > @@ -680,7 +680,8 @@ > __bail(_assert, _metadata->no_print, _metadata->step)) > > #define __INC_STEP(_metadata) \ > - if (_metadata->passed && _metadata->step < 255) \ > + /* Keep "step" below 255 (which is used for "SKIP" reporting). */ \ > + if (_metadata->passed && _metadata->step < 253) \ > _metadata->step++; > > #define is_signed_type(var) (!!(((__typeof__(var))(-1)) < (__typeof__(var))1)) > @@ -976,12 +977,6 @@ void __run_test(struct __fixture_metadata *f, > t->passed = 0; > } else if (t->pid == 0) { > t->fn(t, variant); > - /* Make sure step doesn't get lost in reporting */ > - if (t->step >= 255) { > - ksft_print_msg("Too many test steps (%u)!?\n", t->step); > - t->step = 254; > - } > - /* Use 255 for SKIP */ > if (t->skip) > _exit(255); > /* Pass is exit 0 */ >
Yes, this works for me. Thanks!
| |