-
Notifications
You must be signed in to change notification settings - Fork 26
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
Create plugin: Add e2e specs #855
Conversation
Hello! 👋 This repository uses Auto for releasing packages using PR labels. ❌ This PR cannot be merged until the following issues are addressed:
🏷️ More info about which labels to use
|
|
||
if rand.Int()%2 == 0 { | ||
status = backend.HealthStatusError | ||
message = "randomized error" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing this randomized error with something that is consistent so that e2e tests can be added for the health check. The code in models/settings is copied over from the datasource-basic example.
@@ -50,6 +50,7 @@ export const AppConfig = ({ plugin }: AppConfigProps) => { | |||
<Field label="API Key" description="A secret key for authenticating to our custom API"> | |||
<SecretInput | |||
width={60} | |||
id="config-api-key" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
..so that input field becomes associated with the label field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own understanding - is this required for playwright to find the input field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find the input field using test id too, but it's preferred to test user-visible behavior if possible. So await page.getByRole('textbox', { name: 'API Key' })
is preferred over await page.getByTestId('config-api-key').
Ofc this is not always possible. When accessing grafana ui elements outside of the plugin code, plugin-e2e users should use the e2e selectors. e.g panelEditPage.getByGrafanaSelector(selectors.components.CodeEditor.container)
. There's more info regarding this in the Select UI elements guide in the e2e docs.
@@ -1,3 +1,4 @@ | |||
import { getBackendSrv, isFetchError } from '@grafana/runtime'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of less copied from datasource-http example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a lot of changes to what is scaffolded though I think they're for the best as it feels like neater approaches to things like health checks.
@@ -50,6 +50,7 @@ export const AppConfig = ({ plugin }: AppConfigProps) => { | |||
<Field label="API Key" description="A secret key for authenticating to our custom API"> | |||
<SecretInput | |||
width={60} | |||
id="config-api-key" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own understanding - is this required for playwright to find the input field?
} | ||
} catch (err) { | ||
let message = ''; | ||
if (_.isString(err)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... lodash feels a bit overkill for checking primitives. Is this how we do it in examples repo? frontend platform has been trying to remove/discourage usage of lodash in core for quite some time now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from the examples repo. Yeah I don't like this either. Will try to fix here and update examples repo too
@@ -22,7 +22,7 @@ | |||
"@grafana/e2e": "{{ grafanaVersion }}", | |||
"@grafana/e2e-selectors": "{{ grafanaVersion }}",{{/unless}} | |||
"@grafana/eslint-config": "^7.0.0",{{#if usePlaywright}} | |||
"@grafana/plugin-e2e": "^0.24.0",{{/if}} | |||
"@grafana/plugin-e2e": "^0.27.0",{{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm under the impression that we're not gonna release these changes until 1.0.1 is out the door?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No you're right! It's just that one of the specs depend on changes released in 0.27.0, so needed to bump to be able to test and verify what was generated. We should point to 1.0.1 before merging to main. 👍
What this PR does / why we need it:
This PR adds plugin type specific specs to the scaffolding. There's specific specs for...
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: