[SYCL][Unit Tests] Fix regression after https://github.com/intel/llvm/pull/19687 #20534
Conversation
This patch fixes a regression happened after intel#19687: `check-all` lost the entire SYCL-Unit test suite, so SYCL unit tests did not run for `check-all` target.
dm-vodopyanov
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
sycl/test/Unit/lit.cfg.py
Outdated
|
|
||
| # 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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also Option 3 exists: refactor our CMake files so that even for non_preview the test binary name suffix is still Tests
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm also thinking to completely revert #19687 - this looks even safer and consistent...
There was a problem hiding this comment.
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
dm-vodopyanov
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
dm-vodopyanov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
dm-vodopyanov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
dm-vodopyanov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| # the SYCL preview features enabled. | ||
| # Produces two binaries, named `basename(test_name_prefix_non_preview)` and `basename(test_name_prefix_preview)` | ||
| # Produces two binaries, named `basename(test_name_prefix_Non_Preview_Tests)` and `basename(test_name_prefix_Preview_Tests)` | ||
| macro(add_sycl_unittest test_name_prefix link_variant) | ||
| add_sycl_unittest_internal(${test_name_prefix}_non_preview ${link_variant} FALSE ${ARGN}) | ||
| add_sycl_unittest_internal(${test_name_prefix}_preview ${link_variant} TRUE ${ARGN}) | ||
| add_sycl_unittest_internal(${test_name_prefix}_Non_Preview_Tests ${link_variant} FALSE ${ARGN}) | ||
| add_sycl_unittest_internal(${test_name_prefix}_Preview_Tests ${link_variant} TRUE ${ARGN}) | ||
| endmacro() |
There was a problem hiding this comment.
Was the regression because we assume Tests suffix after each test?
dm-vodopyanov
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
dm-vodopyanov
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
| #include <regex> | ||
|
|
||
| TEST(ConfigTests, CheckConfigProcessing) { | ||
| TEST(ConfigTests, DISABLED_CheckConfigProcessing) { |
There was a problem hiding this comment.
Are all the disabled unittests failing because they set some environment variable which gets unintentionally modified when we run preview and non-preview unittests parallelly?
IIRC, preview and non-preview unit tests are in different folders so there shouldn't be any race on filesystem...
Btw, in CI, when we run check-sycl-unitests, we run both preview and non-preview unittests and we don't see any failure there.
There was a problem hiding this comment.
What's the difference in our setup when we run unit tests via check-sycl-unitests and check-sycl? It's weird that unitests are failing with one but not another.
4 participants
This patch fixes a regression happened after #19687:
check-alllost the entire SYCL-Unit test suite, so SYCL unit tests did not run forcheck-alltarget.