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

docs: Create best-practices.md documentation #463

Merged
merged 2 commits into from
Nov 21, 2024
Merged

Conversation

snobbee
Copy link
Contributor

@snobbee snobbee commented Nov 20, 2024

Relates to:

Eliza community’s best practices documentation

Risks

Low risk. This PR focuses on updating documentation to reflect the latest best practices for submitting pull requests, including pre-review with AI and template usage. No functional code changes are involved.

Background

What does this PR do?

This PR updates the contributing guidelines to include new best practices for pull request submissions. It adds a recommendation for conducting pre-reviews using AI tools, elaborates on the use of PR templates, and emphasizes the importance of squashing commits.

What kind of change is this?

  • Documentation: This PR add the best practices documentation to streamline the contribution process and make it more consistent for all team members.

Documentation changes needed?

My changes require an update to the project documentation. The best practices guidelines have been added to include instructions about using AI tools for pre-review, PR templates, and squashing commits. All updates have been made accordingly.

Testing

Where should a reviewer start?

Review the updated sections on "Pre-Review with AI," "Pull Request Templates," and "Squashing Commits" in the best-practices documentation.

Copy link
Collaborator

@monilpat monilpat left a comment

Choose a reason for hiding this comment

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

Thanks for working on this feel free to address changes and then can accept :)

docs/docs/community/best-practices.md Outdated Show resolved Hide resolved
docs/docs/community/best-practices.md Outdated Show resolved Hide resolved
docs/docs/community/best-practices.md Outdated Show resolved Hide resolved
@snobbee
Copy link
Contributor Author

snobbee commented Nov 21, 2024

@monilpat here is the result of the command pnpm test:

$ pnpm test

> eliza@ test /tmp/eliza
> bash ./scripts/test.sh

Testing package: plugin-video-generation
No test script found in plugin-video-generation, skipping tests...
Testing package: client-auto
No test script found in client-auto, skipping tests...
Testing package: plugin-node
No test script found in plugin-node, skipping tests...
Testing package: core
npm error A complete log of this run can be found in: /tmp/.npm/_logs/2024-11-21T12_01_12_966Z-debug-0.log
Running tests for package: core

> @ai16z/eliza@0.1.3 test
> vitest run


 RUN  v2.1.5 /tmp/eliza/packages/core

stdout | src/tests/token.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/providers.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/defaultCharacters.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/messages.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/evaluators.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/videoGeneration.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/database.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/goals.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/models.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/relationships.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/posts.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/env.test.ts
Current directory: /tmp/eliza/packages/core/src/test_resources
Trying to load env from: /tmp/eliza/packages/core/.env.test

stdout | src/tests/env.test.ts > Environment Setup > should verify .env.test file exists
Current working directory: /tmp/eliza/packages/core
__dirname: /tmp/eliza/packages/core/src/tests
Path /tmp/eliza/packages/core/.env.test exists: true
Path /tmp/eliza/packages/core/packages/core/.env.test exists: false
Path /tmp/eliza/packages/core/.env.test exists: true
Path /tmp/eliza/packages/core/src/.env.test exists: false
Path /tmp/eliza/packages/core/src/tests/.env.test exists: false

 ✓ src/tests/database.test.ts (8)
 ✓ src/tests/goals.test.ts (8)
 ✓ src/tests/database.test.ts (8)
 ✓ src/tests/defaultCharacters.test.ts (11)
 ✓ src/tests/env.test.ts (1)
 ✓ src/tests/evaluators.test.ts (4)
 ✓ src/tests/goals.test.ts (8)
 ✓ src/tests/messages.test.ts (6)
 ✓ src/tests/models.test.ts (7)
 ✓ src/tests/posts.test.ts (3)
 ✓ src/tests/providers.test.ts (5)
 ✓ src/tests/relationships.test.ts (6)
 ❯ src/tests/token.test.ts (0)
 ❯ src/tests/videoGeneration.test.ts (0)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Suites 2 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  src/tests/token.test.ts [ src/tests/token.test.ts ]
