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

Bohandley/integration tests #27

Merged
merged 74 commits into from
Mar 13, 2024
Merged

Bohandley/integration tests #27

merged 74 commits into from
Mar 13, 2024

Conversation

bohandley
Copy link
Collaborator

@bohandley bohandley commented Feb 26, 2024

  • 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:

  1. Build the backend mage -v
  2. Build the frontend yarn install yarn build
  3. run docker compose -f docker-compose-debug.yaml up for Grafana at locahost:3099 and Prometheus at localhost:9090
  4. run yarn test:e2e to run plugin-e2e tests against the docker Grafana instance and Prometheus.

Next steps

  • Write tests for auth to test middleware which needs to be added in plugin-sdk

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:

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.

Copy link
Collaborator Author

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#28

Copy link
Collaborator Author

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.

Base automatically changed from bohandley/use-prom-library-with-sigv4-config to main March 12, 2024 13:34
Copy link

@sunker sunker 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 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. 👍

- 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'
Copy link

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?

Copy link
Collaborator Author

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.

Copy link

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
Copy link

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#29

with:
name: test-results
path: test-results/
retention-days: 30
Copy link

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#30

Copy link

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>/


await annotationEditPage.datasource.set(ds.name);
await annotationEditPage
.getByTestIdOrAriaLabel(selectors.components.DataSource.Prometheus.queryEditor.code.queryField).focus();
Copy link

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

Copy link
Collaborator Author

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.

page,
}) => {
const ds = await readProvisionedDataSource<DataSourcePluginOptionsEditorProps<PromOptions>>({ fileName: 'datasources.yml' });
const configPage = await createDataSourceConfigPage({type: ds.type, deleteDataSourceAfterTest: true});
Copy link

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();
Copy link

@sunker sunker Mar 12, 2024

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#31

Copy link

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',
    },
  },
...
}

Copy link
Collaborator Author

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!

grafana/grafana#84394


// await explorePage.datasource.set(dsDefaultEditorBuilder.name);

await page.getByTestId('data-testid Select a data source').click();
Copy link

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",
Copy link

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

projects: [
{
name: 'data source',
use: { ...devices['Desktop Chrome']},
Copy link

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/
Copy link

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link

@NWRichmond NWRichmond left a 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!

@bohandley bohandley merged commit b344d4e into main Mar 13, 2024
2 checks passed
@bohandley bohandley deleted the bohandley/integration-tests branch March 13, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants