Cleanup embedder vulkan unittests & Enable Vulkan unit tests on macOS #173688
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request aims to clean up the embedder Vulkan unit tests and enable them on macOS. While the configuration changes in config.gni correctly enable the Vulkan tests, a change in embedder_gl_unittests.cc seems to inadvertently remove a large number of parameterized tests for the Vulkan backend, which would reduce test coverage. I've left a critical comment highlighting this potential issue. Additionally, I've made a suggestion to improve a test suite name for better code clarity and maintainability, in line with the cleanup goal of this PR.
| @@ -5187,8 +5185,7 @@ TEST_F(EmbedderTest, CompositorMustBeAbleToRenderKnownSceneToOpenGLSurfaces) { | |||
| INSTANTIATE_TEST_SUITE_P( | |||
| EmbedderTestGlVk, | |||
| EmbedderTestMultiBackend, | |||
| ::testing::Values(EmbedderTestContextType::kOpenGLContext, | |||
| EmbedderTestContextType::kVulkanContext)); | |||
| ::testing::Values(EmbedderTestContextType::kOpenGLContext,)); | |||
There was a problem hiding this comment.
Removing kVulkanContext from this test suite disables a significant number of parameterized tests (TEST_P) for the Vulkan backend, including CanRenderGradientWithoutCompositor, CanRenderGradientWithCompositor, etc. This results in a significant reduction of Vulkan test coverage, as these tests don't appear to be instantiated for Vulkan elsewhere.1
To maintain test coverage, these tests should be instantiated for the Vulkan backend, perhaps in embedder_vk_unittests.cc.
Style Guide References
Footnotes
| @@ -5187,8 +5185,7 @@ TEST_F(EmbedderTest, CompositorMustBeAbleToRenderKnownSceneToOpenGLSurfaces) { | |||
| INSTANTIATE_TEST_SUITE_P( | |||
| EmbedderTestGlVk, | |||
There was a problem hiding this comment.
Now that this test suite instantiation only includes the OpenGL context, the name EmbedderTestGlVk is misleading. Please consider renaming it to EmbedderTestGl to accurately reflect its contents.1
EmbedderTestGl,Style Guide References
Footnotes
There was a problem hiding this comment.
Gemini's suggestions are good.
The linking error in the failing CI steps seem apropos to this change: If you don't think so you should try rebasing the PR.
ld64.lld: error: undefined symbol: GrBackendTextures::MakeVk(int, int, GrVkImageInfo const&, std::_fl::basic_string_view<char, std::_fl::char_traits<char>>)
>>> referenced by embedder.cc:1321 (../../../flutter/shell/platform/embedder/embedder.cc:1321)
>>> obj/flutter/shell/platform/embedder/embedder_as_internal_library.embedder.o:(symbol std::_fl::__function::__func<InferExternalViewEmbedderFromArgs(FlutterCompositor const*, bool)::$_0, std::_fl::allocator<InferExternalViewEmbedderFromArgs(FlutterCompositor const*, bool)::$_0>, std::_fl::unique_ptr<flutter::EmbedderRenderTarget, std::_fl::default_delete<flutter::EmbedderRenderTarget>> (GrDirectContext*, std::_fl::shared_ptr<impeller::AiksContext> const&, FlutterBackingStoreConfig const&)>::operator()(GrDirectContext*&&, std::_fl::shared_ptr<impeller::AiksContext> const&, FlutterBackingStoreConfig const&)+0x7be)
ld64.lld: error: undefined symbol: GrDirectContexts::MakeVulkan(skgpu::VulkanBackendContext const&, GrContextOptions const&)
>>> referenced by embedder_surface_vulkan.cc:173 (../../../flutter/shell/platform/embedder/embedder_surface_vulkan.cc:173)
>>> obj/flutter/shell/platform/embedder/embedder_as_internal_library.embedder_surface_vulkan.o:(symbol flutter::EmbedderSurfaceVulkan::CreateGrContext(VkInstance_T*, unsigned int, unsigned long, char const**, unsigned long, char const**, flutter::ContextType) const+0x3e9)
ld64.lld: error: undefined symbol: skgpu::VulkanExtensions::init(std::_fl::function<void (* (char const*, VkInstance_T*, VkDevice_T*))()>, VkInstance_T*, VkPhysicalDevice_T*, unsigned int, char const* const*, unsigned int, char const* const*)
>>> referenced by embedder_surface_vulkan.cc:165 (../../../flutter/shell/platform/embedder/embedder_surface_vulkan.cc:165)
>>> obj/flutter/shell/platform/embedder/embedder_as_internal_library.embedder_surface_vulkan.o:(symbol flutter::EmbedderSurfaceVulkan::CreateGrContext(VkInstance_T*, unsigned int, unsigned long, char const**, unsigned long, char const**, flutter::ContextType) const+0x37d)
|
Thanks for the review! Since this PR involves deeper engine-level Vulkan work, and I’m still learning the Flutter engine, I think it’s best to temporarily close this PR. I’m going to focus on areas I feel more comfortable with and come back to this once I have a better understanding of engine-level contributions. I’ve learned a lot from this experience and really appreciate the feedback. I hope to revisit this PR in the future after I’ve gained more knowledge in this area. Thanks again |
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.