-
Notifications
You must be signed in to change notification settings - Fork 645
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
[runtime][vulkan][cts] Disable flaky test WaitForFiniteTime on Android #17246
[runtime][vulkan][cts] Disable flaky test WaitForFiniteTime on Android #17246
Conversation
This test is flaky on Google Pixel 6 Pro. On other Vulkan backends it does not fail. There is high likelihood that the Vulkan implementation on Pixel 6 Pro is broken.
unset(ADDITIONAL_TEST_ARGS) | ||
if(ANDROID) | ||
# Disable this test due to flaky failures on Google Pixel 6 Pro. | ||
set(ADDITIONAL_TEST_ARGS "--gtest_filter=-*WaitForFiniteTime*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also (by convention) prefix private variables with an underscore, like _ADDITIONAL_TEST_ARGS
. There is the expectation there that using such underscore-prefixed variables outside of the file they are defined is unsupported. Using unset
(or set to empty string/list) is still good practice here though, in case the variable happened to be set by another module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the doc:
Note CMake reserves identifiers that:
begin with CMAKE_ (upper-, lower-, or mixed-case), or
begin with CMAKE (upper-, lower-, or mixed-case), or
begin with _ followed by the name of any CMake Command.
Since you don't know what will be added to the CMake commands in the future the actual set of names you should avoid is all names prefixed with _
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I haven't seen that argument before. Interesting. I have seen many other projects using underscore by convention though 🤔. If we want to be extra careful we could prefix with _IREE
.
I'm fine with either way here.
@@ -4,6 +4,12 @@ | |||
# See https://llvm.org/LICENSE.txt for license information. | |||
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | |||
|
|||
unset(ADDITIONAL_TEST_ARGS) | |||
if(ANDROID) | |||
# Disable this test due to flaky failures on Google Pixel 6 Pro. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've also seen failures on Moto Edge X30: https://github.com/iree-org/iree/actions/runs/8892701894/job/24418057015#step:6:432
Could just generalize the comment to "on Android".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I improved the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. For more context, those are the two phone models that we test on CI (semi-modern devices that are in a lab at Google managed by the team there). We'd test on more devices if we had the lab capacity for it. So this might be flaky on all Android devices, or Android devices with the same family of GPU drivers/versions. We don't really know.
iree-org#17246) This test is flaky on Google Pixel 6 Pro and Moto Edge X30. On other Vulkan backends it does not fail. There is some likelihood that the Vulkan implementations on Pixel 6 Pro and Moto Edge X30 is broken. Although, they have different GPUs. Adreno 730 and Mali-G78 MP20 respectively. Signed-off-by: Lubo Litchev <lubol@google.com>
This test is flaky on Google Pixel 6 Pro.
On other Vulkan backends it does not fail.
There is high likelihood that the Vulkan implementation on Pixel 6 Pro is broken.