Skip to content

[SYCL][Unit Tests] Fix regression after https://github.com/intel/llvm/pull/19687 #20534

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Nov 6, 2025
Merged

[SYCL][Unit Tests] Fix regression after https://github.com/intel/llvm/pull/19687 #20534

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
194e20b
[SYCL][Unit Tests] Fix regression after https://github.com/intel/llvm…
dm-vodopyanov Nov 3, 2025
360c60e
Fix clang format
dm-vodopyanov Nov 3, 2025
c8999d6
Update
dm-vodopyanov Nov 4, 2025
8d543d7
Update
dm-vodopyanov Nov 5, 2025
69b4775
Update
dm-vodopyanov Nov 5, 2025
02ae68a
Update
dm-vodopyanov Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion sycl/test/Unit/lit.cfg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import os
import platform
import subprocess
import sys

import lit.formats
import lit.util
Expand All @@ -21,7 +22,15 @@
config.test_source_root = config.test_exec_root

# testFormat: The test format to use to interpret tests.
config.test_format = lit.formats.GoogleTest(config.llvm_build_mode, "Tests")
config.test_format = lit.formats.GoogleTest(config.llvm_build_mode, test_suffix="Tests_non_preview")
Copy link
Contributor Author

@dm-vodopyanov dm-vodopyanov Nov 3, 2025

Choose a reason for hiding this comment

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

Not sure what is better:
Option 1: add a workaroud here as it is now: we call the existing ctor, and when modify attribute self.test_suffixes
Option 2: modify llvm/llvm/utils/lit/lit/formats/googletest.py but it will be a modification of common LLVM sources, so if community changes something there, it will break on our end. Modification means add a ctor which takes list of test suffixes. I think suggesting the patch to the community is not an option as we need to fix this very fast.

Copy link
Contributor

@steffenlarsen steffenlarsen Nov 3, 2025

Choose a reason for hiding this comment

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

I think I prefer a combination of the two: Go with Option 1 for now, then open a change to upstream LLVM with the modifications in Option 2. When it is merged upstream and it reaches intel/llvm we can drop the workaround.

Copy link
Contributor

@AlexeySachkov AlexeySachkov Nov 3, 2025

Choose a reason for hiding this comment

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

Also Option 3 exists: refactor our CMake files so that even for non_preview the test binary name suffix is still Tests

Copy link
Contributor Author

@dm-vodopyanov dm-vodopyanov Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As pre-commit shows, it looks like it is better to separate non_preview and preview tests by dividing SYCL-Unit into SYCL-Unit-Preview and SYCL-Unit-Non-Preview as the preview and non-preview variations of the same test can run simultaneously and we can get a data race.

Copy link
Contributor Author

@dm-vodopyanov dm-vodopyanov Nov 4, 2025

Choose a reason for hiding this comment

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

In c8999d6: kept single test_prefix (Tests). Test names started to look ugly, like SchedulerTests_Non_Preview_Tests, but I think it could be temporary. Also disabled tests which used the same external recourses and as a result failed due to data races. These tests should be re-written. I will create a ticket against SYCL RT team and add the ticket number to the test sources.

Copy link
Contributor

@uditagarwal97 uditagarwal97 Nov 4, 2025

Choose a reason for hiding this comment

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

Rather than completely disabling unit tests for testing preview breaking changes, you could do:

config.test_format = lit.formats.GoogleTest(config.llvm_build_mode, "Non_Preview_Tests")

in sycl/test/Unit/lit.cfg.py - so that we run only non-preview unitests as a part of check-sycl/check-all. This won't reduce testing coverage as we'd still be running preview version of SYCL unittests in intel/llvm CI as part of ninja check-sycl-unittests

Copy link
Contributor Author

@dm-vodopyanov dm-vodopyanov Nov 5, 2025

Choose a reason for hiding this comment

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

@uditagarwal97 But what about our feature board? Let's imagine the situation that we need to see the results with preview version of library in it. In that case they won't be there.

Copy link
Contributor

@AlexeySachkov AlexeySachkov Nov 5, 2025

Choose a reason for hiding this comment

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

@uditagarwal97 But what about our feature board? Let's imagine the situation that we need to see the results with preview version of library in it. In that case they won't be there.

Why would we need such results? Either we can't break ABI meaning that our main goal is to cover regular path with tests, or we can break ABI in which case let's just break it and just have one code path instead of two

Copy link
Contributor Author

@dm-vodopyanov dm-vodopyanov Nov 5, 2025

Choose a reason for hiding this comment

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

@uditagarwal97 But what about our feature board? Let's imagine the situation that we need to see the results with preview version of library in it. In that case they won't be there.

Why would we need such results? Either we can't break ABI meaning that our main goal is to cover regular path with tests, or we can break ABI in which case let's just break it and just have one code path instead of two

OK. Let's include only non-preview tests to check-all as part of this specific PR.

Copy link
Contributor Author

@dm-vodopyanov dm-vodopyanov Nov 5, 2025

Choose a reason for hiding this comment

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

Ready to review.

# Workaround, as lit.formats.GoogleTest's ctor takes only single string as test suffix
# and does not support a list of test suffixes.
test_suffix_preview = "Tests_preview"
kIsWindows = sys.platform in ["win32", "cygwin"]
if kIsWindows:
test_suffix_preview += ".exe"
config.test_format.test_suffixes.add(test_suffix_preview)
# End of workaround.

# Propagate the temp directory. Windows requires this because it uses \Windows\
# if none of these are present.
Expand Down
Loading