fix(plugin-e2e): wait for feature toggles instead of reading them once - #2832
Conversation
|
|
|
The two red legs are pre-existing, not from this change. Both fail on This change can't reach that test: Fixed in #2836, verified 8.5.27 → 13.2.0. Unrelated and repo-wide: |
sunker
left a comment
There was a problem hiding this comment.
Thanks for raising this! Added a minor comment.
The create-plugin update step should be fixed here, although you'll need to sync with main to pick up the fix. I suspect the other two failures may be intermittent and are something for me and the team to investigate. Let me know if you're still seeing them.
|
The rethrow is in, and the branch now includes main for #2837. Thanks for sorting that. The two version legs are not intermittent. |
isLegacyFeatureEnabled read window.grafanaBootData.settings.featureToggles with no guard, so it threw "Cannot read properties of undefined" whenever the app shell had rendered before its boot data was merged. DashboardPage.addPanel calls it on any Grafana 13 or later, so the panelEditPage fixture failed outright. Optional chaining alone is not enough. A missing map is indistinguishable from every toggle being disabled, so callers would take their legacy branch on an instance where the toggle is enabled. This waits for the map and only falls back to an empty one if it never arrives, which matches the existing bootData fixture's handling. Adds unit tests for the enabled, absent-toggle and never-arrives cases.
24c6ce4 to
6ea2936
Compare
What this PR does / why we need it:
isLegacyFeatureEnabledreadwindow.grafanaBootData.settings.featureToggleswith no guard:Grafana can render the app shell before it has merged its boot data. When that happens the map is briefly absent and the second line throws
TypeError: Cannot read properties of undefined (reading '<toggle>').DashboardPage.addPanelcalls this on any Grafana 13 or later to decideuseNewSidebarLayout, so thepanelEditPagefixture fails outright and takes the whole test with it.We hit this on a Grafana Cloud instance, whose shell fetches its settings asynchronously. It does not reproduce against a local Grafana, which inlines the settings into the document, so the failure only appears against a deployed instance.
Optional chaining alone would be the wrong fix. A missing map is indistinguishable from every toggle being disabled, so
addPanelwould silently choose its legacy branch on an instance wheredashboardNewLayoutsis actually enabled. That trades a loud crash for a quiet wrong path. This waits for the map instead, and only falls back to an empty one if it never arrives, which is the same degrade-and-log approach thebootDatafixture already uses.Worth noting the
Windowdeclaration insrc/index.tstypesfeatureTogglesas always present, which is why there was nothing to prompt a guard here. I have left that type alone rather than making it optional, since it describes what Grafana settles on, and widening it would ripple through unrelated call sites.Which issue(s) this PR fixes:
No upstream issue. Found downstream while getting a Cloud E2E suite green, catalogued in grafana/grafana-cloudwatch-datasource#619.
Special notes for your reviewer:
isFeatureToggleEnabled.test.tscovering an enabled toggle, a toggle absent from the map, that the map is waited for rather than read once, and that a map which never arrives returnsfalseand logs rather than throwing.npm run lint,npm run typecheckandnpx vitest runare all clean inpackages/plugin-e2e, 48 tests passing.One adjacent thing I did not change, in case you want it in a separate PR:
DataSourcePicker.set()fills the combobox and presses Enter without asserting that anything was selected. A name that matches nothing leaves the panel on its previous datasource and the test fails much later on a missing control. That silent no-op cost us several days of misdiagnosis. Happy to send a patch if you would like the assertion added.