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

test: Adding web client testing framework #498

Merged
merged 16 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/test.yml
Copy link
Collaborator

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

Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ jobs:
- uses: actions/checkout@main
- name: Install Rust
run: rustup update --no-self-update
- name: Install Node.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

This and the Makefile changes look good to me, but will probably ask someone on the miden or lambda class team to take a closer look!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Expand Down
5 changes: 5 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,15 @@ test-deps: ## Install dependencies for tests
integration-test: ## Run integration tests
cargo nextest run --workspace --exclude miden-client-web --release --test=integration $(FEATURES_CLI) --no-default-features

.PHONY: integration-test-web-client
integration-test-web-client: ## Run integration tests for the web client
cd ./crates/web-client && npm run test:clean

.PHONY: integration-test-full
integration-test-full: ## Run the integration test binary with ignored tests included
cargo nextest run --workspace --exclude miden-client-web --release --test=integration $(FEATURES_CLI)
cargo nextest run --workspace --exclude miden-client-web --release --test=integration $(FEATURES_CLI) --run-ignored ignored-only -- test_import_genesis_accounts_can_be_used_for_transactions
$(MAKE) integration-test-web-client

.PHONY: kill-node
kill-node: ## Kill node process
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,10 @@ pub struct SyncNoteResponse {
/// Block header of the block with the first note matching the specified criteria
#[prost(message, optional, tag = "2")]
pub block_header: ::core::option::Option<super::block_header::BlockHeader>,
/// Proof for block header's MMR with respect to the chain tip.
/// Merkle path to verify the block's inclusion in the MMR at the returned `chain_tip`.
///
/// More specifically, the full proof consists of `forest`, `position` and `path` components. This
/// value constitutes the `path`. The other two components can be obtained as follows:
/// - `position` is simply `resopnse.block_header.block_num`
/// - `forest` is the same as `response.chain_tip + 1`
/// An MMR proof can be constructed for the leaf of index `block_header.block_num` of
/// an MMR of forest `chain_tip` with this path.
#[prost(message, optional, tag = "3")]
pub mmr_path: ::core::option::Option<super::merkle::MerklePath>,
/// List of all notes together with the Merkle paths from `response.block_header.note_root`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,10 @@ pub struct SyncNoteResponse {
/// Block header of the block with the first note matching the specified criteria
#[prost(message, optional, tag = "2")]
pub block_header: ::core::option::Option<super::block_header::BlockHeader>,
/// Proof for block header's MMR with respect to the chain tip.
/// Merkle path to verify the block's inclusion in the MMR at the returned `chain_tip`.
Copy link
Collaborator Author

@julian-demox julian-demox Aug 28, 2024

Choose a reason for hiding this comment

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

Not sure why these generated files changed. Its conflicting in the auto-merge so will remove the change

///
/// More specifically, the full proof consists of `forest`, `position` and `path` components. This
/// value constitutes the `path`. The other two components can be obtained as follows:
/// - `position` is simply `resopnse.block_header.block_num`
/// - `forest` is the same as `response.chain_tip + 1`
/// An MMR proof can be constructed for the leaf of index `block_header.block_num` of
/// an MMR of forest `chain_tip` with this path.
#[prost(message, optional, tag = "3")]
pub mmr_path: ::core::option::Option<super::merkle::MerklePath>,
/// List of all notes together with the Merkle paths from `response.block_header.note_root`
Expand Down
4 changes: 2 additions & 2 deletions crates/rust-client/src/store/web_store/accounts/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,14 @@ impl WebStore {
pub(super) async fn get_account_code(
&self,
root: Digest,
) -> Result<(Vec<Digest>, ModuleAst), StoreError> {
) -> Result<(Vec<u8>, ModuleAst), StoreError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leaving a small note here to any reviewers that this change is in here because this PR #456 modified the way account code procedures are stored in the client and unintentionally broke account storage/retrieval in the web client due to this small detail. Flagging because this small fix seems out of place with the rest of the testing code!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for calling this out

Copy link
Contributor

Choose a reason for hiding this comment

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

Something seems a bit out of sync here: get_account_code on next has a slightly different signature (i.e., it returns AccountCode struct rather than a tuple as in this PR).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed with @dagarcia7 and decided to remove from this PR. His work is overlapping with this and we plan on bringing in the account tests later on alongside it.

let root_serialized = root.to_string();

let promise = idxdb_get_account_code(root_serialized);
let js_value = JsFuture::from(promise).await.unwrap();
let account_code_idxdb: AccountCodeIdxdbObject = from_value(js_value).unwrap();

let procedures = serde_json::from_str(&account_code_idxdb.procedures).unwrap();
let procedures = account_code_idxdb.procedures;

let module = ModuleAst::from_bytes(&account_code_idxdb.module).unwrap();

Expand Down
3 changes: 2 additions & 1 deletion crates/rust-client/src/store/web_store/accounts/models.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ use serde::{de::Error, Deserialize, Deserializer, Serialize};
#[derive(Serialize, Deserialize)]
pub struct AccountCodeIdxdbObject {
pub root: String,
pub procedures: String,
#[serde(deserialize_with = "base64_to_vec_u8_required", default)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

pub procedures: Vec<u8>,
#[serde(deserialize_with = "base64_to_vec_u8_required", default)]
pub module: Vec<u8>,
}
Expand Down
6 changes: 6 additions & 0 deletions crates/web-client/.mocharc.json
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
}
8 changes: 4 additions & 4 deletions crates/web-client/js/types/index.d.ts
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";
20 changes: 16 additions & 4 deletions crates/web-client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,27 @@
"../LICENSE.md"
],
"scripts": {
"build": "rimraf dist && rollup -c rollup.config.js && cpr js/types dist && rimraf dist/wasm*"
"build": "rimraf dist && rollup -c rollup.config.js && cpr js/types dist && rimraf dist/wasm*",
"test": "node --loader ts-node/esm --loader esm ./node_modules/mocha/bin/mocha --file ./test/mocha.global.setup.mjs",
"test:clean": "npm install && npm run build && node --loader ts-node/esm --loader esm ./node_modules/mocha/bin/mocha --file ./test/mocha.global.setup.mjs"
},
"devDependencies": {
"@rollup/plugin-commonjs": "^25.0.7",
"@rollup/plugin-node-resolve": "^15.2.3",
"@wasm-tool/rollup-plugin-rust": "^2.4.5",
"@types/chai": "^4.3.17",
"@types/mocha": "^10.0.7",
"@types/node": "^22.4.1",
"chai": "^5.1.1",
"cpr": "^3.0.1",
"rimraf": "^5.0.1",
"rollup": "^3.27.2"
"esm": "^3.2.25",
"http-server": "^14.1.1",
"mocha": "^10.7.3",
"puppeteer": "^23.1.0",
"rimraf": "^6.0.1",
"rollup": "^3.27.2",
"ts-node": "^10.9.2",
"typescript": "^5.5.4",
"@wasm-tool/rollup-plugin-rust": "wasm-tool/rollup-plugin-rust"
},
"dependencies": {
"dexie": "^4.0.1"
Expand Down
57 changes: 57 additions & 0 deletions crates/web-client/test/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# Testing
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should specify a node version somewhere the supported node version.


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. In crates/web-client run `yarn test` to run all tests
igamigo marked this conversation as resolved.
Show resolved Hide resolved
2. 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
2. `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
20 changes: 20 additions & 0 deletions crates/web-client/test/account.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { expect } from "chai";
import {
createNewWallet,
getAccount,
getAccounts,
} from "./webClientTestUtils.js";

describe("account tests", () => {
it("get accounts", async () => {
const accountId = await createNewWallet("OffChain", false);
const accounts = await getAccounts();
expect(accounts.find((acc) => acc.id === accountId)).to.be.not.null;
});

it("get account", async () => {
const accountId = await createNewWallet("OffChain", false);
const result = await getAccount(accountId);
expect(result).to.be.equal(accountId);
});
});
15 changes: 15 additions & 0 deletions crates/web-client/test/faucet.test.ts
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",
Copy link
Collaborator

Choose a reason for hiding this comment

The 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);
});
});
5 changes: 5 additions & 0 deletions crates/web-client/test/global.test.d.ts
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;
}
47 changes: 47 additions & 0 deletions crates/web-client/test/mocha.global.setup.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
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 LOCAL_SERVER_PORT = 8080;
const LOCAL_SERVER = `http://localhost:${LOCAL_SERVER_PORT}`;

before(async function () {
console.log("Starting test server...");
serverProcess = exec(`http-server ./dist -p ${LOCAL_SERVER_PORT}`);
browser = await puppeteer.launch({ headless: true });
testingPage = await browser.newPage();
await testingPage.goto(LOCAL_SERVER);

// Uncomment below to enable console logging
// testingPage.on("console", (msg) => console.log("PAGE LOG:", msg.text()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about adding an env var like debug_mode or similar to enable console logging? I mean something like:

if env.debug_mode {
  testingPage.on("console", (msg) => console.log("PAGE LOG:", msg.text()));
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 () => {
const { WebClient } = await import("./index.js");
// let rpc_url = "http://18.203.155.106:57291";
let rpc_url = "http://localhost:57291";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed in the .github/workflows/test.yaml, part of the testing infrastructure is to spin up a local miden node. Would be curious to see if we can point set the RPC url to be that of the freshly spun up node? More of question for the miden team, but this makes sense for now 👍

Copy link
Collaborator Author

@julian-demox julian-demox Aug 28, 2024

Choose a reason for hiding this comment

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

Thanks for calling this out. I had meant to call this out a bit more in code and in general. I'm planning on adding a top level const to at the very least clarify this ports importance with a comment on it being kept in sync.

There's definitely more robust ways to do this that dont involve manually keeping it in sync. We could have the workflow install a toml parser, and then use that parsed value to pass into the .js via the environment, but I'm going to ask the Miden team for input on whether they'd prefer this given it adds some complexity to their github workflow when this port ultimately may never change .

const client = new WebClient();
await client.create_client(rpc_url);

window.client = client;
});
});
});

after(async function () {
console.log("Stopping test server...");
await browser.close();
serverProcess.kill();
});

export { testingPage };
9 changes: 9 additions & 0 deletions crates/web-client/test/wallet.test.ts
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
Copy link
Collaborator

Choose a reason for hiding this comment

The 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, get_accounts()) contained the new address. Maybe you were planning on this already for future tests but just in case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep that is part of future tests

Loading