[REVIEW][Java] Refactor CagraBuildAndSearchIT to explicitly express different execution modes #1006
Conversation
ChrisHegarty
left a comment
ChrisHegarty
left a comment
There was a problem hiding this comment.
Thanks for checking on this, and for improving the test. LGTM
This is an important and significant point, which I want to emphasise - it's the reason why the bug reported in #764 went unnoticed. |
cjnolet
added
improvement
|
/ok to test 2f1942b |
|
/ok to test ea583c8 |
mythrocks
left a comment
mythrocks
left a comment
There was a problem hiding this comment.
I really like this refactor.
|
/merge |
github-project-automation
bot
moved this from In Progress
to Done
in Vector Search, ML, & Data Mining Release Board
|
It looks like the PR merged with the |
@mythrocks once a PR is set to review, it's ready for merge if it's approved. The only time we won't review/merge is if it has WIP In the title. |
…ifferent execution modes (rapidsai#1006) This PR refactors `CagraBuildAndSearchIT`, introducing separate test for different execution modes (same thread, different thread, concurrent). This way different modes are tested every time, without the need to rely on randomization to hit a particular test path. The execution is handled by utility functions (e.g. `runConcurrently`); right now these are private to the test suite, but if we think they could be useful across other tests I can move them to a utility class. It also fixes a issue where we did not wait and/or report failures in the concurrent case (now exceptions are propagated back to the calling JUnit thread). Authors: - Lorenzo Dematté (https://github.com/ldematte) - Corey J. Nolet (https://github.com/cjnolet) Approvers: - Chris Hegarty (https://github.com/ChrisHegarty) - MithunR (https://github.com/mythrocks) URL: rapidsai#1006
4 participants
This PR refactors
CagraBuildAndSearchIT, introducing separate test for different execution modes (same thread, different thread, concurrent).This way different modes are tested every time, without the need to rely on randomization to hit a particular test path.
The execution is handled by utility functions (e.g.
runConcurrently); right now these are private to the test suite, but if we think they could be useful across other tests I can move them to a utility class.It also fixes a issue where we did not wait and/or report failures in the concurrent case (now exceptions are propagated back to the calling JUnit thread).