test: Adding web client testing framework #498

julian-demox · 2024-08-27T18:55:17Z

No description provided.

dagarcia7 · 2024-08-28T15:23:30Z

crates/rust-client/src/store/web_store/accounts/mod.rs

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


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!

Thanks for calling this out

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

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.

dagarcia7 · 2024-08-28T15:24:11Z

crates/rust-client/src/store/web_store/accounts/models.rs

@@ -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)]


https://github.com/0xPolygonMiden/miden-client/pull/498/files?show-viewed-files=true&file-filters%5B%5D=#r1734891119

Same with this change!

dagarcia7 · 2024-08-28T15:47:03Z

.github/workflows/test.yml

@@ -29,6 +29,8 @@ jobs:
      - uses: actions/checkout@main
      - name: Install Rust
        run: rustup update --no-self-update
+      - name: Install Node.js


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!

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.

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

dagarcia7 · 2024-08-28T16:00:43Z

crates/web-client/tsconfig.json

+    // "strictFunctionTypes": true,                      /* When assigning functions, check to ensure parameters and the return values are subtype-compatible. */
+    // "strictBindCallApply": true,                      /* Check that the arguments for 'bind', 'call', and 'apply' methods match the original function. */
+    // "strictPropertyInitialization": true,             /* Check for class properties that are declared but not set in the constructor. */
+    // "noImplicitThis": true,                           /* Enable error reporting when 'this' is given the type 'any'. */


Should we remove all of the rules that have been commented out?

Will remove

dagarcia7 · 2024-08-28T16:05:42Z

crates/web-client/test/mocha.global.setup.mjs

+    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";


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 👍

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 .

dagarcia7 · 2024-08-28T16:15:13Z

crates/web-client/test/webClientTestUtils.js

Nice! This looks great. Only feedback here is it might be worth considering separating out the testing utils into separate locations based on what they do? I.e. account utils, transaction utils, note utils, etc. Don't think this will be an infinite file in the worst case, but might just get pretty long. Minor feedback, though, and isn't bad as is right now. Also some of these are unused since we're just adding a few tests to start so up to you on whether to keep, comment out, or delete for the official PR review!

I think its definitely something we have to keep an eye on. If we think the file is going to grow to double its current size (would be ~1000 lines). Then I believe splitting it up now makes sense. Otherwise, I like "monolithic" since this file has to implement some odd patterns that have unavoidable duplication of code to get the web context working correctly. It helps developer experience with copilot code completion and isolates the complexity.

julian-demox · 2024-08-28T19:14:16Z

crates/rust-client/src/rpc/web_tonic_client/generated/responses.rs

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


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

…st code per comments

crates/web-client/test/README.md

SantiagoPittella · 2024-09-06T18:29:13Z

.github/workflows/test.yml

@@ -29,6 +29,8 @@ jobs:
      - uses: actions/checkout@main
      - name: Install Rust
        run: rustup update --no-self-update
+      - name: Install Node.js


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.

SantiagoPittella · 2024-09-06T18:32:51Z

crates/web-client/test/mocha.global.setup.mjs

+  await testingPage.goto(TEST_SERVER);
+
+  // Uncomment below to enable console logging
+  // testingPage.on("console", (msg) => console.log("PAGE LOG:", msg.text()));


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())); }

Yep, I like that. I'll add some info to the README as well

SantiagoPittella · 2024-09-06T18:52:35Z

crates/web-client/test/README.md

@@ -0,0 +1,75 @@
+# Testing


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

SantiagoPittella · 2024-09-06T19:16:15Z

Looks like there is some issue in the process of terminating the http-server, locally it worked fine. But the CI is stuck in that process, we have tried by re-running and the error persisted.

The error:

Stopping test server...
  1) "after all" hook in "{root}"
  2 passing (11m)
  1 failing
  1) "after all" hook in "{root}":
     Error: Timeout of 600000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
      at listOnTimeout (node:internal/timers:569:17)
      at processTimers (node:internal/timers:512:7)

tomyrd

