-
Notifications
You must be signed in to change notification settings - Fork 33
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
175 descriptor set optimizations batch inserts #209
175 descriptor set optimizations batch inserts #209
Conversation
…ds refinement and verification testing
Target CPP Coverage: 64.3906% Target Python Coverage: 98.02% |
@rolandoquesada Question for you: I'm assuming our "end to end" tests from python do not really exercise coverage outside of the client because they are running against the server? |
Target CPP Coverage: 64.3906% Target Python Coverage: 98.02% |
…els as part of metadata
Target CPP Coverage: 64.3906% Target Python Coverage: 98.02% |
Target CPP Coverage: 64.3906% Target Python Coverage: 98.02% |
Alright folks, I think this is ready for a merge, however we have some nits around code coverage. |
@araghuna1 @s-gobriel any final updates/review comments? |
Very good work @ifadams on the batch inserts. I wonder if you think it is valuable to add a couple of c++ unit tests (in addition to the python tests you included for end-to-end to testing). These unittests can target the functionality to add in batches using the different engines and retrieve and check that the values are correct. but otherwise, I think this is good. |
Hey @s-gobriel, if you have concern about this, we can add in additional tests now, or if we want to get it merged immediately we should create an issue to immediately develop these tests as a task next quarter. I might suggest the latter, but I'm okay with either decision. |
I understand. Let's add these tests in the next quarter, especially that the python test is covering the functionality |
Target CPP Coverage: 64.3906% Target Python Coverage: 98.02% |
Initial development done, still some refinement and test updates to be done, but ready for initial review.