feat: [Issue-185] Token Provider Tests #365

normand1 · 2024-11-16T20:12:11Z

Relates to:

#185

Risks

Low: Only modification to src code files was adding optional syntax an

Background

What does this PR do?

Removes jest in favor of vitest as discussed in this issue: Core Unit Tests #340
Fixes tests that were no longer running in packages/core/src/tests/token.test.ts

What kind of change is this?

Improvements - Tests

See Issue above

Documentation changes needed?

My changes do not require a change to the project documentation.

Testing

pnpm -w run test -- 'src/tests/token.test.ts'

Where should a reviewer start?

pnpm install
pnpm run build
pnpm -w run test -- 'src/tests/token.test.ts'

Detailed testing steps

normand1 · 2024-11-17T03:44:11Z

@ponderingdemocritus This is based on your earlier instructions to include and use vitest:
#340 (comment)

ponderingdemocritus · 2024-11-17T03:58:25Z

packages/adapter-sqlite/package.json

+    },
+    "exports": {
+        ".": "./dist/index.js",
+        "./sqlite_vec": "./dist/sqlite_vec.js"


i don't think this is needed - exported at index

ponderingdemocritus · 2024-11-17T04:01:01Z

packages/plugin-solana/src/index.ts

+import { Connection } from "@solana/web3.js";
+import { PublicKey } from "@solana/web3.js";
+
+export { TokenProvider, WalletProvider, Connection, PublicKey };


remove the solana exports and import where needed from the packages

Good catch, this was left over from another test I was working on, but ended up simplifying for this first PR

ponderingdemocritus

good start - see comments

normand1 · 2024-11-17T04:23:12Z

@ponderingdemocritus Thanks for the feedback! Just pushed an update based on comments

monilpat · 2024-11-18T23:05:51Z

packages/core/src/tests/token.test.ts

+            "2weMjPLLybRMMva1fM3U31goWWrCpF59CHWNhnCJ9Vyh",
+            new WalletProvider(
+                new Connection("https://api.mainnet-beta.solana.com"),
+                new PublicKey("2weMjPLLybRMMva1fM3U31goWWrCpF59CHWNhnCJ9Vyh")
+            )


Instead of hard coding these can we move these into a constants file? Thank you!

monilpat · 2024-11-18T23:06:53Z

packages/core/src/tests/token.test.ts

+        // Ensure the mock was called
+        expect(fetchSpy).toHaveBeenCalled();
+


We should remove this as the line below does the same check and there this is redundant

monilpat · 2024-11-18T23:09:58Z

packages/core/src/test_resources/types.ts

@@ -0,0 +1,6 @@
+export interface User {


I am surprised that this type hasn't already been declared and can't be reused. Could be mistaken but wanted to point it out thanks

This type is intended to generically represent the different User types used across packages in the agent runtime based on whether we're using sqlite, supabase or sqljs. I couldn't find an abstract type that currently exists for this, but I'd be happy to replace it here if there is one. If there isn't one then I agree this would be helpful to have a real type that does this instead of this one sitting in a test file :)

monilpat · 2024-11-18T23:11:08Z

packages/core/src/test_resources/createRuntime.ts

+                user = {
+                    id: zeroUuid,
+                    email: "test@example.com",
+                } as User;
+                session = {
+                    user: user,
+                };


Nit: user can be inlined here

monilpat · 2024-11-18T23:12:19Z

packages/core/src/test_resources/createRuntime.ts

+                user = {
+                    id: zeroUuid,
+                    email: "test@example.com",
+                } as User;
+                session = {
+                    user: user,
+                };


monilpat · 2024-11-18T23:12:36Z

packages/core/src/test_resources/createRuntime.ts

+    }
+
+    const runtime = new AgentRuntime({
+        serverUrl: "https://api.openai.com/v1",


probably a const we can use here

monilpat · 2024-11-18T23:13:09Z

packages/core/src/test_resources/basic.test.ts

+import { describe, expect, it } from "vitest";
+
+describe("Basic Test Suite", () => {
+    it("should run a basic test", () => {


Please remove this

monilpat

Overall looks good thanks for doing this - some code quality changes to make then can accept!

monilpat

Thanks for working on this. It's ready to go after addressing comments

normand1 · 2024-11-18T23:42:51Z

Thanks for working on this. It's ready to go after addressing comments

Awesome thank you for the review, taking a look at making those updates now

monilpat · 2024-11-19T00:27:34Z

Also, forgot to mention please address the merge conflicts thanks :)

normand1 · 2024-11-19T02:09:27Z

@monilpat I believe I've addressed all feedback, ready for someone to take another look, thanks!

jkbrooks · 2024-11-19T20:38:04Z

FYI, we now have two conflicting files here

packages/core/src/tests/goals.test.ts
pnpm-lock.yaml

@monilpat @normand1 can we resolve before merge?

normand1 · 2024-11-20T00:09:13Z

@jkbrooks Resolved conflicts

monilpat · 2024-11-20T00:11:43Z

Cool with the resolved conflicts LGTM!

normand1 added 8 commits November 15, 2024 15:27

Merge branch 'main' of https://github.com/ai16z/eliza

64f3231

Merge branch 'main' of https://github.com/ai16z/eliza

9042e7b

use vite

d778e60

test running

c93510c

cleanup

4d7e0aa

cleanup

a3eb3ed

cleanup

14f1cff

cleanup

7940692

normand1 mentioned this pull request Nov 16, 2024

Core Unit Tests #340

Closed

normand1 added 2 commits November 16, 2024 14:46

remove log statement

780cfb2

merge from main

5a338d0

ponderingdemocritus changed the title ~~[Issue-185] Token Provider Tests~~ feat: [Issue-185] Token Provider Tests Nov 17, 2024

ponderingdemocritus reviewed Nov 17, 2024

View reviewed changes

review feedback updates

055c609

monilpat reviewed Nov 18, 2024

View reviewed changes

monilpat requested changes Nov 18, 2024

View reviewed changes

merge from main

5e24871

normand1 added 3 commits November 18, 2024 19:41

update to address review feedback

caa13f7

inline user model

6fbde25

remove hardcoded endpoint

ff28e99

handle merge conflicts

329db11

jkbrooks merged commit bea5567 into elizaOS:main Nov 20, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [Issue-185] Token Provider Tests #365

feat: [Issue-185] Token Provider Tests #365

normand1 commented Nov 16, 2024 •

edited

Loading

normand1 commented Nov 17, 2024

ponderingdemocritus Nov 17, 2024

ponderingdemocritus Nov 17, 2024

normand1 Nov 17, 2024

ponderingdemocritus left a comment

normand1 commented Nov 17, 2024 •

edited

Loading

monilpat Nov 18, 2024 •

edited

Loading

monilpat Nov 18, 2024

monilpat Nov 18, 2024

normand1 Nov 19, 2024

monilpat Nov 18, 2024

monilpat Nov 18, 2024

monilpat Nov 18, 2024

monilpat Nov 18, 2024

monilpat left a comment

monilpat left a comment

normand1 commented Nov 18, 2024

monilpat commented Nov 19, 2024

normand1 commented Nov 19, 2024

jkbrooks commented Nov 19, 2024

normand1 commented Nov 20, 2024

monilpat commented Nov 20, 2024

		// Ensure the mock was called
		expect(fetchSpy).toHaveBeenCalled();

feat: [Issue-185] Token Provider Tests #365

feat: [Issue-185] Token Provider Tests #365

Conversation

normand1 commented Nov 16, 2024 • edited Loading

Relates to:

Risks

Background

What does this PR do?

What kind of change is this?

Documentation changes needed?

Testing

Where should a reviewer start?

Detailed testing steps

normand1 commented Nov 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ponderingdemocritus left a comment

Choose a reason for hiding this comment

normand1 commented Nov 17, 2024 • edited Loading

monilpat Nov 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monilpat left a comment

Choose a reason for hiding this comment

monilpat left a comment

Choose a reason for hiding this comment

normand1 commented Nov 18, 2024

monilpat commented Nov 19, 2024

normand1 commented Nov 19, 2024

jkbrooks commented Nov 19, 2024

normand1 commented Nov 20, 2024

monilpat commented Nov 20, 2024

normand1 commented Nov 16, 2024 •

edited

Loading

normand1 commented Nov 17, 2024 •

edited

Loading

monilpat Nov 18, 2024 •

edited

Loading