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

chore(test): fix and re-activate integration and unit tests #1004

Merged

Conversation

justinthelaw
Copy link
Contributor

@justinthelaw justinthelaw commented Sep 6, 2024

Description

See PR #994 for details on testing issues, and #564 for the overall integration testing issues. If the integration tests were run prior to committing, or in the pipeline, the file_ids issue fixed by PR #994 (related to Issue #1001) would have been caught earlier.

BREAKING CHANGES

  • N/A

CHANGES

  • Fixes the Makefile targets for executing integration tests - now works on all shells and scenarios
  • Further improves README for local unit and integration test execution
  • Enables integration and unit testing in local development and inside our CI pipelines
  • Refactors the Repeater to better mock an actual in-cluster model, fixing issues with chat completion integration tests and streaming chat unit tests
  • Refactors unit tests to test the behavior of the API and SDK, rather than the Repeater implementation code
  • Bumps unstructured and nltk versions to allow for .pptx chunking, which was previously not working and failed integration tests

Related Issue

Fixes Issues #1001 #564
Related to PR #994

Checklist before merging

@justinthelaw justinthelaw linked an issue Sep 6, 2024 that may be closed by this pull request
Copy link

netlify bot commented Sep 6, 2024

Deploy Preview for leapfrogai-docs canceled.

Name Link
🔨 Latest commit 926ba60
🔍 Latest deploy log https://app.netlify.com/sites/leapfrogai-docs/deploys/66df5826f162fa00088192e4

@justinthelaw justinthelaw changed the title chore(test): fix integration testing and add empty files assistant test chore(test): fix integration test make targets, add empty files test Sep 6, 2024
@justinthelaw justinthelaw self-assigned this Sep 6, 2024
@justinthelaw justinthelaw changed the title chore(test): fix integration test make targets, add empty files test chore(test): fix integration tests and add blank file ids runs test Sep 7, 2024
@justinthelaw justinthelaw linked an issue Sep 7, 2024 that may be closed by this pull request
@justinthelaw justinthelaw added the tech-debt Not a feature, but still necessary label Sep 7, 2024
@justinthelaw justinthelaw changed the title chore(test): fix integration tests and add blank file ids runs test chore(test): fix integration tests and add empty file_ids runs test Sep 7, 2024
@justinthelaw justinthelaw changed the title chore(test): fix integration tests and add empty file_ids runs test chore(test): fix and reactivate integration tests Sep 7, 2024
@justinthelaw justinthelaw changed the title chore(test): fix and reactivate integration tests chore(test): fix and re-activate integration tests Sep 9, 2024
@justinthelaw justinthelaw marked this pull request as ready for review September 9, 2024 16:10
@justinthelaw justinthelaw requested a review from a team as a code owner September 9, 2024 16:10
@justinthelaw justinthelaw changed the title chore(test): fix and re-activate integration tests chore(test): fix and re-activate integration and unit tests Sep 9, 2024
jalling97
jalling97 previously approved these changes Sep 9, 2024
Copy link
Contributor

@jalling97 jalling97 left a comment

Choose a reason for hiding this comment

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

Having the integration tests be a part of the pipeline is fantastic!

Just pointing out though that the pytest workflow takes about 25 minutes now, which is about as long as it takes to test text-embeddings. So while we're not taking any longer to run all of the tests, we're further cementing that 25-30 minute timeframe. If we make changes in the future to futher optimize the E2E tests, addressing the integration tests will probably also be necessary.

@justinthelaw
Copy link
Contributor Author

justinthelaw commented Sep 9, 2024

Having the integration tests be a part of the pipeline is fantastic!

Just pointing out though that the pytest workflow takes about 25 minutes now, which is about as long as it takes to test text-embeddings. So while we're not taking any longer to run all of the tests, we're further cementing that 25-30 minute timeframe. If we make changes in the future to futher optimize the E2E tests, addressing the integration tests will probably also be necessary.

@jalling97 totally agree! I will make an evergreen spike to investigate speeding up all e2e tests, including integration tests. The difficulty is definitely that each backend is very time-consuming to install and build. What we can do in the spike is investigate GH caching (like I do with the pip deps) and also more usage of a no/low-weight model like the repeater or T5, GPT2 etc., in tests-only.

EDIT: here is the aforementioned issue

@justinthelaw justinthelaw merged commit d32bd72 into main Sep 9, 2024
27 checks passed
@justinthelaw justinthelaw deleted the 1001-choretest-write-backend-integration-test-for-pr-995 branch September 9, 2024 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore tech-debt Not a feature, but still necessary
Projects
None yet
3 participants