Only import the needed plugins in unit tests by lorem--ipsum · Pull Request #66751 · metabase/metabase

lorem--ipsum · 2025-12-10T08:58:59Z

In this PR, bulk setupEnterprisePlugins calls are replaced with more targeted calls to setupEnterpriseOnlyPlugin.
This demonstrates and quantifies how importing specific plugins in unit tests improves performance, but also really helps understanding what depends on which plugins.

Before:

Benchmark Results:
Runs: 10
Average: 198711.20ms
Min: 178353ms
Max: 242729ms

After:

Benchmark Results:
Runs: 10
Average: 110234.00ms
Min: 109427ms
Max: 111105ms

Command executed through a loop:

npx jest selectors/whitelabel/tests/premium.unit.spec.ts home/components/HomeHelpCard/tests/premium.unit.spec.tsx browse/metrics/tests/premium.unit.spec.tsx search/components/SearchSidebar/tests/premium.unit.spec.tsx admin/permissions/pages/CollectionPermissionsPage/tests/enterprise.unit.spec.tsx admin/settings/components/EmbeddingSettings/EmbeddingSettings/tests/simple-embedding.unit.spec.tsx query_builder/components/template_tags/TagEditorHelp/tests/premium.unit.spec.ts query_builder/components/view/PreviewQueryModal/tests/premium.unit.spec.tsx common/components/Pickers/QuestionPicker/components/QuestionPicker.unit.spec.tsx common/components/EntityPicker/components/ResultItem/ResultItem.unit.spec.tsx query_builder/components/SavedQuestionHeaderButton/SavedQuestionHeaderButton.unit.spec.js search/components/SearchSidebar/tests/enterprise.unit.spec.tsx query_builder/components/view/sidebars/QuestionInfoSidebar/tests/premium.unit.spec.tsx query_builder/components/view/sidebars/QuestionInfoSidebar/tests/content-verification.unit.spec.tsx dashboard/components/DashboardInfoSidebar/tests/premium.unit.spec.ts collections/components/PinnedItemCard/PinnedItemCard-ee.unit.spec.tsx entities/collections/utils.unit.spec.ts dashboard/components/DashboardSettingsSidebar/tests/premium.unit.spec.ts dashboard/components/DashboardHeader/tests/enterprise.unit.spec.ts common/components/SaveQuestionForm/context.unit.spec.tsx collections/components/CollectionHeader/tests/enterprise.unit.spec.tsx admin/settings/components/EmbeddingSettings/EmbeddingSettings/tests/enterprise.unit.spec.tsx admin/settings/components/EmbeddingSettings/EmbeddingSettings/tests/premium.unit.spec.tsx admin/settings/components/EmbeddingSettings/EmbeddingSecuritySettings/tests/enterprise.unit.spec.tsx public/components/EmbedModal/StaticEmbedSetupPane/tests/premium.unit.spec.tsx embedding/components/SharingMenu/test/DashboardSharingMenu.enterprise.unit.spec.tsx admin/people/forms/UserForm.unit.spec.tsx search/components/SearchResult/tests/SearchResult-Collections.unit.spec.tsx admin/settings/components/SettingsPages/test/AuthenticationSettingsPage.enterprise.unit.spec.tsx notifications/modals/CreateOrEditQuestionAlertModal/CreateOrEditQuestionAlertModal.unit.spec.tsx

WiNloSt · 2025-12-17T04:58:02Z

frontend/src/metabase/dashboard/components/DashboardInfoSidebar/tests/premium.unit.spec.ts

      }),
    }),
    hasEnterprisePlugins: true,
+    specificPlugins: ["collections"],


Can you help change existing tests that use a different property name? e.g.

metabase/frontend/src/embedding-sdk-bundle/components/public/dashboard/EditableDashboard/enterprise.unit.spec.tsx

Line 26 in 9fa1bdf

enterprisePlugins: ["sdk_subscriptions", "embedding"],

