Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL][E2E] Replace TEMPORARY_DISABLED by UNSUPPORTED #15946

Merged
merged 5 commits into from
Nov 12, 2024

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".
sycl/test-e2e/README.md Outdated Show resolved Hide resolved
@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

sycl/test-e2e/README.md Outdated Show resolved Hide resolved
@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