Skip to content

Commit

Permalink
[SYCL][E2E] Track amount of XFAILed tests (#15603)
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
AlexeySachkov and maarquitos14 authored Oct 11, 2024
1 parent c9e4676 commit ec57ad7
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 0 deletions.
24 changes: 24 additions & 0 deletions sycl/test-e2e/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<sycl/sycl.hpp>` 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.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions sycl/test-e2e/Sampler/normalized-clampedge-nearest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
50 changes: 50 additions & 0 deletions sycl/test-e2e/no-xfail-without-tracker.cpp
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit ec57ad7

Please sign in to comment.