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

Re-enable annotations e2e test #198

Merged
merged 3 commits into from
Dec 20, 2022
Merged

Conversation

kevinwcyu
Copy link
Contributor

@kevinwcyu kevinwcyu commented Dec 16, 2022

  • Updates @grafana/* packages in order to re-enable the annotations in the e2e test. Packages are updated to v9.3.2 since there were some fixes in the e2e and e2e-selector packages that are needed for the tests to work.
  • Updates @grafana/aws-sdk to fix a build error where className was not properly recognized as a prop on ResourceSelector used in QueryEditor.tsx

Note: to run e2e locally, you'll need to make sure you're using and npm version < 9. For more info see grafana/grafana#59425.

Fixes: #187

@kevinwcyu kevinwcyu requested a review from a team as a code owner December 16, 2022 20:05
@github-actions
Copy link

github-actions bot commented Dec 16, 2022

Levitate is-compatible report:

🔍 Resolving @grafana/data@latest...
🔍 Resolving @grafana/ui@latest...
🔍 Resolving @grafana/runtime@latest...
🔍 Resolving @grafana/e2e-selectors@latest...

🔬 Checking compatibility between ./src/module.ts and @grafana/data@9.3.2...
✔ Found @grafana/data version 9.3.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/ui@9.3.2...
✔ Found @grafana/ui version 9.3.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/runtime@9.3.2...
✔ Found @grafana/runtime version 9.3.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/e2e-selectors@9.3.2...
✔ Found @grafana/e2e-selectors version 9.3.2 locally

✔️ ./src/module.ts appears to be compatible with @grafana/data,@grafana/ui,@grafana/runtime,@grafana/e2e-selectors

@github-actions
Copy link

Backend code coverage report for PR #198
No changes

@github-actions
Copy link

Frontend code coverage report for PR #198

Plugin Main PR Difference
src 83.54% 83.54% 0%

Copy link
Contributor

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

LGTM

Copy link
Member

@sarahzinger sarahzinger left a comment

Choose a reason for hiding this comment

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

Looks ok to me! A few questions for my own knowledge, also I think I kind of lost where the resource selector/query editor was involved? Did those changes happen elsewhere or am I just missing it lol.

@@ -6,7 +6,7 @@ services:
build:
context: ./.config
args:
grafana_version: ${GRAFANA_VERSION:-9.1.2}
Copy link
Member

Choose a reason for hiding this comment

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

just curious if this change is necessary, seems harmless, more just curious if the e2e tests use this docker file at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the e2e tests that run in ci don't use this docker file, i was using this to test the e2e locally.

@@ -37,6 +37,7 @@ export const mockDatasource = new DataSource({
type: 'athena-datasource',
name: 'Athena Data Source',
access: 'proxy',
readOnly: false,
Copy link
Member

Choose a reason for hiding this comment

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

what did this do? I'm guessing this is just a new required field in 9.3.2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, new required field

const regionsResponse = await ctx.ds.getRegions();

expect(regionsResponse).toHaveLength(response.length);
expect(regionsResponse).toBe(response);
expect(regionsResponse).toEqual(response);
Copy link
Member

Choose a reason for hiding this comment

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

curious if this is the result of just a package update or if this is like meaningfully different in some way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the mock's response was previously set with backendSrv.get = jest.fn(() => Promise.resolve(response)); (removed line 70), but now it's set up in the beforeEach so now it's not testing for reference equality anymore.

@@ -61,61 +56,76 @@ describe('AthenaDatasource', () => {
jsonData: { defaultRegion: 'testRegion', catalog: 'testCatalog', database: 'testDatabase' },
} as unknown as DataSourceInstanceSettings<AthenaDataSourceOptions>;
ctx.ds = new DataSource(ctx.instanceSettings);
runtime.setBackendSrv(backendSrv as runtime.BackendSrv);
ctx.ds.getResource = jest.fn().mockImplementation((path: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

why were these changes needed is setBackendSrv not a thing in the newer version? Looks good tho, just curious!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setBackendSrv is still a thing, but after upgrading, there were errors with needing to mock fetch from the BackendSrv. i didn't want to spend time mocking out the internals of the BackendSrv.

@kevinwcyu kevinwcyu merged commit c69892c into main Dec 20, 2022
@kevinwcyu kevinwcyu deleted the kevinwcyu/enable-annotations-e2e branch December 20, 2022 15:47
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.

Enable annotations e2e test scenario
3 participants