-
Notifications
You must be signed in to change notification settings - Fork 2
test: add Playwright integration tests for dev-playground #76
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
Conversation
Add comprehensive frontend integration tests using Playwright to verify: - Page navigation and routing - Chart and data visualization rendering - SSE streaming and reconnection - Telemetry and SQL helpers functionality - Console error detection Tests use mocked API responses to verify UI behavior without backend dependencies. Includes test utilities, mock data fixtures, and Playwright configuration. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
pkosiec
left a comment
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.
Great job! 🚀
Two general comments from my side:
- We should be cautious to not introduce too many tests that test purely the dev-playground app, not the underlying AppKit UI / Backend.
- As our team is small, we should keep the number of tests small as well. I think this PR shows a lot of value, but some/most of the tests are begging to be converted to E2E ones 😄 How big that effort would be?
Please see my detailed comments and then we can discuss them further👍
| **Note:** These are frontend-only integration tests. API calls are intercepted at the browser level and return mock data, so the AppKit backend plugins are not tested. They focus on verifying UI behavior, navigation, data rendering, and client-side interactions. | ||
|
|
||
| ### Running Tests | ||
|
|
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'd add a note to ensure the DATABRICKS_APP_PORT is not overriden in the .env file - as initially running those tests failed in my case because of that reason.
EDIT: We specify DATABRICKS_APP_PORT=8001 in the .env.dist so after all maybe we should adjust the default test configuration to point to the app running on 8001 port?
JFYI - we specify it to avoid collision with the clean app example and run pnpm dev on the root of appkit repo to run all the apps.
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.
@calvarjorge I think you missed that PR, PTAL - let's configure playwright to point to the app working on 8001 port as our .env.dist configures, alright?
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.
Sure, will update!
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 test would be valuable if were executed against real backend. I know we initially discussed to have separate backend and FE tests, but maybe we should use this test as a very first E2E one? How much of effort would that cost?
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.
Again, great test, as long as the backend isn't mocked. What do you think?
My reasoning here is that we should avoid adding too much tests that will slow us down, and instead, we should focus on the ones that really bring value.
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 test already has a value with mocked backend, but it would be multiplied if we converted it to E2E test. That is, we'd mock the SDK on backend, run the dev-playground app, and run this test. What do you think?
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.
Same as before: This test already has a value with mocked backend, but it would be multiplied if we converted it to E2E test. That is, we'd mock the SDK on backend, run the dev-playground app, and run this test. What do you think?
- Remove navigation.spec.ts (tests router, not AppKit) - Remove 3 tests from smoke.spec.ts (test Dev Playground, not AppKit) - Move test utilities from fixtures/ to utils/ Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
…gration_tests_locally
apps/dev-playground/package.json
Outdated
| "zod": "^4.1.13" | ||
| }, | ||
| "devDependencies": { | ||
| "@playwright/test": "^1.50.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.
should we add a more recent version? latest is 1.58, is there any reason we have 1.50.1?
| await waitForPageLoad(page); | ||
| await page.waitForFunction( | ||
| () => document.querySelectorAll(".animate-pulse").length === 0, | ||
| { timeout: 10000 }, | ||
| ); |
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.
these two should be wrapped in a Promise.all
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 they are wrapped they will run in parallel. If the page has not loaded, the second promise will evaluate to true immediately no?
| await page.goto("/telemetry"); | ||
| await waitForPageLoad(page); |
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.
these two and the other two in the previous test can be moved into the beforeEach and refactored like
await Promise.all([waitForPageLoad(page), page.goto("/telemetry")]);
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.
Removed the waitForPageLoad func altogether, which is just an alias of page.waitForLoadState("networkidle");. When used with page.goto, I changed it to page.goto("/arrow-analytics", { waitUntil: "networkidle" }); and moved it to the beforeEach where possible.
Related to the Promise.all, even if now is not needed, I do have doubts wether this can just me applied generally. It will start the funcs concurrently, potentially causing a race condition in cases where we only want to evaluate the second func after the first. Maybe there's sth I'm missing though.
Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
pkosiec
left a comment
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, but let's wait for @MarioCadenas approve too 👍
Also, before merge, please address this comment, to make our lifes easier: #76 (comment) - thanks!
* ci: add playground integration tests job Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com> * chore: trigger CI Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com> * ci: skip ServiceContext initialization in CI Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com> * feat: allow passing a WorkspaceClient to createApp for e2e testing Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com> * chore: trigger CI Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com> * refactor: move e2e env vars from playwright config to CI workflow Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com> * docs: regenerate API docs for createApp client param Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com> --------- Signed-off-by: Jorge Calvar <jorge.calvar@databricks.com>
|
The Integration Vs. E2E topic was discussed offline - we will end up going with the integration tests as they are, i.e., mocking the backend. Doing E2E would require more effort, since we'd have to create a mock server for the Databricks API, and this is not the priority. |
Uh oh!
There was an error while loading. Please reload this page.