-
Notifications
You must be signed in to change notification settings - Fork 0
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
Bohandley/integration tests #27
Conversation
…other running grafana instances
…g off basic auth in grafana docker
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.
It might be worth updating to actions/checkout@v4
, actions/setup-go@v5
, and actions/setup-node@v4
. The latest versions of the Go and Node actions have the most capabilities with respect to dependency caching, which can significantly speed up CI/CD.
For more info:
- Go: https://github.com/actions/setup-go#caching-dependency-files-and-build-outputs
- Node: https://github.com/actions/setup-node?tab=readme-ov-file#caching-global-packages-data
An important difference between the latest Go & Node actions is that the Go one enables caching by default, and the Node one does not - it requires a few lines of config.
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.
Awesome, I'm going to add this as an issue to improve CI/CD for this repo!
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.
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.
Go caching is working! Saved 6 minutes of workflow time. I'm a little stuck on the node caching currently but it is captured in the issue.
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 think this looks great @bohandley! Added a few comments with some tips and tricks related to playwright and plugin-e2e. Let me know if you have any questions. 👍
.github/workflows/e2e.yml
Outdated
- name: Wait for prometheus to be configured in grafana | ||
uses: nev7n/wait_for_response@v1 | ||
with: | ||
url: 'http://localhost:3099/api/datasources/uid/prometheus-amazon/health' |
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.
Wouldn't just waiting for this url be enough?
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 not sure what you mean? Waiting for this response ensures that the Grafana can make a request to Prometheus and the response is a 200. This might not be necessary though, waiting for Prometheus and Grafana might be enough.
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.
Yeah I mean if this url returns 200, it means Grafana and Prometheus are running so we can remove those steps in the workflow?
- name: Start the docker container for E2E | ||
run: | | ||
docker compose -f docker-compose-debug.yaml pull | ||
docker compose -f docker-compose-debug.yaml up -d |
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.
If I understand correctly, this PR only tests your plugin against the latest enterprise main build. I see you have specified grafana 10.4.0 as min dep in the plugin.json. If the plugin is supposed to be compatible with all grafana versions beyond 10.4.0, I suggest you start using the e2e-versions
Action. This Action will resolve a range of Grafana versions to test against so you can be sure you plugin is compatible with that entire range. It's documented here.
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.
This is awesome! We still need to decide what versions we will support so I will add this to an issue to update the workflow.
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.
with: | ||
name: test-results | ||
path: test-results/ | ||
retention-days: 30 |
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.
Downloading the report manually and firing it up locally is pretty time consuming. The example workflow for a backend ds plugin in this guide has a step that pushes the playwright report to a GCS bucket. It also drops a link to the report in the workflow summary, so you can browse the report directly from the PR without having to download it manually.
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.
This is super helpful. We will have to choose a storage location appropriate for the team that will take this repository on, so I will add this as an issue to improve the debugging experience.
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.
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.
This is super helpful. We will have to choose a storage location appropriate for the team that will take this repository on, so I will add this as an issue to improve the debugging experience.
If you use the publish-report
Action, you won't be able to choose storage location. It uses Grafana's dev artifact storage bucket, and the url to the report will be:
https://storage.googleapis.com/releng-pipeline-artifacts-dev/<repo-name>/<pr-number>/<the-version-of-grafana-you're-testing-against>/
e2e/annotation-editor.spec.ts
Outdated
|
||
await annotationEditPage.datasource.set(ds.name); | ||
await annotationEditPage | ||
.getByTestIdOrAriaLabel(selectors.components.DataSource.Prometheus.queryEditor.code.queryField).focus(); |
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.
Don't think you need this
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 are right, I don't think I need the focus or setting the DS, because the default DS is Prometheus.
e2e/configuration.spec.ts
Outdated
page, | ||
}) => { | ||
const ds = await readProvisionedDataSource<DataSourcePluginOptionsEditorProps<PromOptions>>({ fileName: 'datasources.yml' }); | ||
const configPage = await createDataSourceConfigPage({type: ds.type, deleteDataSourceAfterTest: true}); |
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.
deleteDataSourceAfterTest
defaults to true, so you can remove this. 👍
await explorePage.datasource.set(ds.name); | ||
// query patterns | ||
await expect(explorePage | ||
.getByTestIdOrAriaLabel(selectors.components.QueryBuilder.queryPatterns)).toBeVisible(); |
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.
Using the selectors from grafana/e2e-selectors is probably not a good idea here. If the value of this selector changes in for example 11.0.0, your test will no longer work. I suggest one of the following:
- define the selector locally in this plugin repo
- if the same selector should be used by multiple plugins, define a versioned selector in plugin-e2e instead and use the selector fixture in your test.
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.
This will require an issue created for core Grafana. We are currently using the same selectors for core Prometheus DS which still relies on cypress. Additional question: can we use plugin-e2e in core Grafana for core data sources?
Future work will hopefully only use one set of selectors and one framework for e2e tests, and as we are keeping all of these Prometheus data sources as similar as possible, I will create an issue for this.
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.
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.
Additional question: can we use plugin-e2e in core Grafana for core data sources?
Yes! Just add a new folder with your plugin name in this directory, and then setup a new project that runs your tests here. I'd be happy to help out!
Future work will hopefully only use one set of selectors and one framework for e2e tests, and as we are keeping all of these Prometheus data sources as similar as possible, I will create an issue for this.
Our long term goal is to detach @grafana/e2e-selectors
from grafana packages so that it's no longer tied to a certain Grafana version. Instead, each selector will be a valid from a min Grafana version (that's how it works in plugin-e2e today). e.g
Components: {
CodeEditor: {
container: {
'10.2.3': 'data-testid Code editor container',
'8.3.0': 'Code editor container',
},
},
...
}
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 began an epic describing the changes in Grafana that would need to happen to use selectors across all the Prometheus data sources, core and external. If you have any suggestions or improvements to the epic please feel free to edit the description!
|
||
// await explorePage.datasource.set(dsDefaultEditorBuilder.name); | ||
|
||
await page.getByTestId('data-testid Select a data source').click(); |
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 will troubleshoot this. If you see this error in ci again, please share playwright report.
package.json
Outdated
"@grafana/eslint-config": "6.0.1", | ||
"@grafana/plugin-e2e": "0.17.1", |
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.
nit: the most recent version is 0.19.0
playwright.config.ts
Outdated
projects: [ | ||
{ | ||
name: 'data source', | ||
use: { ...devices['Desktop Chrome']}, |
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.
Up to you, but I don't recommend testing with anonymous user. See this guide on how to add a setup project that ensures all tests will start as logged in admin user. It won't hurt performance in your tests.
.gitignore
Outdated
.eslintcache | ||
/test-results/ | ||
# /playwright-report/ |
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 suggest you uncomment this - there's not reason to version control you local generated reports.
Also add /playwright/.auth/
# with: | ||
# version: latest | ||
# args: coverage | ||
- name: Build backend |
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.
Building the backend here @itsmylife
cache-dependency-path: '**/package-lock.json' | ||
- name: Install dependencies | ||
run: npm install -g yarn && yarn install --frozen-lockfile; | ||
- name: Build Frontend |
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.
Building the frontend here @itsmylife
@@ -12,4 +12,4 @@ services: | |||
- 3000:3000/tcp |
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.
Shouldn't this to be "3000:3000"
with quotes around and without tcp?
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.
Oh! That docker file is not what we use to test things currently, but it comes "out of the box" with a backend plugin.
The docker compose file with all the nice things is docker-compose-debug.yaml
.
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.
The expected tests are passing when I run the pull_request
action with act
.
Nicely done, @bohandley!
Use playwright and @grafana/plugin-e2e to write tests in parity with core Grafana prometheus.
Create github workflow with docker-compose-debug.yaml to test integration with Prometheus
How to test:
mage -v
yarn install
yarn build
docker compose -f docker-compose-debug.yaml up
for Grafana at locahost:3099 and Prometheus at localhost:9090yarn test:e2e
to run plugin-e2e tests against the docker Grafana instance and Prometheus.Next steps