[Impeller] fail CI if validations are enabled but not available. #51378

jonahwilliams · 2024-03-13T18:15:21Z

Will test this by unsetting the VVL configuration

gaaclarke · 2024-03-13T18:19:15Z

impeller/renderer/backend/vulkan/context_vk_unittests.cc

@@ -222,5 +222,16 @@ TEST(ContextVKTest, WarmUpFunctionCreatesRenderPass) {
                        "vkCreateRenderPass") != functions->end());
 }

+TEST(ContextVKTest, FatalMissingValidations) {


Is this compiled into impeller_golden_tests? We should have one of these for that target too.

This is just a check that the setting works. I've enabled this setting for playgrounds, so I should be able to confirm that removing any of the VVL config causes failures.

What I meant is that we want to make sure that impeller_golden_tests --gtest_filter=ContextVKTest.FatalMissingValidations is executed too. I suspect this will execute for impeller_tests but not impeller_golden_tests.

That doesn't matter though, that isn't what is running the check.

its just a test that the check works

its just a test that the check works

I know =)

that isn't what is running the check.

I know, I want this test running in both executables. In impeller_unittests and impeller_golden_tests we should fail if we don't have vulkan validation.

I looked into this more. It doesn't seem possible for me to set up a playground where validations are requested but still misisng, without mocking out the loader. Did you have a different idea here?

gaaclarke · 2024-03-13T18:20:17Z

impeller/playground/backend/vulkan/playground_impl_vk.cc

@@ -88,6 +88,9 @@ PlaygroundImplVK::PlaygroundImplVK(PlaygroundSwitches switches)
  context_settings.shader_libraries_data = ShaderLibraryMappingsForPlayground();
  context_settings.cache_directory = fml::paths::GetCachesDirectory();
  context_settings.enable_validation = switches_.enable_vulkan_validation;
+  context_settings.fatal_missing_validations =


Where is this getting set to true for the test runner?

IIRC both goldens and regular playground tests go through this path to create the context.

I'm not sure. I'd set a breakpoint in the impeller_golden_tests here to make sure. We swap out playground tests for golden tests in that target so this might not be getting hit. That's why we should make sure we have the test for that executable too (as mentioned in the other comment).

Both impeller unittests and goldens have failed on the commit where I broke run_tests.py:

https://github.com/flutter/engine/pull/51378/checks?check_run_id=22626102272

I think that means its working.

The golden test runner failed from a flake:

[ FAILED ] FlutterEngineTest.CanOverrideBackgroundColor 1 FAILED TEST

No its in there: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8753556488722177297/+/u/test:_Impeller-golden__dart_and_engine_tests_for_host_release/stdout

It did hit a flake during one of the reruns though.

That failure is when executing impeller_unittests. Let's GVC when you have a chance. I think I can clear this up in under 5 minutes over gvc =)

matanlurey

In general the change looks good to me, but defer to Aaron on the comments he's left already.

gaaclarke · 2024-03-14T15:47:11Z

In general the change looks good to me, but defer to Aaron on the comments he's left already.

We had a vulkan mind meld yesterday and we're of one minds here. My suggestion is to get this working for the impeller_golden_tests executable too. If it is involved, filing an issue and addressing in a separate PR is fine.

jonahwilliams · 2024-03-14T20:36:26Z

okay ... I can't figure out how to do this in the testbed itself.

gaaclarke

lgtm modulo filing an issue for golden test runner and verifying that we aren't getting undefined behavior in EXPECTED_DEATH.

gaaclarke · 2024-03-19T15:52:51Z

impeller/renderer/backend/vulkan/context_vk_unittests.cc

@@ -222,5 +222,16 @@ TEST(ContextVKTest, WarmUpFunctionCreatesRenderPass) {
                        "vkCreateRenderPass") != functions->end());
 }

