Skip to content
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

Plugin E2E: Change FE feature toggles during test session #651

Merged
merged 10 commits into from
Jan 23, 2024

Conversation

sunker
Copy link
Contributor

@sunker sunker commented Jan 9, 2024

What this PR does / why we need it:

This is a bit experimental, and I'm not sure we want to go down this road so feedback is much appreciated.

I foresee that many plugin authors are going to want to test their plugins with certain feature toggles both enabled and disabled. Currently, the only way to change the value of a feature toggle in CI is by setting the GF_FEATURE_TOGGLES_ENABLE environment variable when starting the grafana container in the github workflow. Then they can use the isFeatureToggleEnabled fixture inside a test to check whether a certain feature is enabled. This works, but in order to try different values for a certain feature, they'll have to start multiple versions of Grafana in the workflow. This is complicated and it will also demand more resources.

As you know, Grafana feature toggles are being passed from Grafana backend to the frontend by setting window.grafanaBootData.settings.featureToggles. Playwright provide some guidelines for how to mock browser apis, such as the window object. In this PR, I'm adding a addInitScript function that is executed whenever the page is navigated. The function will poll for the existence of window.grafanaBootData, and when it exists it will override and/or add features that are defined in the playwright test session. The plugin-e2e consumer can then define their feature toggles in the playwright.config.ts like this:

  use: {
    baseURL: 'http://localhost:3000',
    featureToggles: {
      redshiftAsyncQueryDataSupport: false,
    },
    ...
  },

Of override the feature toggle for a certain file:

#queryEditor.spec.ts
test.use({
  featureToggles: {
    redshiftAsyncQueryDataSupport: true,
  },
});

I know this solution is kind of brittle and I wouldn't raise the PR if it wasn't for the fact the problem feels important to solve.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

📦 Published PR as canary version: Canary Versions

✨ Test out this PR locally via:

npm install @grafana/create-plugin@3.2.0-canary.651.469ce0d.0
npm install @grafana/plugin-e2e@0.10.0-canary.651.469ce0d.0
# or 
yarn add @grafana/create-plugin@3.2.0-canary.651.469ce0d.0
yarn add @grafana/plugin-e2e@0.10.0-canary.651.469ce0d.0

@sunker sunker force-pushed the plugin-e2e/change-feature-toggles branch from 0a1eb10 to c5e64c1 Compare January 9, 2024 12:33
Copy link

github-actions bot commented Jan 9, 2024

Hello! 👋 This repository uses Auto for releasing packages using PR labels.

✨ This PR can be merged and will trigger a new minor release.
NOTE: When merging a PR with the release label please avoid merging another PR. For further information see here.

@sunker sunker marked this pull request as ready for review January 9, 2024 13:16
@sunker sunker requested review from jackw, academo and mckn January 9, 2024 13:17
@academo
Copy link
Member

academo commented Jan 10, 2024

@sunker We have a mechanism to enable and disable feature toggles in frontend already that uses local storage. you can see how it works here so there's no need to write code that modifies the grafanaBootData https://github.com/grafana/grafana/blob/4fa6bad7c03948cd648f784283bc60d7b2199ad3/packages/grafana-runtime/src/config.ts#L217

@sunker
Copy link
Contributor Author

sunker commented Jan 10, 2024

@sunker We have a mechanism to enable and disable feature toggles in frontend already that uses local storage. you can see how it works here so there's no need to write code that modifies the grafanaBootData https://github.com/grafana/grafana/blob/4fa6bad7c03948cd648f784283bc60d7b2199ad3/packages/grafana-runtime/src/config.ts#L217

Oh I did not know that. Thanks @academo, I'll try that instead.

Copy link
Collaborator

@mckn mckn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the need for something like this is valid and something that we should provide! The question is how we should tailor this. If we don't change the value across the entire stack (frontend + backend) there is a risk of having functionality that doesn't function properly.

On the other hand, if we update the value across the entire stack we might have problems running tests affected by the same toggle in parallel.

I guess there are pros/cons with every approach. Can we do something like the following?

Thoughts on that? This approach will update the feature flag

@sunker
Copy link
Contributor Author

sunker commented Jan 10, 2024

Thanks for great input @mckn!

Great, I did not know we had this API either.

One concern that I have with using this API is that it was introduced in Grafana 10.2.0. So far, all features in plugin-e2e are backwards compatible to Grafana 8.3.0. We could probably limit this feature in plugin-e2e to Grafana versions >=10.2.0, but it would not be ideal. Actually the same issue applies to using the overrideFeatureTogglesFromLocalStorage method @academo as it was introduced in Grafana 10.1.0.

On the other hand, if we update the value across the entire stack we might have problems running tests affected by the same toggle in parallel.

I think this is a very valid concern. Just to circumvent this problem, I wonder if it's best to limit this capability in plugin-e2e to only change the frontend part of feature toggle. Maybe it's fine as long as we are very clear about that in the docs? If a plugin authors needs to tests a feature toggle across the entire stack, it needs to be defined when configuring the Grafana instance.

@mckn
Copy link
Collaborator

mckn commented Jan 10, 2024

Thanks for great input @mckn!

Great, I did not know we had this API either.

One concern that I have with using this API is that it was introduced in Grafana 10.2.0. So far, all features in plugin-e2e are backwards compatible to Grafana 8.3.0. We could probably limit this feature in plugin-e2e to Grafana versions >=10.2.0, but it would not be ideal. Actually the same issue applies to using the overrideFeatureTogglesFromLocalStorage method @academo as it was introduced in Grafana 10.1.0.

On the other hand, if we update the value across the entire stack we might have problems running tests affected by the same toggle in parallel.

I think this is a very valid concern. Just to circumvent this problem, I wonder if it's best to limit this capability in plugin-e2e to only change the frontend part of feature toggle. Maybe it's fine as long as we are very clear about that in the docs? If a plugin authors needs to tests a feature toggle across the entire stack, it needs to be defined when configuring the Grafana instance.

That would work! An alternative would be to only do the frontend part of the toggle in versions earlier than 10.2.0? If that would be a way forward.

@sunker sunker requested a review from a team as a code owner January 17, 2024 19:05
@sunker sunker force-pushed the plugin-e2e/change-feature-toggles branch from 037e073 to 15e9b02 Compare January 18, 2024 07:34
@sunker sunker added minor Increment the minor version when merged release Create a release when this pr is merged labels Jan 18, 2024
@sunker sunker changed the title Plugin E2E: Change feature toggles during test session Plugin E2E: Change FE feature toggles during test session Jan 18, 2024
@sunker
Copy link
Contributor Author

sunker commented Jan 18, 2024

An alternative would be to only do the frontend part of the toggle in versions earlier than 10.2.0? If that would be a way forward.

Think it could become a bit confusing. Also, then they may have to write different test code for different versions of Grafana, which is not ideal.

@sunker
Copy link
Contributor Author

sunker commented Jan 18, 2024

@academo this PR should be ready for review now. Now I'm updating local storage in case Grafana version => 10.1.0. If not, I'm updating the bootData directly.

};
}
});
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the js in this file should be 100% compatible with recent versions of all browsers.

@sunker sunker mentioned this pull request Jan 18, 2024
6 tasks
@sunker sunker merged commit dcd61ad into main Jan 23, 2024
18 checks passed
@sunker sunker deleted the plugin-e2e/change-feature-toggles branch January 23, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged release Create a release when this pr is merged
Projects
Development

Successfully merging this pull request may close these issues.

3 participants