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

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

Merged
merged 14 commits into from
Dec 12, 2024

Conversation

jzvikart
Copy link
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
@monilpat monilpat 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
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 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
@monilpat monilpat 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
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
Contributor Author

Changed target branch to develop due to policy change.

Copy link
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 🚀

@monilpat monilpat merged commit 3faf92a into elizaOS:develop Dec 12, 2024
6 of 8 checks passed

- name: Run integration tests
env:
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
Copy link
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
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
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
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