From ec57ad7e1232edc7468827ed51b91039bd991203 Mon Sep 17 00:00:00 2001 From: Alexey Sachkov Date: Fri, 11 Oct 2024 13:52:05 +0200 Subject: [PATCH] [SYCL][E2E] Track amount of `XFAIL`ed tests (#15603) We have a problem with our approach to `XFAIL`ing failed tests - we don't always submit trackers for those failures, thus simply hiding them from us. Some of tests we `XFAIL`ed have been in that state for years and without a tracker submitted about them, we don't even know that there are issues. This patch introduces a new requirement for marking test as expected to fail: every `XFAIL` directive has to be followed by `XFAIL-TRACKER` which points to a public or internal issue submitted to analyze the failure and remove `XFAIL` directive. To ensure that the new requirement is followed, a new test was added which checks amount of improper `XFAIL` directives (i.e. those which are not followed by `XFAIL-TRACKER` directive). Similar approach is expected to be applied later for `UNSUPPORTED` directives, but it will be done as a separate PR. --------- Co-authored-by: Marcos Maronas --- sycl/test-e2e/README.md | 24 +++++++++ .../normalized-clampedge-linear-float.cpp | 1 + .../Sampler/normalized-clampedge-nearest.cpp | 1 + sycl/test-e2e/no-xfail-without-tracker.cpp | 50 +++++++++++++++++++ 4 files changed, 76 insertions(+) create mode 100644 sycl/test-e2e/no-xfail-without-tracker.cpp diff --git a/sycl/test-e2e/README.md b/sycl/test-e2e/README.md index 44f96af580f7..e04ebdc6d957 100644 --- a/sycl/test-e2e/README.md +++ b/sycl/test-e2e/README.md @@ -320,3 +320,27 @@ implementation header files is still in progress and the final set of these "fine-grained" includes that might be officially documented and suggested for customers to use isn't determined yet. **Until then, code outside of this project must keep using `` provided by the SYCL2020 specification.** + +## Marking tests as expected to fail + +Every test should be written in a way that it is either passed, or it is skipped +(in case it is not compatible with an environment it was launched in). + +If for any reason you find yourself in need to temporary mark test as expected +to fail under certain conditions, you need to submit an issue to the repo to +analyze that failure and make test passed or skipped. + +Once the issue is created, you can update the test by adding `XFAIL` and +`XFAIL-TRACKER` directive: +``` +// GPU driver update caused failure +// XFAIL: level_zero +// XFAIL-TRACKER: PRJ-5324 + +// Sporadically fails on CPU: +// XFAIL: cpu +// XFAIL-TRACKER: https://github.com/intel/llvm/issues/DDDDD +``` + +If you add `XFAIL` without `XFAIL-TRACKER` directive, +`no-xfail-without-tracker.cpp` test will fail, notifying you about that. diff --git a/sycl/test-e2e/Sampler/normalized-clampedge-linear-float.cpp b/sycl/test-e2e/Sampler/normalized-clampedge-linear-float.cpp index 50ae3c6916d1..0322f317db0d 100644 --- a/sycl/test-e2e/Sampler/normalized-clampedge-linear-float.cpp +++ b/sycl/test-e2e/Sampler/normalized-clampedge-linear-float.cpp @@ -3,6 +3,7 @@ // RUN: %{build} -o %t.out // RUN: %{run} %t.out // XFAIL: hip +// XFAIL-TRACKER: https://github.com/intel/llvm/issues/14732 // CUDA works with image_channel_type::fp32, but not with any 8-bit per channel // type (such as unorm_int8) diff --git a/sycl/test-e2e/Sampler/normalized-clampedge-nearest.cpp b/sycl/test-e2e/Sampler/normalized-clampedge-nearest.cpp index 381ed808001a..6f349254dda5 100644 --- a/sycl/test-e2e/Sampler/normalized-clampedge-nearest.cpp +++ b/sycl/test-e2e/Sampler/normalized-clampedge-nearest.cpp @@ -5,6 +5,7 @@ // Missing __spirv_ImageWrite, __spirv_SampledImage, // __spirv_ImageSampleExplicitLod on AMD // XFAIL: hip_amd +// XFAIL-TRACKER: https://github.com/intel/llvm/issues/14732 /* This file sets up an image, initializes it with data, diff --git a/sycl/test-e2e/no-xfail-without-tracker.cpp b/sycl/test-e2e/no-xfail-without-tracker.cpp new file mode 100644 index 000000000000..54da766fe9fc --- /dev/null +++ b/sycl/test-e2e/no-xfail-without-tracker.cpp @@ -0,0 +1,50 @@ +// This test is intended to ensure that we have no trackers marked as XFAIL +// without a tracker information added to a test. +// +// The format we check is: +// XFAIL: lit,features +// XFAIL-TRACKER: [GitHub issue URL|Internal tracker ID] +// +// GitHub issue URL format: +// https://github.com/owner/repo/issues/12345 +// +// Internal tracker ID format: +// PROJECT-123456 +// +// REQUIRES: linux +// +// Explanation of the command: +// - search for all "XFAIL" occurrences, display line with match and the next one +// -I, --include to drop binary files and other unrelated files +// - in the result, search for "XFAIL" again, but invert the result - this +// allows us to get the line *after* XFAIL +// - in those lines, check that XFAIL-TRACKER is present and correct. Once +// again, invert the search to get all "bad" lines +// - make a final count of how many ill-formatted directives there are and +// verify that against the reference +// +// RUN: grep -rI "XFAIL:" %S -A 1 --include=*.c --include=*.cpp \ +// RUN: --no-group-separator | \ +// RUN: grep -v "XFAIL:" | \ +// RUN: grep -Pv "XFAIL-TRACKER:\s+(?:https://github.com/[\w\d-]+/[\w\d-]+/issues/[\d]+)|(?:[\w]+-[\d]+)" | \ +// RUN: wc -l | FileCheck %s --check-prefix NUMBER-OF-XFAIL-WITHOUT-TRACKER +// +// The number below is a number of tests which are *improperly* XFAIL-ed, i.e. +// we either don't have a tracker associated with a failure listed in those +// tests, or it is listed in a wrong format. +// Note: strictly speaking, that is not amount of files, but amount of XFAIL +// directives. If a test contains several XFAIL directives, some of them may be +// valid and other may not. +// +// That number *must not* increase. Any PR which causes this number to grow +// should be rejected and it should be updated to either keep the number as-is +// or have it reduced (preferrably, down to zero). +// +// If you see this test failed for your patch, it means that you either +// introduced XFAIL directive to a test improperly, or broke the format of an +// existing XFAIL-ed tests. +// Another possibility (and that is a good option) is that you updated some +// tests to match the required format and in that case you should just update +// (i.e. reduce) the number below. +// +// NUMBER-OF-XFAIL-WITHOUT-TRACKER: 187