Added Account Integration Tests for Web Client #532

dagarcia7 · 2024-09-26T03:11:34Z

Summary

In order to prevent regressions on the web client as the rust and web-clients develop, we need integration tests that can be run via github workflows to test the basic functionality of the web client. In #498, the infrastructure was added to be able to implement mocha tests for the web client and have them execute via a github action workflow. However, no real tests were added at the time.

This PR adds the first batch of these tests. The code tested are the code under the new_account.rs and account.rs files. in the web-client. Incoming in future PRs will be tests for notes.rs, sync.rs, and transactions.rs, and new_transactions.rs.

dagarcia7 · 2024-09-26T03:15:45Z

crates/web-client/test/webClientTestUtils.js

@@ -6,107 +6,6 @@ import { testingPage } from "./mocha.global.setup.mjs";
 * @typedef {import("../dist/index").AccountId} AccountId
 */

-/**


I think for each testing file, we'll need setup functions like this that leverage the use of the testingPage and an actual browser context to perform certain actions in WASM and return certain data to be checked in the actual tests. For that reason, I've started removing these functions from here and replaced them with similar relevant code in each new testing file. As I add more integration tests and more relevant files, I will continue removing stuff from this file.

dagarcia7 · 2024-09-26T03:16:26Z

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

Removed this file and replaced it with new_account.test.ts, and account.test.ts.

dagarcia7 · 2024-09-26T03:19:31Z

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

+      return {
+        id: newWallet.id().to_string(),
+        nonce: newWallet.nonce().to_string(),
+        vault_commitment: newWallet.vault().commitment(),
+        storage_commitment: newWallet.storage().commitment(),
+        code_commitment: newWallet.code().commitment(),
+        is_faucet: newWallet.is_faucet(),
+        is_regular_account: newWallet.is_regular_account(),
+        is_updatable: newWallet.is_updatable(),
+        is_public: newWallet.is_public(),
+        is_new: newWallet.is_new(),
+      }
+    },


I can add a comment somewhere in the codebase if it helps, but testing the WASM is a bit interesting. Basically, you need puppeteer to create a browser context that loads and uses the WASM. That being said, once you get to the actual unit tests, you don't have access to any WASM things because you no longer have the browser context. So until we figure out a better solution, for now, I'm just extracting any useful information I need from the resulting client calls and referencing this returned data in the unit tests.

dagarcia7 · 2024-09-26T03:22:29Z

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

+      window.Account = Account;
+      window.AccountHeader = AccountHeader;
+      window.AccountStorageMode = AccountStorageMode;
+      window.AuthSecretKey = AuthSecretKey


There are some checks in which I want to assert that the object returned from WASM is of a specific type. To do this, I have to attach the object type to the window here in the mocha global setup code so that it's accessible in the testingPage.evaluate scope in the testing files.

dagarcia7 · 2024-09-26T03:23:10Z

crates/web-client/test/global.test.d.ts

+declare global {
+  interface Window {
+    client: WebClient;
+    Account: typeof Account;
+    AccountHeader: typeof AccountHeader;
+    AccountStorageMode: typeof AccountStorageMode;
+    AuthSecretKey: typeof AuthSecretKey;
+    create_client: () => Promise<void>;
+  }
+}


This is another place I had to modify to make the instanceof assertions work.

Great idea! I was struggling to find a way to type everything cleanly in the .ts mocha context, this is way more straightforward.

dagarcia7 · 2024-09-26T03:26:12Z

crates/web-client/src/new_account.rs

Taking advantage while writing these tests to slightly clean up the JS <-> Rust Web Client contracts. In this particular file, there is some better error handling, so type changes to the inputs (actual u8s, u64s, etc. passed as inputs as opposed to strings), and changing the outputs to an actual Account object.

dagarcia7 · 2024-09-26T03:30:12Z

crates/web-client/src/models/asset_vault.rs

+
+#[wasm_bindgen]
+impl AssetVault {
+    pub fn commitment(&self) -> String {


Just exposed this for now, but can add to it later. Just didn't want to bloat the testing PR much more than I already did with error handling and interface polishing.

dagarcia7 · 2024-09-26T04:01:21Z

crates/web-client/src/account.rs

        } else {
            Err(JsValue::from_str("Client not initialized"))
        }
    }

-    pub async fn get_account(&mut self, account_id: String) -> Result<JsValue, JsValue> {
+    pub async fn get_account(&mut self, account_id: &str) -> Result<Account, JsValue> {


I think get_account is good taking in a &str, but eventually i want everything else to be AccountId. But in order to do this, I have to be able to create mock accountIds without actually creating them in the Database so for now just having things take &str as account Ids.

Is the idea to be able to just create arbitrary AccountIds? There's AccountId::from_hex(&str) or AccountId::try_from(u64) which you can use with these constants for example.

Oh, this is awesome! Thanks for linking. I created a TestUtils, exposed it to the WASM, and added a create_mock_account_id() method that uses one of these constants to create an AccountId object. Updated all of the account interfaces to take in AccountId objects as a result now. Thanks so much for pointing this out!

julian-demox · 2024-09-26T19:30:49Z

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

+    it("returns error attempting to retrieve a non-existing account", async () => {
+        await expect(
+            getAccountNoMatch()
+          ).to.be.rejectedWith("Failed to get account: Store error: Account data was not found for Account Id 0x1111111111111111");


Is there any specific Error type that we can check for rather than the string output? I'm guessing not, but would be nice in case any messaging upstream changes

Yeah good question. I replied here with what I was thinking on this front. Maybe exposing an Errors struct in rust for WASM that handles conversions between the rust-client errors and transforms them to errors we can actually return. That way, we can change the signature of something like get_accounts to be something like Result<Vec<AccountHeader>, GetAccountsError>> or something like this as opposed to Result<Vec<AccountHeader>, JsValue>>.

julian-demox · 2024-09-26T19:32:12Z

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

+        }
+
+        const client = window.client;
+        await client.get_account("0x1111111111111111");


nit: Would be nice to create a "test-data.ts" type file where we can put dummy values that can be used throughout

So for this line specifically, I actually went with @igamigo's suggestion here and exposed a TestUtils struct in Rust for WASM. This lets us leverage any mocking capabilities that the rust-client exposes for the purpose of those tests, and we in turn can create functions that let us take advantage of this in WASM. For this example specifically, I can now call window.TestUtils.create_mock_account_id to create a fake value that can be parsed into an AccountId.

In general, though, definitely agree that if we find ourselves needing things that are useful across all the tests like dummy values or functions like isValidAddress, then a test-data.ts or test-utils.ts file definitely makes sense 👍

julian-demox · 2024-09-26T19:40:21Z

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

+// new_wallet tests
+// =======================================================================================================
+
+export const createNewWallet = async (


Not blocking for this PR.

I agree with the choice to move some of the functions out of the utils file, but functions like new wallet and new faucet might be useful to be shared across tests, no? We could make that file a .ts file now too, with your changes to the global window typing.

Yeah, good point! I think the reusable parts of the code in this file are the setup like this

return await testingPage.evaluate(async () => { if (!window.client) { await window.create_client(); } const client = window.client;

but I think beyond that, in every integration test, it shouldn't be too much effort to just call client.new_wallet once this setup code is in place and then do what we have to do (call mint, consume, sync, etc.) 🤔.

Idk I don't see it quite yet, but you're probably right and need to keep a pulse on this as the integration tests take better shape 👍

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

tomyrd · 2024-09-26T16:46:37Z

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

+};
+
+describe("fetch_and_cache_account_auth_by_pub_key tests", () => {
+    it("retrieves an existing account auth and caches it", async () => {


How are we sure that the account auth was cached?

Hm.. good point 🤔. So the "caching" for now is a simple JS map that gets maintained in memory when this function gets called. My thinking here was that for the purpose of this test (at least for now), it would be enough to verify that the proper account auth was fetch and returned because it would mean that the full JS query to indexedDB would have succeeded.

One question regarding this, the difference between client.get_account_auth and client.fetch_and_cache_account_auth_by_pub_key is that the second one caches the result?

igamigo

Leaving some comments for now! The issue with the CI is related to a bug with clippy which we are "fixing" with this PR: #534

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

igamigo · 2024-09-26T21:31:38Z

crates/web-client/src/account.rs

        } else {
            Err(JsValue::from_str("Client not initialized"))
        }
    }

-    pub async fn get_account(&mut self, account_id: String) -> Result<JsValue, JsValue> {
+    pub async fn get_account(&mut self, account_id: &str) -> Result<Account, JsValue> {


Is the idea to be able to just create arbitrary AccountIds? There's AccountId::from_hex(&str) or AccountId::try_from(u64) which you can use with these constants for example.

igamigo · 2024-09-26T21:38:12Z

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

+import { expect } from 'chai';
+import { testingPage } from "./mocha.global.setup.mjs";
+
+// get_account tests


nit: These are usually written in uppercase

igamigo · 2024-09-26T21:38:56Z

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

+            addressOfCreatedAccount: newAccount.id().to_string(),
+            addressOfGetAccountResult: result.id().to_string(),


Comparing account hashes might be more complete here, to see that integrity with code, nonce and storage is maintained.

Great suggestion! Updated 👍

tomyrd

LGTM, works correctly on my machine. The only thing I would like to mention is that when I purposefully added a throw new Error("..."); in the code, the error message shown by Mocha was Error: the string "..." was thrown, throw an Error :). It seems Mocha prefers Error values instead of strings, we could change them in the future.

* Web Client Account Integration Tests

dagarcia7 commented Sep 26, 2024

View reviewed changes

dagarcia7 force-pushed the web-integration-tests branch from 2821885 to 94e4e60 Compare September 26, 2024 05:10

Web Client Account Integration Tests

b1f5494

dagarcia7 force-pushed the web-integration-tests branch from 94e4e60 to b1f5494 Compare September 26, 2024 05:10

dagarcia7 marked this pull request as ready for review September 26, 2024 05:11

Auto generated script_root change

4e348d1

dagarcia7 force-pushed the web-integration-tests branch from 4e348d1 to 4cf4c41 Compare September 26, 2024 16:50

julian-demox reviewed Sep 26, 2024

View reviewed changes

tomyrd reviewed Sep 26, 2024

View reviewed changes

igamigo reviewed Sep 26, 2024

View reviewed changes

Feedback

01fcecb

dagarcia7 force-pushed the web-integration-tests branch from 4cf4c41 to 01fcecb Compare September 28, 2024 04:04

tomyrd approved these changes Sep 30, 2024

View reviewed changes

Merge branch 'next' into web-integration-tests

31ccfd9

julian-demox merged commit 22b6161 into 0xPolygonMiden:next Oct 1, 2024
14 checks passed

julian-demox deleted the web-integration-tests branch October 1, 2024 15:16

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

Added Account Integration Tests for Web Client (#532)

d3d0e8c

* Web Client Account Integration Tests

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

Added Account Integration Tests for Web Client (#532)

4bbf673

* Web Client Account Integration Tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Account Integration Tests for Web Client #532

Added Account Integration Tests for Web Client #532

dagarcia7 commented Sep 26, 2024

dagarcia7 Sep 26, 2024

dagarcia7 Sep 26, 2024

dagarcia7 Sep 26, 2024

dagarcia7 Sep 26, 2024

dagarcia7 Sep 26, 2024

julian-demox Sep 26, 2024

dagarcia7 Sep 26, 2024

dagarcia7 Sep 26, 2024

dagarcia7 Sep 26, 2024

igamigo Sep 26, 2024

dagarcia7 Sep 28, 2024

julian-demox Sep 26, 2024

dagarcia7 Sep 28, 2024

julian-demox Sep 26, 2024

dagarcia7 Sep 28, 2024

julian-demox Sep 26, 2024

dagarcia7 Sep 28, 2024

tomyrd Sep 26, 2024

dagarcia7 Sep 28, 2024

tomyrd Sep 30, 2024

igamigo left a comment

igamigo Sep 26, 2024

igamigo Sep 26, 2024

igamigo Sep 26, 2024

dagarcia7 Sep 28, 2024

tomyrd left a comment

		addressOfCreatedAccount: newAccount.id().to_string(),
		addressOfGetAccountResult: result.id().to_string(),

Added Account Integration Tests for Web Client #532

Added Account Integration Tests for Web Client #532

Conversation

dagarcia7 commented Sep 26, 2024

Summary

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

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

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

tomyrd left a comment

Choose a reason for hiding this comment