There are 5 of them if you search for "enterprisePlugins: ["

WiNloSt · 2025-12-17T05:00:15Z

frontend/src/metabase/dashboard/components/DashboardInfoSidebar/tests/premium.unit.spec.ts

@@ -21,6 +21,7 @@ const setupEnterprise = (opts: SetupOpts) => {
      }),
    }),
    hasEnterprisePlugins: true,


Also, I wouldn't mind not changing this as this PR is already huge. But having both flags could be confusing imo as the API isn't quite clear why we need both. I see in the implementation that if hasEnterprise:plugins: true but no specificPlugins, then we import all plugins.

I'd say doing that is unnecessary, as we mostly specify specificPlugins at the setup level, so every test would probably already have to define specificPlugins.

What do you think? Or do you think we really need 2 flags?

WiNloSt · 2025-12-17T05:05:31Z

frontend/src/metabase/dashboard/components/DashboardHeader/tests/setup.tsx

-  if (hasEnterprisePlugins) {
-    setupEnterprisePlugins();
+  if (hasEnterprisePlugins && tokenFeatures?.audit_app) {
+    setupEnterpriseOnlyPlugin("audit_app");


This looks in contrast with other tests that accept specificPlugins from the test, not in the setup function. Should we pass specificPlugins instead?

WiNloSt · 2025-12-17T05:09:23Z

frontend/src/metabase/dashboard/components/DashboardSettingsSidebar/tests/setup.tsx

+  if (tokenFeatures.audit_app) {
+    plugins.push("audit_app");


This looks incorrect. This setup function is used by frontend/src/metabase/dashboard/components/DashboardSettingsSidebar/tests/enterprise.unit.spec.ts which means we're testing enterprise build without proper token features.
And in those tests, we should activate plugins, but don't provide tokenFeatures. And this change will result in enterprise tests not activating the plugins at all.

I haven't take time to check all tests, but I think overall, we should just pass specificPlugins and that parameter alone should dictact what plugin will be activated, not inferring from token features.

…lyPlugin calls

…e fn

lorem--ipsum · 2025-12-18T13:05:23Z

...frontend/src/metabase-enterprise/caching/components/StrategyEditorForDatabases.unit.spec.tsx

 function setup(opts: SetupOpts = {}) {
  baseSetup({
-    hasEnterprisePlugins: true,
+    enterprisePlugins: "*", // TODO be more granular about this


There is some dark magic in this test file, some tests depend on each other. I haven't had the time to investigate this furthermore but that would be a good followup task.

The idea on this file as well as in other ones is to try and be specific about what plugins are required. If that's not possible at the moment, we fallback to using "*" as a way to import all plugins as before.

I think this is a good compromise 👍 * would all be acceptable, I guess, as we have no standard on how we should name a special value. Or at least, not that I know of.

iethree

Nice work!

My one concern remains that features and plugins don't necessarily map 1:1 and we could get unexpected test behavior. I think we should add a warning about this behavior to the docstring for setupEnterpriseOnlyPlugin so that people are aware of why their tests might not be behaving like they do in the real app. Maybe link to the dependency summary I generated.

github-actions · 2025-12-19T06:36:09Z

e2e tests failed on `c0c35ff7288d752cf0110f35e9082097a2f52df7-1`

e2e test run

File	Test Name
`transforms.cy.spec.ts`	scenarios > admin > transforms > creation > should be possible to create and run a Python transform
`people.cy.spec.js`	scenarios > admin > people > invite member when SSO is configured #23630

* Demo of only importing the relevant plugin in unit test * Optimize enterprise plugin unit tests with targeted setupEnterpriseOnlyPlugin calls * fixed tests * fixup * . * fixed test * removed hasEnterprisePlugin * fixup * removed remaining hasEnterprisePlugins and unnecessary setupEnterprise fn * specificPlugins -> enterprisePlugins * . * fixed type * docs

github-actions · 2025-12-19T06:53:12Z

@lorem--ipsum Manual conflict resolution is required on #67248

metabase-bot · 2025-12-19T06:54:47Z

...frontend/src/metabase-enterprise/caching/components/StrategyEditorForDatabases.unit.spec.tsx

@@ -18,7 +18,7 @@ import { createMockTokenFeatures } from "metabase-types/api/mocks";

 function setup(opts: SetupOpts = {}) {
  baseSetup({


Consider specifying the exact plugins needed instead of using "*" for better test performance and clarity about dependencies.

metabase-bot · 2025-12-19T06:54:48Z

frontend/src/metabase/collections/components/CollectionHeader/tests/enterprise.unit.spec.tsx

      entity_id: createMockEntityId("okNLSZKdSxaoG58JSQY54"),
    },
-    hasEnterprisePlugins: true,
+    enterprisePlugins: "*" as const, // TODO: specify exact plugins needed


Consider specifying the exact plugins needed instead of using "*" for better test performance and clarity about dependencies.

metabase-bot · 2025-12-19T06:54:49Z

frontend/src/metabase/collections/components/CollectionHeader/tests/enterprise.unit.spec.tsx

      can_write: false,
    },
-    hasEnterprisePlugins: true,
+    enterprisePlugins: ["audit_app" as const, "collections" as const],


Style inconsistency: The as const type assertion is used here but not in the vast majority of other enterprisePlugins arrays throughout this PR (only 3 out of 101 usages). Consider removing as const from the array elements for consistency, unless there's a specific type narrowing requirement here that's not present elsewhere.

metabase-bot · 2025-12-19T06:54:59Z

TypeScript/JavaScript Code Review

I've reviewed the TypeScript/JavaScript changes in this PR for compliance with coding standards. The changes generally follow good patterns with consistent interface definitions and proper type usage.

Issues Found:

TODO Comments: Two test files contain TODO comments requesting more granular plugin specification:
- StrategyEditorForDatabases.unit.spec.tsx:20
- CollectionHeader/tests/enterprise.unit.spec.tsx:79
Style Inconsistency: The as const type assertion is used on array elements in one location but not in the other 98 similar usages throughout the PR. This creates an inconsistency in the codebase.

Positive Observations:

Consistent refactoring pattern from hasEnterprisePlugins boolean to enterprisePlugins array
Proper TypeScript interface definitions with Parameters<typeof setupEnterpriseOnlyPlugin>[0][]
Consistent default parameter patterns (enterprisePlugins = [])
Proper conditional plugin setup with forEach loops

The changes align with the PR's goal of improving test performance through targeted plugin imports.

* Demo of only importing the relevant plugin in unit test * Optimize enterprise plugin unit tests with targeted setupEnterpriseOnlyPlugin calls * fixed tests * fixup * . * fixed test * removed hasEnterprisePlugin * fixup * removed remaining hasEnterprisePlugins and unnecessary setupEnterprise fn * specificPlugins -> enterprisePlugins * . * fixed type * docs Co-authored-by: Sébastien <sebastien@metabase.com>

github-actions · 2025-12-19T12:01:10Z

@lorem--ipsum Something went wrong while creating backport [Logs]

lorem--ipsum · 2025-12-19T12:03:37Z

@lorem--ipsum Something went wrong while creating backport [Logs]

It's okay, I just updated the label for precision but the backport has already been made and merged (which is why the action failed — there was nothing to commit).

WiNloSt · 2025-12-19T08:45:34Z

...frontend/src/metabase-enterprise/caching/components/StrategyEditorForDatabases.unit.spec.tsx

 function setup(opts: SetupOpts = {}) {
  baseSetup({
-    hasEnterprisePlugins: true,
+    enterprisePlugins: "*", // TODO be more granular about this


I think this is a good compromise 👍 * would all be acceptable, I guess, as we have no standard on how we should name a special value. Or at least, not that I know of.

WiNloSt · 2025-12-22T06:59:20Z

frontend/src/metabase/actions/components/ActionViz/ExplainerText/tests/enterprise.unit.spec.ts


 function setup(opts: SetupOpts) {
-  baseSetup({ hasEnterprisePlugins: true, ...opts });
+  baseSetup({ ...opts });


Not sure if this is correct. What I think should happen in this PR is either one of the following modifications.

removing hasEnterprisePlugins: true and replacing it with one or more plugins

removing hasEnterprisePlugins: true and replacing it with *

removing hasEnterprisePlugins: false and doing nothing as we won't be activating any plugins if not passing enterprisePlugins

This however, removes hasEnterprisePlugins: true, meaning that it won't test the case where the plugin is activated anymore it will just look like common.* test. As a reminder, enterprise.* test should test the situation where plugins are activated, but no appropriate token features are provided, so the features shouldn't be available.

WiNloSt · 2025-12-22T07:00:17Z

frontend/src/metabase/actions/components/ActionViz/ExplainerText/tests/setup.tsx

+  enterprisePlugins.forEach((plugin) => {
+    setupEnterpriseOnlyPlugin(plugin);
+  });


This is exactly what I think looks the cleanest. I just read through another variable where there's a check for the existence of enterprisePlugins. They could looks like this for consistency too.

WiNloSt · 2025-12-22T07:01:47Z

...tabase/actions/containers/ActionCreator/FormCreator/Description/test/enterprise.unit.spec.ts


 function setup(opts: SetupOpts) {
  return baseSetup({
-    hasEnterprisePlugins: true,


Here's another enterprise test that has no plugins activated. I think it should either be enterprisePlugins: ["somefeature"] or enterprisePlugins: "*"

WiNloSt · 2025-12-22T07:02:51Z

...ions/containers/ActionCreator/FormCreator/EmptyFormPlaceholder/tests/enterprise.unit.spec.ts


 function setup(opts: SetupOpts) {
  baseSetup({
-    hasEnterprisePlugins: true,


one thing I can say is that the enterprise tests should have the same set of plugins as the premium tests. So in this case we should add enterprisePlugins: ["whitelabel"], but not passing the tokenFeatures.

This would apply to all enterprise test that has hasEnterprisePlugins: true, removed without adding enterprisePlugins as I might not be able to cover all of them.

WiNloSt · 2025-12-22T07:04:39Z

frontend/src/metabase/admin/settings/tests/oss.unit.spec.tsx

 const setup = async ({ isAdmin = true, initialRoute = "" } = {}) => {
  return baseSetup({
-    hasEnterprisePlugins: false,
+    enterprisePlugins: undefined,


saw other tests just casually drop hasEnterprisePlugins: false, maybe it's fine not to pass enterprisePlugins at all?

WiNloSt · 2025-12-22T07:05:13Z

frontend/src/metabase/browse/metrics/tests/enterprise.unit.spec.tsx

-    ...opts,
-  });
-}
+import { setup } from "./setup";


there should be plugin initialization on enterprise tests.

WiNloSt · 2025-12-22T07:06:58Z

frontend/src/metabase/collections/components/CollectionHeader/tests/enterprise.unit.spec.tsx

      entity_id: createMockEntityId("okNLSZKdSxaoG58JSQY54"),
    },
-    hasEnterprisePlugins: true,
+    enterprisePlugins: "*" as const, // TODO: specify exact plugins needed


Please always provide dates on all todo comments. Ref

metabase-bot bot assigned lorem--ipsum Dec 10, 2025

metabase-bot bot added the .Team/Embedding label Dec 10, 2025

lorem--ipsum force-pushed the unit-test-targeted-plugin-init branch from a311d0a to bd3338d Compare December 16, 2025 11:16

lorem--ipsum requested a review from a team as a code owner December 16, 2025 11:16

lorem--ipsum force-pushed the unit-test-targeted-plugin-init branch from bd3338d to d005f70 Compare December 16, 2025 12:30

lorem--ipsum added the double-backport label Dec 16, 2025

WiNloSt reviewed Dec 17, 2025

View reviewed changes

WiNloSt requested a review from a team December 17, 2025 05:11

lorem--ipsum added 11 commits December 18, 2025 11:35

Demo of only importing the relevant plugin in unit test

078830c

Optimize enterprise plugin unit tests with targeted setupEnterpriseOn…

af089c8

…lyPlugin calls

fixed tests

ae9c08d

fixup

8717dcd

.

f3580e0

fixed test

60b3183

removed hasEnterprisePlugin

40b2ae3

fixup

1e2dd7e

removed remaining hasEnterprisePlugins and unnecessary setupEnterpris…

8bad37d

…e fn

specificPlugins -> enterprisePlugins

8ac9455

.

7250b97

lorem--ipsum force-pushed the unit-test-targeted-plugin-init branch from 3959cf4 to 7250b97 Compare December 18, 2025 10:35

fixed type

ddab8f2

lorem--ipsum commented Dec 18, 2025

View reviewed changes

lorem--ipsum changed the title ~~Demo of only importing the relevant plugin in unit test~~ Only import the needed plugins in unit tests Dec 18, 2025

lorem--ipsum requested a review from a team December 18, 2025 15:04

iethree approved these changes Dec 18, 2025

View reviewed changes

docs

91d2886

uladzimirdev approved these changes Dec 18, 2025

View reviewed changes

lorem--ipsum added 3 commits December 18, 2025 18:09

Merge branch 'master' into unit-test-targeted-plugin-init

9f449ea

Merge branch 'master' into unit-test-targeted-plugin-init

f3f21aa

Merge branch 'master' into unit-test-targeted-plugin-init

c0c35ff

lorem--ipsum merged commit ae13d81 into master Dec 19, 2025
176 of 177 checks passed

lorem--ipsum deleted the unit-test-targeted-plugin-init branch December 19, 2025 06:51

This was referenced Dec 19, 2025

🤖 backported "Only import the needed plugins in unit tests" #67247

Merged

🤖 backported "Only import the needed plugins in unit tests" #67248

Closed

metabase-bot bot reviewed Dec 19, 2025

View reviewed changes

github-actions bot added this to the 0.58 milestone Dec 19, 2025

lorem--ipsum added backport Automatically create PR on current release branch on merge and removed double-backport labels Dec 19, 2025

WiNloSt reviewed Dec 22, 2025

View reviewed changes

This was referenced Dec 30, 2025

[CLEAN] Synthetic Benchmark PR #66751 - Only import the needed plugins in unit tests qodo-benchmark/metabase#91

Open

[CORRUPTED] Synthetic Benchmark PR #66751 - Only import the needed plugins in unit tests qodo-benchmark/metabase#92

Open

escherize mentioned this pull request Feb 11, 2026

58 fake snowflake sync merge conflict fix #69523

Closed

		@@ -18,7 +18,7 @@ import { createMockTokenFeatures } from "metabase-types/api/mocks";

		function setup(opts: SetupOpts = {}) {
		baseSetup({

Conversation

lorem--ipsum commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

iethree left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

e2e tests failed on c0c35ff7288d752cf0110f35e9082097a2f52df7-1

Uh oh!

Uh oh!

github-actions bot commented Dec 19, 2025

Uh oh!

metabase-bot bot Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

metabase-bot bot Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

metabase-bot bot Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

metabase-bot bot commented Dec 19, 2025

TypeScript/JavaScript Code Review

Issues Found:

Positive Observations:

Uh oh!

github-actions bot commented Dec 19, 2025

Uh oh!

lorem--ipsum commented Dec 19, 2025

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lorem--ipsum commented Dec 10, 2025 •

edited

Loading

github-actions bot commented Dec 19, 2025 •

edited

Loading

e2e tests failed on `c0c35ff7288d752cf0110f35e9082097a2f52df7-1`