-
Notifications
You must be signed in to change notification settings - Fork 34
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
test: Adding web client testing framework #498
Changes from 8 commits
936bbb5
af79f9f
401b5d5
1fe0799
8701bbd
3e14482
aa9e51f
ed78776
b9313c4
dfbe340
ab72e83
61df391
a8b273d
03aa4b1
cbb721b
1ada1d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,8 @@ jobs: | |
- uses: actions/checkout@main | ||
- name: Install Rust | ||
run: rustup update --no-self-update | ||
- name: Install Node.js | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This and the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be worth to create a new workflow in the CI that compiles and runs the web integration tests and thus have them run in parallel, decreasing the time taken by the CI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was debating doing so but wasn't sure if wanted integration testing to be encapsulated to one workflow. I'm onboard though, will set up for next revision |
||
uses: actions/setup-node@v3 | ||
- uses: taiki-e/install-action@nextest | ||
- run: make clean-node | ||
- run: make node | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"require": ["ts-node/register", "esm"], | ||
"extension": ["ts"], | ||
"spec": "test/**/*.test.ts", | ||
"timeout": 600000 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
export { | ||
WebClient, | ||
NewTransactionResult, | ||
SerializedAccountStub, | ||
NewSwapTransactionResult | ||
WebClient, | ||
NewTransactionResult, | ||
SerializedAccountStub, | ||
NewSwapTransactionResult, | ||
} from "./crates/miden_client"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
# Testing | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should specify a node version somewhere the supported |
||
|
||
The .wasm must be run within the context of a webpage. To this end, we've set up a Mocha | ||
test suite which hosts the .wasm on a local server and then executes WebClient commands | ||
within the context of the web page. | ||
|
||
## Running tests | ||
|
||
1. Install dependencies via `yarn` | ||
1. Ensure the .wasm is built by running `yarn build` | ||
1. In crates/web-client run `yarn test` to run all tests | ||
igamigo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- Can alternatively run `yarn test:clean` to run the .wasm build process prior to testing. We provide both paths as the build process can take some time. | ||
|
||
1. For running an individual test by name run `yarn test -g <test-name>` | ||
|
||
## Writing tests | ||
|
||
1. The test setup in `mocha.global.setup.mjs` should expose the `create_client` function which can be used inside tests. | ||
- Any further setup of wasm code should be done in this file and similarly expose a function for testing here | ||
1. `webClientTestUtils.js` should contain all interfaces for interacting with the web client. If further methods need to be added, follow existing patterns which use the exposed `testingPage` and pass through any required arguments to the page execution. Example: | ||
|
||
``` | ||
/** | ||
* | ||
* @param {string} arg1 | ||
* @param {boolean} arg2 | ||
* | ||
* @returns {Promise<string>} The result | ||
*/ | ||
export const webClientCall = async (arg1, arg2) => { | ||
return await testingPage.evaluate( | ||
async (_arg1, _arg2) => { | ||
if (!window.client) { | ||
await window.create_client(); | ||
} | ||
|
||
/** @type {WebClient} */ | ||
const client = window.client; | ||
const result = client.webClientCall(_arg1, _arg2); | ||
|
||
return result; | ||
}, | ||
arg1, | ||
arg2 | ||
); | ||
}; | ||
``` | ||
|
||
- Add JSDocs to methods. This will allow typing in the `*.test.ts` files. | ||
- We add the `if (!window.client)` to avoid spinning up clients unnecessarily lengthening test time. This unfortunately cannot be pulled out to a helper method as the testingPage scope does not share the scope of the file. | ||
- Similarly, the boilerplate for passing args through as shown above is necessary due to scoping. | ||
|
||
## Debugging | ||
|
||
1. When inside of a `page.evaluate` , console logs are being sent to the servers console rather than your IDE's. You can uncomment the line as seen below in the `mocha.global.setup.mjs`: | ||
|
||
``` | ||
page.on("console", (msg) => console.log("PAGE LOG:", msg.text())); | ||
``` | ||
|
||
This will forward logs from the server to your terminal logs | ||
|
||
## Troubleshooting | ||
|
||
1. When trying to run the tests, if you receieve the following error: | ||
|
||
``` | ||
Error: Could not find Chrome (ver. 128.0.6613.119). This can occur if either | ||
1. you did not perform an installation before running the script (e.g. `npx puppeteer browsers install ${browserType}`) or | ||
2. your cache path is incorrectly configured (which is: /Users/ignacioamigo/.cache/puppeteer). | ||
For (2), check out our guide on configuring puppeteer at https://pptr.dev/guides/configuration. | ||
``` | ||
|
||
Try running: `npx puppeteer browsers install` and then run the tests again |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import { createNewFaucet, isValidAddress } from "./webClientTestUtils.js"; | ||
|
||
describe("faucet tests", () => { | ||
it("create a new faucet", async () => { | ||
const result = await createNewFaucet( | ||
"OffChain", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a heads up, the storage mode names will change to "Private" and "Public" when #514 gets merged. |
||
false, | ||
"DMX", | ||
"10", | ||
"1000000" | ||
); | ||
|
||
isValidAddress(result); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { Page } from "puppeteer"; | ||
|
||
declare module "./mocha.global.setup.mjs" { | ||
export const testingPage: Page; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import puppeteer from "puppeteer"; | ||
import { exec } from "child_process"; | ||
|
||
import { register } from "ts-node"; | ||
|
||
register({ | ||
project: "./tsconfig.json", | ||
}); | ||
|
||
let serverProcess; | ||
let browser; | ||
let testingPage; | ||
|
||
const TEST_SERVER_PORT = 8080; | ||
const TEST_SERVER = `http://localhost:${TEST_SERVER_PORT}`; | ||
|
||
// Should be in sync with the rpc port in tests/config/miden-node.toml | ||
const LOCAL_MIDEN_NODE_PORT = 57291; | ||
|
||
before(async () => { | ||
console.log("Starting test server..."); | ||
serverProcess = exec(`http-server ./dist -p ${TEST_SERVER_PORT}`); | ||
browser = await puppeteer.launch({ headless: true }); | ||
testingPage = await browser.newPage(); | ||
await testingPage.goto(TEST_SERVER); | ||
|
||
// Uncomment below to enable console logging | ||
// testingPage.on("console", (msg) => console.log("PAGE LOG:", msg.text())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about adding an env var like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I like that. I'll add some info to the README as well |
||
|
||
// Creates the client in the test context and attach to window object | ||
await testingPage.exposeFunction("create_client", async () => { | ||
await testingPage.evaluate(async (port) => { | ||
const { WebClient } = await import("./index.js"); | ||
let rpc_url = `http://localhost:${port}`; | ||
const client = new WebClient(); | ||
await client.create_client(rpc_url); | ||
|
||
window.client = client; | ||
}, LOCAL_MIDEN_NODE_PORT); | ||
}); | ||
}); | ||
|
||
after(async () => { | ||
console.log("Stopping test server..."); | ||
await browser.close(); | ||
serverProcess.kill("SIGTERM"); | ||
|
||
await new Promise((resolve, reject) => { | ||
serverProcess.on("close", (code) => { | ||
console.log(`Server process exited with code ${code}`); | ||
resolve(); | ||
}); | ||
|
||
serverProcess.on("error", (error) => { | ||
console.error("Error killing server process:", error); | ||
reject(error); | ||
}); | ||
}); | ||
}); | ||
|
||
export { testingPage }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import { createNewWallet, isValidAddress } from "./webClientTestUtils.js"; | ||
|
||
describe("wallet tests", () => { | ||
it("create a new wallet", async () => { | ||
const result = await createNewWallet("OffChain", false); | ||
|
||
isValidAddress(result); | ||
}); | ||
}); | ||
Comment on lines
+3
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice if we were to test that the accounts that the client is tracking (ie, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep that is part of future 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.
nit: missing newline on this file