-
Notifications
You must be signed in to change notification settings - Fork 2
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
ci: collect and upload test coverage to coddecov vec-337 #50
Conversation
AVS Python Release 1.0.2
AVS Python Release 2.0.0
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
@DomPeliniAerospike this is ready for review, please give it a look when you can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does code coverage need each test category to complete? It shouldn't cause a problem, but it might be nice to generate the report even if a test category fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you verify that code coverage will be displayed? The message above implies that code coverage will be available upon merge when a PR to the Dev branch is made. It might be good to verify that this is working as expected once this is merged.
|
Number 1 is definitely not an issue, adding coverage reports to all tests just makes all the actions more verbose. This is also redundant, since all coverage reports (besides extensive_vector_search tests) will generate the same report. Should be fine for now, but it might be worth removing the redundancies in future pipeline work. For number 2, I'm not sure how to access the code coverage website for this repository, but I can wait until actions provides a link in a future PR and take your word for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
|
This is a good point. Currently, only async is tested. When I was setting up actions, I changed them to only test aio rather than both to save time. I must've forgot to change it back. In reality, we probably don't need to run sync and async for all tests since they use the same The coverage difference between TLS configurations is slight. MTLS and TLS should have the same coverage, but yes, non-TLS setups will have slightly different coverage the TLS setups. There might be a better way than running all tests for all setups. Maybe we just need a smaller suite which just connects the client with different setups and ensure database operations can be done. If we did this, we could avoid running the async and sync suites in entirety for every test, since difference in client setups are accounted for elsewhere. We would need to do a bit of an overhaul actions overhaul, but it we could address this when we begin to finalize the CI/CD pipeline to reduce complexity. TLDR: Lets do full async and sync suites for each setup, but the test verbosity and complexity could be reduced by making a client connection suite of some sort which will accomplish the same coverage. |
The failing test seems to be a timing issue. It is passing locally. I'm going to merge this and look for a way to address the timing issue and reduce test run times in another PR. |
* feat: vec-327 add field_exclusions optional parameter to client.get and client.vector_search() to filer out fields returned by the AVS server * feat: vec-327 deprecate field_names .get() and .vector_search() argument. Add include_fields and exclude_fields arguments to those functions. fix: issue where an empty list used as field_names argument would still include all result fields. * feat: Add vector_search_by_key method to sync and async clients vec-330 (#53) * test: add tests with empty include_fields argument * chore: check that __eq__ is being used with the same class in Key type * remove extra type hints * review changes pt1 * review changes pt2, only assign projection filter fields once * docs: document Key argument, set, as string type * ci: collect and upload test coverage to coddecov vec-337 (#50) * ci: Separate extensive vector search tests and run them on push to dev and main vec-373 (#52) * ci: split extensive vector search tests into another file * ci: trigger extensive vector search tests on push to dev and main only * ci: separate is_indexed and extensive vector search tests --------- Co-authored-by: Dominic Pelini <[email protected]> Co-authored-by: Dominic Pelini <[email protected]>
No description provided.