-
Notifications
You must be signed in to change notification settings - Fork 31
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: Make it possible to test data source config page #546
Conversation
Hello! 👋 This repository uses Auto for releasing packages using PR labels. ✨ This PR can be merged. It will not be considered when calculating future versions of the npm packages and will not appear in the changelogs. |
@@ -0,0 +1 @@ | |||
GOOGLE_JWT_FILE= |
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.
secret added to the repo. can also be found in plugin-provisioning repo if you want to try this locally (or provide your own ofc).
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v3 | ||
|
||
- name: Copy auth file | ||
run: mkdir -p packages/plugin-e2e/playwright/.auth/ && cp .github/user.json packages/plugin-e2e/playwright/.auth/user.json |
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 wish playwright could create this file if it doesn't already exist, but doesn't seem like it's possible.
|
||
expect.soft(createDsReq.ok(), `Failed to create data source: ${text}`).toBeTruthy(); | ||
return existingDataSource.json(); | ||
}; |
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 code here is pretty rough and probably doesn't cover all scenarios...will probably come back to this in upcoming PRs.
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.
LGTM!
getByTestIdOrAriaLabel(selector: string, root?: Locator): Locator { | ||
if (selector.startsWith('data-testid')) { | ||
return (root || this.ctx.page).getByTestId(selector); | ||
} | ||
|
||
return (root || this.ctx.page).locator(`[aria-label="${selector}"]`); | ||
} |
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 do see the reason for adding this but it would be nice to push the user to use either test id or aria label. But given the structure we have in grafana core I guess it would be non-trivial to get in place.
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.
Yes! However, lately steps have been taken to migrate all selectors from aria-label to data-testid. But even if we'd be able to get to that point reasonably soon in grafana core, this package would still need to function with much older versions of Grafana so we're gonna have to keep this abstraction for some time. =/
What this PR does / why we need it:
This PR adds fixtures, models and expect matchers that enables testing the data source config page in a data source plugin.
Also adding a two simple playwright tests that checks that configuring a new google sheets data source works as expected. This could serve as an example of how plugin-e2e can help testing the data source config page, but it's also helpful to have meanwhile working in this repo.
Which issue(s) this PR fixes:
Part of grafana/grafana#78078
Fixes #
Special notes for your reviewer: