Skip to content

Conversation

@KornevNikita
Copy link
Contributor

@KornevNikita KornevNikita commented Oct 31, 2024

To unify the mechanism to disable tests this patch replaces REQUIRES: TEMPORARY_DISABLED by UNSUPPORTED: true to disable the test. Also updating the tests using "TEMPORARY_DISABLED".

To completely disable the test "REQUIRES: TEMPORARY_DISABLED" is used.
There can be typos, lower-case style, etc, so the disabled test may be missed
by grep. To unify the mechanism to disable tests this patch adds new
features for `UNSUPPORTED` to disable the test. Also updating the tests
using "TEMPORARY_DISABLED".
@KornevNikita KornevNikita force-pushed the extend-unsupported-features branch from ddd9dca to 27083fc Compare November 4, 2024 11:56
@KornevNikita KornevNikita marked this pull request as ready for review November 4, 2024 11:56
@KornevNikita KornevNikita requested review from a team as code owners November 4, 2024 11:56
@KornevNikita KornevNikita requested a review from againull November 4, 2024 11:56
@bader
Copy link
Contributor

bader commented Nov 4, 2024

@KornevNikita, I'm not sure I understand the problem you are trying to solve with this change.

the disabled test may be missed by grep

Did I get it right that you are using grep to find disabled tests? If so, please, try using llvm-lit instead of grep tool. Here are useful command line options:

--show-excluded Show excluded tests.
--show-skipped Show skipped tests.
--show-unsupported Show unsupported tests.
--show-used-features Show all features used in the test suite (in XFAIL, UNSUPPORTED and REQUIRES) and exit.

Full list of options: https://llvm.org/docs/CommandGuide/lit.html

@KornevNikita
Copy link
Contributor Author

@KornevNikita, I'm not sure I understand the problem you are trying to solve with this change.

the disabled test may be missed by grep

Did I get it right that you are using grep to find disabled tests? If so, please, try using llvm-lit instead of grep tool. Here are useful command line options:

--show-excluded Show excluded tests. --show-skipped Show skipped tests. --show-unsupported Show unsupported tests. --show-used-features Show all features used in the test suite (in XFAIL, UNSUPPORTED and REQUIRES) and exit.

Full list of options: https://llvm.org/docs/CommandGuide/lit.html

The main idea is to unify the mechanism to disable tests, as this "TEMPORARY_DISABLED" is non-standard, but some work-around. We could just switch it to UNSUPPORTED: * - but it doesn't accept the asterisk.

@bader
Copy link
Contributor

bader commented Nov 4, 2024

@KornevNikita, I'm not sure I understand the problem you are trying to solve with this change.

the disabled test may be missed by grep

Did I get it right that you are using grep to find disabled tests? If so, please, try using llvm-lit instead of grep tool. Here are useful command line options:
--show-excluded Show excluded tests. --show-skipped Show skipped tests. --show-unsupported Show unsupported tests. --show-used-features Show all features used in the test suite (in XFAIL, UNSUPPORTED and REQUIRES) and exit.
Full list of options: https://llvm.org/docs/CommandGuide/lit.html

The main idea is to unify the mechanism to disable tests, as this "TEMPORARY_DISABLED" is non-standard, but some work-around. We could just switch it to UNSUPPORTED: * - but it doesn't accept the asterisk.

From my POV, replacing a single way (i.e. REQUIRES: TEMPORARY_DISABLED) to disable a LIT test with 3 new ways (i.e. UNSUPPORTED: overall, flaky, hangs) doesn't make it standard. It's still a temporary work-around of some test issues.

We could just switch it to UNSUPPORTED: * - but it doesn't accept the asterisk.

Sorry, it looks like the right way to use UNSUPPORTED: true.

@KornevNikita
Copy link
Contributor Author

KornevNikita commented Nov 4, 2024

Sorry, it looks like the right way to use UNSUPPORTED: true.

Didn't think about that. If this works I'd prefer to proceed with it.

@bader
Copy link
Contributor

bader commented Nov 4, 2024

Sorry, it looks like the right way to use UNSUPPORTED: true.

Didn't think about that. If this works I'd prefer to proceed with it.

It's used by a few tests in llvm-project.

compiler-rt/test/builtins/Unit/gcc_personality_test.c

// FIXME: UNSUPPORTED as currently it cannot be built by lit properly.
// UNSUPPORTED: true

Other uses are in bolt/test/X86/reader-stale-yaml-std.test and bolt/test/X86/dwarf4-ftypes-dwp-input-dwp-output.test.

@KornevNikita
Copy link
Contributor Author

@bader thanks for pointing out! I'll update the PR

@KornevNikita
Copy link
Contributor Author

@intel/dpcpp-esimd-reviewers could you please take a look?

@KornevNikita KornevNikita changed the title [SYCL][E2E] Extend UNSUPPORTED [SYCL][E2E] Replace TEMPORARY_DISABLED by UNSUPPORTED Nov 12, 2024
Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, was OOO. lgtm!

@KornevNikita
Copy link
Contributor Author

@intel/llvm-gatekeepers could you please merge?

@sarnex sarnex merged commit 75f476d into intel:sycl Nov 12, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants