Add integration tests for query_unpaged and execute_unpaged APIs
#1228
Conversation
|
|
| async fn unpaged_error(use_prepared_statements: bool) { | ||
| let table_name = if use_prepared_statements { | ||
| "execute_unpaged" | ||
| } else { | ||
| "query_unpaged" | ||
| }; | ||
| let session: Session = create_session(table_name).await; | ||
| session.await_schema_agreement().await.unwrap(); | ||
|
|
||
| let query_result: Result<QueryResult, ExecutionError>; | ||
| if use_prepared_statements { | ||
| let prepared_statement = session | ||
| .prepare(format!("SELECT * FROM {}", table_name)) | ||
| .await | ||
| .unwrap(); | ||
| // NOTE: drop table to make the main query return error | ||
| session | ||
| .ddl(format!("DROP TABLE IF EXISTS {}", table_name)) | ||
| .await | ||
| .unwrap(); | ||
| query_result = session.execute_unpaged(&prepared_statement, &[]).await; | ||
| } else { | ||
| let select_query = Query::new(format!("SELECT * FROM fake{}", table_name)); | ||
| query_result = session.query_unpaged(select_query, &[]).await; | ||
| } | ||
| match query_result { | ||
| Ok(_) => panic!("Unexpected success"), | ||
| Err(err) => println!("Table not found as expected: {:?}", err), | ||
| } | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_query_unpaged_error() { | ||
| unpaged_error(false).await | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_execute_unpaged_error() { | ||
| unpaged_error(true).await | ||
| } |
There was a problem hiding this comment.
I'd like to not make tests unnecessarily slow, which means reducing the amount of schema changes.
Those 2 tests could be merged into one which:
- Creates KS and table
- Makes the unprepared check (because it does not remove the table)
- Makes the prepared check.
| #[tokio::test] | ||
| async fn test_query_unpaged_no_rows() { | ||
| check_query_unpaged(0, false).await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_query_unpaged_one_row() { | ||
| check_query_unpaged(1, false).await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_query_unpaged_ten_rows() { | ||
| check_query_unpaged(10, false).await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_query_unpaged_hundred_rows() { | ||
| check_query_unpaged(100, false).await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_query_unpaged_thousand_rows() { | ||
| check_query_unpaged(1000, false).await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_execute_unpaged_no_rows() { | ||
| check_query_unpaged(0, true).await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_execute_unpaged_one_row() { | ||
| check_query_unpaged(1, true).await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_execute_unpaged_ten_rows() { | ||
| check_query_unpaged(10, true).await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_execute_unpaged_hundred_rows() { | ||
| check_query_unpaged(100, true).await; | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_execute_unpaged_thousand_rows() { | ||
| check_query_unpaged(1000, true).await; | ||
| } |
There was a problem hiding this comment.
Those tests could be merged into one test (or two - one for prepared, one for unprepared), in order to re-use the created session and table.
There was a problem hiding this comment.
All the 23 tests in this module take about 5 seconds not considering the compilation time.
Current approach is the test isolation, reusing session and tables we create side-effects.
Having 5 seconds for 23 tests I don't think that the gained test run time worth the side-effects.
Then, existing 11 tests follow the same approach - isolated logic. Which, I think, is proper approach for "tests".
There was a problem hiding this comment.
5 seconds of execution time is not short.
I know that writing them as one tests is a bit more involved (e.g. you need to DELETE stuff from the table between test cases) but I think its still worth it to speed things up.
There was a problem hiding this comment.
It is not just some delete's or truncate's, it is losing test isolation.
Also, delete and truncate calls take some time too.
Adding new tests we should not bother ourselves in which order tests run and how they may fail in the middle and cause other tests to fail due to unexpected DB table state.
I still value the test isolation more than the really small possible time gain.
Also, note that 5 seconds is taken by all the tests in this module including existing ones.
There was a problem hiding this comment.
If some test case fails in the middle, then just report failure without running other test cases.
DELETE will take time, but much less time that creating keyspace, table, and session.
Test time is important. It directly affects iteration time for developers.
There was a problem hiding this comment.
Proper test isolation reduces the investigation time in case of failures.
So, looks like we need other opinions here to get a quorum.
There was a problem hiding this comment.
Investigating failing tests has never been a problem in Rust Driver. Iteration time is something that constantly affects development speed.
There was a problem hiding this comment.
I think we need to merge them, another reason is that there is chance for ddl statement to fail when ran in parralell due to Scylla bug, to make suit more stable we need reduce them as much as possible.
vponomaryov
force-pushed
the
unpaged_queries
branch
from
9ce4ea2 to
4fb2684
Compare
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.