Skip to content

test: Initial release of smoke/integration tests + testing framework#993

Merged
mcp97 merged 14 commits into
elizaOS:developfrom
Sifchain:test/integration-test-poc
Dec 12, 2024
Merged

test: Initial release of smoke/integration tests + testing framework#993
mcp97 merged 14 commits into
elizaOS:developfrom
Sifchain:test/integration-test-poc

Conversation

@jzvikart
Copy link
Copy Markdown
Contributor

@jzvikart jzvikart commented Dec 11, 2024

Relates to:

#1014

Risks

When the proposed tests are configured to run in CI/CD environment, they need one or more API keys which are considered as secrets. It is necessary to ensure that they are not leaked through workflows being run on behalf of malicious pull requests. In addition to restrictions on running CI/CD workflows we recommend setting up a quota on those keys, and rotating them periodically.

Background

What does this PR do?

  1. This PR adds two basic tests:
  • smoke tests: checks that the project can be built and run
  • "Hello Trump": checks that the project can use one of the built-in characters (Trump) to successfully query a LLM (OpenAI).

The "Hello Trump" test requires OpenAI API keys to run. If the API keys are not available, this test will be skipped.

  1. This PR also adds initial (proof of concept) version of integration testing framework. We plan to continue development of this framework in the future as will be adding more integration tests.

Why are we doing this? Any context or related work?

The goal of these tests is to establish a quality gate that every PR should pass at a minimum. We recommend that the tests are run at least:

  • on every PR before merging it
  • every time the main/stable branch is updated

As a policy, merges to main/stable branch should not be allowed if they would break the tests.

Documentation changes needed?

The included file tests/README.md describes the procedure how to set the tests up in a CI/CD environment. The API keys should be set up the repository settings under "secrets". When present, they will automatically be added to the .env file for the duration of the workflow.

Testing

Where should a reviewer start?

This PR adds a workflow file integrationTests.yaml with two jobs. The reviewer should:

  1. Configure API keys as described in the tests/README.md
  2. Re-run the workflows and check that both test are passing
  3. Check output of jobs for any errors.

Deploy Notes

  1. Configure API keys as described in the tests/README.md
  2. Re-run the workflows and check that both test are passing
  3. Check output of jobs for any errors.
  4. Adjust workflow rules to trigger workflows as desired (on pull request, etc.)

Discord username

user98634

odilitime
odilitime previously approved these changes Dec 11, 2024
@mcp97 mcp97 changed the title Initial release of smoke/integration tests + testing framework test:Initial release of smoke/integration tests + testing framework Dec 11, 2024
Copy link
Copy Markdown
Contributor

@mcp97 mcp97 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 doing this once the CI/CD failures are resolved then good to go

@odilitime odilitime changed the title test:Initial release of smoke/integration tests + testing framework feat:Initial release of smoke/integration tests + testing framework Dec 11, 2024
@odilitime odilitime changed the title feat:Initial release of smoke/integration tests + testing framework test:Initial release of smoke/integration tests + testing framework Dec 11, 2024
@mcp97 mcp97 changed the title test:Initial release of smoke/integration tests + testing framework test: Initial release of smoke/integration tests + testing framework Dec 11, 2024
@pgoos
Copy link
Copy Markdown
Contributor

pgoos commented Dec 12, 2024

Ready for final review.

@jzvikart jzvikart changed the base branch from main to develop December 12, 2024 16:32
@jzvikart
Copy link
Copy Markdown
Contributor Author

Changed target branch to develop due to policy change.

Copy link
Copy Markdown
Contributor

@pgoos pgoos left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@mcp97 mcp97 merged commit 3faf92a into elizaOS:develop Dec 12, 2024

- name: Run integration tests
env:
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

GitHub does not allow this secret to be accessible for github action runs from forked repos.

See: pull_request_target:

as well as github's blog post: https://github.blog/news-insights/product-news/github-actions-improvements-for-fork-and-pull-request-workflows/

there are some security concerns with allowing secrets to be added to runtime envs of github actions triggered by pull requests: https://nathandavison.com/blog/github-actions-and-the-threat-of-malicious-pull-requests

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

GitHub does not allow this secret to be accessible for github action runs from forked repos.

Yes, that's expected and also desired - normally you would not want people who clone your repository to get access to your secrets. This means that each repository / fork owner is responsible for setting up their own secrets.

there are some security concerns with allowing secrets to be added to runtime envs of github actions triggered by pull requests: https://nathandavison.com/blog/github-actions-and-the-threat-of-malicious-pull-requests

Yes, this was also mentioned in the "risks" above. A malicious PR (or even branch) might be set up to reveal the secret, and there is no way around it. This means that probably (a) untrusted contributors should not be able to run workflows (already implemented), (b) every PR should be reviewed before running workflows (common sense anyway), and (c) there should be some limitations on API keys such as quota and regular rotation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

GitHub does not allow this secret to be accessible for github action runs from forked repos.

Yes, that's expected and also desired - normally you would not want people who clone your repository to get access to your secrets. This means that each repository / fork owner is responsible for setting up their own secrets.

there are some security concerns with allowing secrets to be added to runtime envs of github actions triggered by pull requests: https://nathandavison.com/blog/github-actions-and-the-threat-of-malicious-pull-requests

Yes, this was also mentioned in the "risks" above. A malicious PR (or even branch) might be set up to reveal the secret, and there is no way around it. This means that probably (a) untrusted contributors should not be able to run workflows (already implemented), (b) every PR should be reviewed before running workflows (common sense anyway), and (c) there should be some limitations on API keys such as quota and regular rotation.

I don't see an easy way to reveal the secret that will be added specifically for ai16z workflow runs. Github CI hides it with *** when you echo it. Other than that, @jzvikart made valid concerns.

@snobbee snobbee deleted the test/integration-test-poc branch December 13, 2024 16:48
lalalune pushed a commit that referenced this pull request May 3, 2026
test: Initial release of smoke/integration tests + testing framework
FranceFlapjack pushed a commit to FranceFlapjack/eliza that referenced this pull request May 16, 2026
test: Initial release of smoke/integration tests + testing framework
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.

5 participants