-
Notifications
You must be signed in to change notification settings - Fork 43
feat: headless app and api for testing js-waku in browser #2371
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
base: master
Are you sure you want to change the base?
Conversation
3b260f8
to
f295a35
Compare
0ab2d81
to
8c53464
Compare
8c53464
to
cffbef3
Compare
size-limit report 📦
|
1d822b1
to
365833c
Compare
Ready for review? Issue description would be nice:D |
ping @adklempner |
@danisharora099 @weboko updated! |
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.
Pull Request Overview
This PR introduces a new browser testing infrastructure that replaces the old testing structure with a headless web app and server API endpoints that align with the nwaku REST API. Key changes include:
- Implementation of a new Express server to run a headless browser with Playwright.
- Introduction of multiple API endpoints (info, push, admin, etc.) to interact with the Waku node.
- Updates to configuration files, Dockerfile, and CI workflows to support the new structure.
Reviewed Changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/browser-tests/src/server.ts.new | New server implementation to serve the headless app with API endpoints and browser initialization. |
packages/browser-tests/src/routes/* | New API routes for legacy and REST API compatible pushes and node management. |
packages/browser-tests/src/queue/index.ts | Message queue implementation for storing and retrieving messages. |
packages/browser-tests/src/browser/index.ts | Browser initialization and page management for the headless tests. |
packages/browser-tests/src/api/* | New shared API functions wrapping Waku SDK functionality for operations like push, node creation, and subscriptions. |
packages/browser-tests/playwright.config.ts, package.json, Dockerfile, CI workflows | Config and build updates to integrate the headless app and testing infrastructure. |
packages/browser-tests/README.md | Updated documentation describing the new architecture and usage examples. |
Comments suppressed due to low confidence (1)
Dockerfile:33
- Ensure consistency in naming; the Dockerfile refers to a 'headless' directory while other scripts and CI workflows refer to 'headless-tests'. Align these names to prevent confusion across environments.
COPY headless/package.json ./headless/
export async function pushMessage( | ||
waku: LightNode, | ||
contentTopic: string, | ||
payload?: Uint8Array<ArrayBuffer> |
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.
Consider revising the type annotation for payload; using 'Uint8Array' without a generic parameter is standard and may avoid potential type mismatches.
Copilot uses AI. Check for mistakes.
.eslintrc.json
Outdated
"build", | ||
"coverage", | ||
"proto", | ||
"**/webpack.config.js", |
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.
can't we suppress it in tsconfig
of a particular projects?
.github/workflows/ci.yml
Outdated
repository: waku-org/js-waku | ||
- uses: actions/setup-node@v3 | ||
with: | ||
node-version: ${{ env.NODE_JS }} | ||
- uses: ./.github/actions/npm | ||
- name: Install headless-tests dependencies |
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.
Since headless-tests
is part of packages in root package.json
- this step is not needed as - uses: ./.github/actions/npm
should trigger installation
.github/workflows/ci.yml
Outdated
repository: waku-org/js-waku | ||
- uses: actions/setup-node@v3 | ||
with: | ||
node-version: ${{ env.NODE_JS }} | ||
- uses: ./.github/actions/npm | ||
- name: Install headless-tests dependencies |
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.
isn't Install headless-tests dependencies
needed only for browser-test
step?
.github/workflows/playwright.yml
Outdated
@@ -29,6 +29,12 @@ jobs: | |||
|
|||
- uses: ./.github/actions/npm | |||
|
|||
- name: Install headless-tests dependencies |
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.
doesn't seem to be needed due to ./.github/actions/npm
called prior
@@ -17,6 +17,7 @@ | |||
"packages/rln", | |||
"packages/tests", | |||
"packages/browser-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.
if browser-tests
depends on headless-tests
- then they should be defined in different order in the array
also, about the naming, maybe it is better to rename:
browser-tests
->browser-container
;headless-tests
->browser-tests
;
That way we define that one is container and another is test cases.
File |
2. Start a headless browser to load the app | ||
3. Expose API endpoints to interact with Waku | ||
|
||
## API Endpoints |
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.
🔥
"start:build": "node ./src/build-example.js", | ||
"start:serve": "npx serve -p 8080 --no-port-switching ./example", | ||
"test": "npx playwright test" | ||
"start": "npm run start:server", |
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: can we group similar commands, please?
"start": "npm run start:server", | |
"start": "npm run start:server", | |
"start:serve": "cd ../headless-tests && npx serve -p 8080 --no-port-switching . -s", | |
"start:server": "node ./dist/server.js", |
"start": "npm run start:server", | ||
"start:serve": "cd ../headless-tests && npx serve -p 8080 --no-port-switching . -s", | ||
"test": "npx playwright test", | ||
"build:headless": "cd ../headless-tests && npm run build", |
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 we should be able instead of cd ../headless-tests && npm run build
to do something like npm run build --workspace=headless-tests
in this and similar places, removing cd ../headless-tests
altogether
let me know if it works, I am not 100% sure
name: "firefox", | ||
use: { ...devices["Desktop Firefox"] } | ||
}, | ||
// { |
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.
not needed anymore? if so, let's remove
packages/headless-tests/package.json
Outdated
"scripts": { | ||
"start": "serve .", | ||
"build": "webpack", | ||
"deploy": "gh-pages -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.
is it really deployed to GitHub pages?
}, | ||
rules: { | ||
// Disable rules that might cause issues with this package | ||
"no-undef": "off" |
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.
let's disable them here instead of inline comments?
@@ -0,0 +1,2 @@ | |||
declare module "dotenv-flow/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.
I am wondering why we need these declarations?
@@ -0,0 +1,106 @@ | |||
/* eslint-disable no-console */ |
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.
why this file has extension .ts.new
? :)
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, unnecessary artifact
@@ -0,0 +1,52 @@ | |||
/* eslint-disable no-console */ |
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.
so many of these, can we suppress it on the level of package's config?
} catch (err) { | ||
// Just log, don't fail | ||
// eslint-disable-next-line no-console | ||
console.error(`Error checking node protocols: ${String(err)}`); |
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.
do we actually need these log
statements? why can't we use debug
package?
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 same code needs to work both in the server and in the web app loaded via headless browser, so it makes minimal use of imported packages besides @waku/sdk
365833c
to
49bc580
Compare
Problem / Description
The current browser test framework has limitations in its structure and integration capabilities. We need a more streamlined approach that better aligns with the nwaku REST API, allowing for seamless testing across different environments including Docker containers used by the Distributed Systems Testing team.
Solution
Notes
Checklist