LGTM, tested on my machine and it works correctly.

Is there a way to cache some of the generated files to reduce subsequent builds? Right now it takes around 10 minutes to build each time:

Finished `release` profile [optimized] target(s) in 1m 33s
created dist in 12m 58.8s

tomyrd · 2024-09-06T19:10:55Z

crates/web-client/test/faucet.test.ts

+describe("faucet tests", () => {
+  it("create a new faucet", async () => {
+    const result = await createNewFaucet(
+      "OffChain",


Just a heads up, the storage mode names will change to "Private" and "Public" when #514 gets merged.

tomyrd · 2024-09-06T19:24:23Z

crates/web-client/test/webClientTestUtils.js

This might be planned for a future PR but we should also add tests for all of the other web client functions referenced here.

Yep! We have a POC on our branches for adding all tests. There were just some bugs we had to work out before adding them in

julian-demox · 2024-09-06T19:45:19Z

Looks like there is some issue in the process of terminating the http-server, locally it worked fine. But the CI is stuck in that process, we have tried by re-running and the error persisted.

The error:
Stopping test server...
  1) "after all" hook in "{root}"
  2 passing (11m)
  1 failing
  1) "after all" hook in "{root}":
     Error: Timeout of 600000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.
      at listOnTimeout (node:internal/timers:569:17)
      at processTimers (node:internal/timers:512:7)

I've added what I believe was a missing await that hopefully address it...

Was having trouble reproducing on my local, I'll go back to that if it doesnt fix it

igamigo

Not too familiar with some of the components of the test harness, but overall code LGTM! Left some minor comments as well.
Additionally, it seems that the new CI job fails with this error:

[!] (plugin rust) Error: Could not load /home/runner/work/miden-client/miden-client/crates/web-client/Cargo.toml (imported by js/wasm.js): info: syncing channel updates for '1.80-x86_64-unknown-linux-gnu'

Not sure what the way to fix it would be (maybe related to available rust versions or targets?), but let me know if I can help.

igamigo · 2024-09-09T14:02:40Z

crates/web-client/test/webClientTestUtils.js

Many of the functions in this file do not seem to be used right now, so I'd remove them from this PR. I'm guessing future tests would use them, and you also mention you have some other tests you are planning on adding later so I don't mind leaving them but it would make future PRs a bit easier to follow if they use code they are adding. Feel free to disregard this comment if the subsequent tests are going to be up for review soon!

Yes we have a follow up PR coming that should add everything.

igamigo · 2024-09-09T14:04:50Z

crates/web-client/test/wallet.test.ts

+describe("wallet tests", () => {
+  it("create a new wallet", async () => {
+    const result = await createNewWallet("OffChain", false);
+
+    isValidAddress(result);
+  });
+});


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.

Yep that is part of future tests

igamigo · 2024-09-09T14:05:23Z

.github/workflows/test.yml

nit: missing newline on this file

julian-demox · 2024-09-09T17:24:59Z

Not too familiar with some of the components of the test harness, but overall code LGTM! Left some minor comments as well. Additionally, it seems that the new CI job fails with this error:
[!] (plugin rust) Error: Could not load /home/runner/work/miden-client/miden-client/crates/web-client/Cargo.toml (imported by js/wasm.js): info: syncing channel updates for '1.80-x86_64-unknown-linux-gnu'
Not sure what the way to fix it would be (maybe related to available rust versions or targets?), but let me know if I can help.

I've added a new commit that attempts to fix this. Been trying to get a test environment on my local that mirrors the github workflow to avoid repeatedly posting commits for testing, but the replication ability is limited.

The newest commit seems to get past the above quoted issue and runs into others mid-test, but I believe those issues were isolated to my local docker container and not related to the workflow itself.

* test: Adding web client testing framework

test: Adding web client testing framework

936bbb5

julian-demox changed the title ~~[DRAFT] test: Adding web client testing framework~~ test: Adding web client testing framework Aug 27, 2024

dagarcia7 reviewed Aug 28, 2024

View reviewed changes

julian-demox commented Aug 28, 2024

View reviewed changes

Fixing generated responses in rust client & cleaning up web client te…

af79f9f

…st code per comments

julian-demox marked this pull request as ready for review August 28, 2024 19:23

tomyrd reviewed Aug 30, 2024

View reviewed changes

crates/web-client/test/README.md Show resolved Hide resolved

julian-demox added 6 commits September 3, 2024 14:12

Adding missing README steps

401b5d5

Reverting account changes & tests

1fe0799

Merge branch 'next' into web-client-testing

8701bbd

Adding logic to ensure server is stopped before exiting tests

3e14482

Updating README for troubleshooting

aa9e51f

Added to CHANGELOG

ed78776

SantiagoPittella reviewed Sep 6, 2024

View reviewed changes

Updating github workflows and adding some test setup instructions

b9313c4

tomyrd approved these changes Sep 6, 2024

View reviewed changes

Fix for integ test workflow hanging

dfbe340

Specifying test workflow node version

ab72e83

igamigo approved these changes Sep 9, 2024

View reviewed changes

Adding step to web client test to fix workflow issue

61df391

julian-demox added 2 commits September 9, 2024 13:38

Fixing syntax issue in github workflow

a8b273d

Properly fixing the indentation issue in the yaml

03aa4b1

julian-demox added 2 commits September 9, 2024 14:48

Adding more verbose logs to the teardown step + more conditional logic

cbb721b

Switched from exec to spawn

1ada1d1

julian-demox merged commit ab68e6b into 0xPolygonMiden:next Sep 10, 2024
14 checks passed

dagarcia7 mentioned this pull request Sep 10, 2024

Add Custom Transactions #519

Merged

dagarcia7 mentioned this pull request Sep 26, 2024

Added Account Integration Tests for Web Client #532

Merged

igamigo pushed a commit that referenced this pull request Nov 7, 2024

test: Adding web client testing framework (#498)

47a3092

* test: Adding web client testing framework

igamigo pushed a commit that referenced this pull request Nov 7, 2024

test: Adding web client testing framework (#498)

6bb3930

* test: Adding web client testing framework

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Adding web client testing framework #498

test: Adding web client testing framework #498

julian-demox commented Aug 27, 2024

dagarcia7 Aug 28, 2024

julian-demox Aug 28, 2024

bobbinth Aug 29, 2024

julian-demox Sep 3, 2024

dagarcia7 Aug 28, 2024

dagarcia7 Aug 28, 2024

SantiagoPittella Sep 6, 2024

julian-demox Sep 6, 2024

dagarcia7 Aug 28, 2024

julian-demox Aug 28, 2024

dagarcia7 Aug 28, 2024

julian-demox Aug 28, 2024 •

edited

Loading

dagarcia7 Aug 28, 2024

julian-demox Aug 28, 2024

julian-demox Aug 28, 2024 •

edited

Loading

SantiagoPittella Sep 6, 2024

SantiagoPittella Sep 6, 2024

julian-demox Sep 6, 2024

SantiagoPittella Sep 6, 2024

SantiagoPittella commented Sep 6, 2024

tomyrd left a comment

tomyrd Sep 6, 2024

tomyrd Sep 6, 2024

julian-demox Sep 6, 2024

julian-demox commented Sep 6, 2024 •

edited

Loading

igamigo left a comment

igamigo Sep 9, 2024

julian-demox Sep 9, 2024

igamigo Sep 9, 2024

julian-demox Sep 9, 2024

igamigo Sep 9, 2024

julian-demox commented Sep 9, 2024

test: Adding web client testing framework #498

test: Adding web client testing framework #498

Conversation

julian-demox commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julian-demox Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julian-demox Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SantiagoPittella commented Sep 6, 2024

tomyrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julian-demox commented Sep 6, 2024 • edited Loading

igamigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julian-demox commented Sep 9, 2024

julian-demox Aug 28, 2024 •

edited

Loading

julian-demox Aug 28, 2024 •

edited

Loading

julian-demox commented Sep 6, 2024 •

edited

Loading