+TEST(ContextVKTest, FatalMissingValidations) {
+  EXPECT_DEATH(const std::shared_ptr<ContextVK> context =


Are you getting a warning about running this in a multithreaded environment? This works by forking and catching signals which won't work reliably on multi threaded processes.

Not running locally

Is there a cmd line flag I can test this locally with?

The tests that you execute before your test are going to matter. If ever there is any test that can spawn a thread and not join it, like creating a thread pool, that run before this test it will be a problem. I'd just try running the test runner with every test enabled. Maybe try enabling the test shuffle a few times and seeing if it prints out the error. I almost guarantee there is at least one test that is leaking threads though. That's what I was seeing with the golden test runner at least.

gtest parallel runs multiple gtest processes, if one or more of those process spawn a subprocess that shouldn't cause any issues. The problems we've had usually stem from reading/writing to the file system.

The problem isn't from running the tests in multiple processes, it comes from the signal function. Which thread will handle a signal is undefined. That's what's being used to catch the death of the process. I'm pretty sure that's why googletest prints out the warning. If you don't see the warning and want to give it a shot that sounds good to me. We should add a comment to the test in case it ever starts flaking.

chinmaygarde · 2024-03-28T20:05:13Z

I can't tell if we are still making progress on this.

jonahwilliams · 2024-03-29T00:57:28Z

I am waiting on follow up from review from either @chinmaygarde , @gaaclarke or @matanlurey . I cannot make the requested additional test (i don't know how), so either we can land this with tweaks or I can close the PR.

chinmaygarde

So, per my understanding, the ask is to request mocking out the loader to simulate the fault?

I think the patch in this state is an improvement over the current state of things and I'm fine landing this to make forward progress.

jonahwilliams · 2024-03-29T01:07:02Z

Right, I need to mock out the failure and mock out parts of the playground harness. I don't think it would be a great test.

matanlurey · 2024-03-29T01:44:04Z

Apologies for missing this.

Wanna sync for a few minutes tomorrow morning? Either I can convince you there is a reasonable way to test this or concede let's land it as-is.

matanlurey · 2024-03-29T01:44:24Z

You're probably right btw, it's more me wanting to understand the playground harness more.

chinmaygarde · 2024-04-04T20:04:07Z

Was there a sync on this? This feels like a generally valuable thing to have.

jonahwilliams · 2024-04-04T21:13:52Z

There was not. I maintain that 1) the test I am adding is useful and 2) I do not know how to add the second test that Is being requested. I have no plans to work on this further.

matanlurey · 2024-04-05T00:40:46Z

I think this is useful as-is. Let's land it.

gaaclarke

I validated that the warning isn't being printed out for the 2 instances it's run here: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8751539136032949377/+/u/test:_Host_Tests_for_host_debug/stdout

I would still prefer a comment in case the sheriff runs into flakes otherwise lgtm.

…able. (flutter/engine#51378)

…146357) flutter/engine@d048b9a...62df3bd 2024-04-05 jonahwilliams@google.com [Impeller] fail CI if validations are enabled but not available. (flutter/engine#51378) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC matanl@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#146357) flutter/engine@d048b9a...62df3bd 2024-04-05 jonahwilliams@google.com [Impeller] fail CI if validations are enabled but not available. (flutter/engine#51378) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC matanl@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

[Impeller] fail CI if validations are enabled but not available.

82b4fad

jonahwilliams requested review from gaaclarke and matanlurey March 13, 2024 18:15

github-actions bot added the e: impeller label Mar 13, 2024

gaaclarke reviewed Mar 13, 2024

View reviewed changes

Oh no I broke runtests.py with a refactor.

cb489e4

matanlurey reviewed Mar 13, 2024

View reviewed changes

++

eee1102

Merge branch 'main' into fail_if_no_validation

85f1e7c

jonahwilliams requested a review from gaaclarke March 14, 2024 20:39

Merge branch 'main' into fail_if_no_validation

c588660

jonahwilliams requested a review from chinmaygarde March 19, 2024 15:49

gaaclarke reviewed Mar 19, 2024

View reviewed changes

chinmaygarde approved these changes Mar 29, 2024

View reviewed changes

matanlurey approved these changes Apr 5, 2024

View reviewed changes

Merge branch 'main' into fail_if_no_validation

1309a27

gaaclarke approved these changes Apr 5, 2024

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 5, 2024

auto-submit bot merged commit 62df3bd into flutter:main Apr 5, 2024

engine-flutter-autoroll mentioned this pull request Apr 5, 2024

Roll Flutter Engine from d048b9aed529 to 62df3bd5f681 (1 revision) flutter/flutter#146357

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 5, 2024

62df3bd5f [Impeller] fail CI if validations are enabled but not avail…

1d8ec8f

…able. (flutter/engine#51378)

[Impeller] fail CI if validations are enabled but not available. #51378

[Impeller] fail CI if validations are enabled but not available. #51378

Uh oh!

Conversation

jonahwilliams commented Mar 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matanlurey left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Mar 14, 2024

Uh oh!

jonahwilliams commented Mar 14, 2024

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Mar 28, 2024

Uh oh!

jonahwilliams commented Mar 29, 2024

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Mar 29, 2024

Uh oh!

matanlurey commented Mar 29, 2024

Uh oh!

matanlurey commented Mar 29, 2024

Uh oh!

chinmaygarde commented Apr 4, 2024

Uh oh!

jonahwilliams commented Apr 4, 2024

Uh oh!

matanlurey commented Apr 5, 2024

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!