Error: Failed to load url @ai16z/adapter-sqlite (resolved id: @ai16z/adapter-sqlite) in /tmp/eliza/packages/core/src/test_resources/createRuntime.ts. Does the file exist?
 ❯ loadAndTransform ../../node_modules/vite/dist/node/chunks/dep-CB_7IfJ-.js:51920:17

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/2]⎯

 FAIL  src/tests/videoGeneration.test.ts [ src/tests/videoGeneration.test.ts ]
Error: [vitest] No "default" export is defined on the "fs" mock. Did you forget to return it from "vi.mock"?
If you need to partially mock a module, you can use "importOriginal" helper inside:

vi.mock(import("fs"), async (importOriginal) => {
  const actual = await importOriginal()
  return {
    ...actual,
    // your mocked methods
  }
})

 ❯ findNearestEnvFile src/settings.ts:36:13
     34|         const envPath = path.join(currentDir, ".env");
     35| 
     36|         if (fs.existsSync(envPath)) {
       |             ^
     37|             return envPath;
     38|         }
 ❯ loadEnvConfig src/settings.ts:70:21
 ❯ src/settings.ts:110:61

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/2]⎯

 Test Files  2 failed | 10 passed (12)
      Tests  59 passed (59)
   Start at  13:01:13
   Duration  471ms (transform 493ms, setup 151ms, collect 569ms, tests 29ms, environment 1ms, prepare 757ms)

Tests failed for core
Testing package: plugin-solana
No test script found in plugin-solana, skipping tests...
Testing package: adapter-sqlite
No test script found in adapter-sqlite, skipping tests...
Testing package: plugin-trustdb
npm error A complete log of this run can be found in: /tmp/.npm/_logs/2024-11-21T12_01_13_920Z-debug-0.log
Running tests for package: plugin-trustdb

> @ai16z/plugin-trustdb@0.1.3 test
> vitest run


 RUN  v2.1.5 /tmp/eliza/packages/plugin-trustdb

include: **/*.{test,spec}.?(c|m)[jt]s?(x)
exclude:  **/node_modules/**, **/dist/**, **/cypress/**, **/.{idea,git,cache,output,temp}/**, **/{karma,rollup,webpack,vite,vitest,jest,ava,babel,nyc,cypress,tsup,build,eslint,prettier}.config.*

No test files found, exiting with code 1
Tests failed for plugin-trustdb
Testing package: create-eliza-app
No test script found in create-eliza-app, skipping tests...
Testing package: plugin-starknet
npm error A complete log of this run can be found in: /tmp/.npm/_logs/2024-11-21T12_01_14_305Z-debug-0.log
Running tests for package: plugin-starknet

> @ai16z/plugin-starknet@0.1.3 test
> vitest run


 RUN  v2.1.5 /tmp/eliza/packages/plugin-starknet

include: **/*.{test,spec}.?(c|m)[jt]s?(x)
exclude:  **/node_modules/**, **/dist/**, **/cypress/**, **/.{idea,git,cache,output,temp}/**, **/{karma,rollup,webpack,vite,vitest,jest,ava,babel,nyc,cypress,tsup,build,eslint,prettier}.config.*

No test files found, exiting with code 1
Tests failed for plugin-starknet
Testing package: adapter-sqljs
No test script found in adapter-sqljs, skipping tests...
Testing package: plugin-bootstrap
No test script found in plugin-bootstrap, skipping tests...
Testing package: client-telegram
No test script found in client-telegram, skipping tests...
Testing package: client-discord
No test script found in client-discord, skipping tests...
Testing package: client-direct
No test script found in client-direct, skipping tests...
Testing package: client-twitter
No test script found in client-twitter, skipping tests...
Testing package: adapter-postgres
No test script found in adapter-postgres, skipping tests...
Testing package: plugin-image-generation
No test script found in plugin-image-generation, skipping tests...
Testing package: adapter-supabase
No test script found in adapter-supabase, skipping tests...
Test process completed.😎

Copy link
Collaborator

@monilpat monilpat left a comment

Choose a reason for hiding this comment

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

LGTM!

@monilpat
Copy link
Collaborator

Can you create issues for the tests that failed ? Thanks

@jkbrooks jkbrooks merged commit f233f78 into elizaOS:main Nov 21, 2024
2 checks passed
@snobbee snobbee deleted the patch-1 branch November 22, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants