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

Improve test performance #1953

Open
wants to merge 39 commits into
base: branch-24.10
Choose a base branch
from

Conversation

dagardner-nv
Copy link
Contributor

@dagardner-nv dagardner-nv commented Oct 17, 2024

Description

  • Improve milvus startup time bypassing milvus' own wait_for_started method which was causing an intermittent delay up to 40s
  • Consolidate tests test_kafka_source_commit and test_kafka_source_batch_pipe these two tests were 80% similar
  • Remove the num_records=100 parameter from test_kafka_source_batch_pipe
  • Reduce parameters for test test_http_server.py::test_simple_request lowering the total number of test combinations from 865 down to 145.
  • Update rag pipeline tests to share a common milvus collection, lowering test time from 45s down to 25s

Closes #1948

By Submitting this PR I confirm:

  • I am familiar with the Contributing Guidelines.
  • When the PR is ready for review, new or existing tests cover these changes.
  • When the PR is ready for review, the documentation is up to date with these changes.

dagardner-nv and others added 30 commits October 15, 2024 14:32
…t the TestDirectories class looking like a test class
consolidate the test_kafka_source_commit and test_kafka_source_batch_pipe tests as they were nearly identical
Improve test time from ~4min to 2:39
@dagardner-nv dagardner-nv added bug Something isn't working non-breaking Non-breaking change skip-ci Optionally Skip CI for this PR labels Oct 17, 2024
@dagardner-nv dagardner-nv self-assigned this Oct 17, 2024
@dagardner-nv dagardner-nv requested a review from a team as a code owner October 17, 2024 15:14
@dagardner-nv dagardner-nv marked this pull request as draft October 17, 2024 15:18

def parse_fn(resp: requests.Response) -> bool:
content = resp.content.decode('utf-8')
return 'OK' in content
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also check the status code? I would hate for it to return a 400 error with Im not OK and this test would pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't even call that method if the status code isn't 200:

            if (resp.status_code == 200):
                if parse_fn(resp):
                    return True

@dagardner-nv dagardner-nv removed the skip-ci Optionally Skip CI for this PR label Oct 17, 2024
@dagardner-nv dagardner-nv marked this pull request as ready for review October 17, 2024 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
Status: Review - Ready for Review
Development

Successfully merging this pull request may close these issues.

[BUG]: CI Test stage is hitting the one hour limit
2 participants