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

Add unit test for pagination #20

Merged
merged 11 commits into from
Jun 18, 2021
Merged

Add unit test for pagination #20

merged 11 commits into from
Jun 18, 2021

Conversation

billy1624
Copy link
Member

#5

@billy1624
Copy link
Member Author

Why cant I mark the PR as draft?

@billy1624 billy1624 self-assigned this Jun 14, 2021
@billy1624 billy1624 linked an issue Jun 14, 2021 that may be closed by this pull request
@tyt2y3
Copy link
Member

tyt2y3 commented Jun 14, 2021

Perhaps you can only create a draft when it is being opened.

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 14, 2021

Shouldn't we include an empty page3?

@billy1624
Copy link
Member Author

Ah... this will solve the issue

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 14, 2021

Yes, we should be able to impl From<M: ModelTrait> for MockRow
And don't forget to assert also the SQL query by into_transaction_log

@billy1624
Copy link
Member Author

And don't forget to assert also the SQL query by into_transaction_log

Seems that I cannot get the transaction log from MockDatabase once it turns into Database.

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 14, 2021

Well, that's bad. Because it is hidden behind a dyn trait.
We can only add the into_transaction_log method to the trait interface.
And then we can take that back out via the connection.

@billy1624
Copy link
Member Author

Well, that's bad. Because it is hidden behind a dyn trait.

Yes :(

@billy1624
Copy link
Member Author

billy1624 commented Jun 14, 2021

How about adding a feature gated into_transaction_log() on Database trait

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 14, 2021

Well, that's an official API, which would be another story.

@billy1624
Copy link
Member Author

What's your plan on this?

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 14, 2021

add the into_transaction_log method to the MockDatabaseTrait trait

@billy1624
Copy link
Member Author

Then we have both DatabaseTrait and MockDatabaseTrait implementing some CommonDatabaseTrait?

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 15, 2021

It can be accessed via MockDatabaseConnection

@billy1624
Copy link
Member Author

Hmmmm.... okay

match db.get_connection() {
    crate::DatabaseConnection::MockDatabaseConnection(mock_conn) => {

    },
    _ => unreachable!(),
};

@billy1624
Copy link
Member Author

I guess this is intensional. So that once we got the test cases setup we can run it directly on Postgres.

DatabaseConnection::MockDatabaseConnection(_) => QueryBuilderBackend::Postgres,

@billy1624
Copy link
Member Author

See latest commit for transaction log checking

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 15, 2021

Intentional. Just wanna try out different syntax to increase test coverage.

@billy1624
Copy link
Member Author

Actually the test case can be run against any live & mock connection loll

#[async_std::test]
async fn into_stream() -> Result<(), QueryErr> {
let (db, pages) = setup();
let mut fruit_stream = fruit::Entity::find().paginate(&db, 2).into_stream();
assert_eq!(fruit_stream.try_next().await?, Some(pages[0].clone()));
assert_eq!(fruit_stream.try_next().await?, Some(pages[1].clone()));
assert_eq!(fruit_stream.try_next().await?, None);
drop(fruit_stream);
let select = SelectStatement::new()
.exprs(vec![
Expr::tbl(fruit::Entity, fruit::Column::Id),
Expr::tbl(fruit::Entity, fruit::Column::Name),
Expr::tbl(fruit::Entity, fruit::Column::CakeId),
])
.from(fruit::Entity)
.to_owned();
let transaction_log = vec![
select.clone().offset(0).limit(2).to_owned(),
select.clone().offset(2).limit(2).to_owned(),
select.clone().offset(4).limit(2).to_owned(),
];
match_transaction_log(db, transaction_log);
Ok(())
}

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 16, 2021

How to refactor and improve the ergonomic for using the Mock API?

@billy1624
Copy link
Member Author

What u mean?

For example...

  • auto generate impl From for Model
  • IntoMockRow trait

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 16, 2021

Yup, make the interface smooth

@billy1624
Copy link
Member Author

Ok, will think about it

@billy1624
Copy link
Member Author

Btw... we can explore the possibility to run our test cases against any real & mock connection

E.g. type def a connection for all test cases to use

@billy1624
Copy link
Member Author

I have created a match_transaction_log macro to support transaction log checking against select, update, delete and insert statement.

sea-orm/src/util.rs

Lines 27 to 38 in 5ef7c2d

#[macro_export]
macro_rules! match_transaction_log {
($logs:expr, $stmts:expr, $query_builder:expr, $build_method:ident) => {
for stmt in $stmts.iter() {
assert!(!$logs.is_empty());
let log = $logs.first().unwrap();
let statement = $query_builder.$build_method(stmt);
assert_eq!(log.to_string(), statement.to_string());
$logs = $logs.drain(1..).collect();
}
};
}

Use it as follows

let stmts = vec![
select.clone().offset(0).limit(2).to_owned(),
select.clone().offset(2).limit(2).to_owned(),
select.clone().offset(4).limit(2).to_owned(),
];
let query_builder = db.get_query_builder_backend();
let mut logs = get_mock_transaction_log(db);
match_transaction_log!(logs, stmts[0..1], query_builder, build_select_statement);
match_transaction_log!(logs, stmts[1..], query_builder, build_select_statement);

@billy1624
Copy link
Member Author

What u mean?

For example...

  • auto generate impl From for Model
  • IntoMockRow trait

Both added

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 17, 2021

Macro is not approved

@billy1624
Copy link
Member Author

billy1624 commented Jun 17, 2021

Macro is not approved

I guess u mean the derive macro

Instead of using derive macro, we can...

https://github.com/SeaQL/sea-orm/blob/aa412016777e171d48cd02a93e23cb3f3bf6b8a9/src/tests_cfg/util.rs

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 17, 2021

I mean both derive and match_transaction_log
I don't see a necessity for those.

@billy1624
Copy link
Member Author

I mean both derive and match_transaction_log
I don't see a necessity for those.

Just to avoid boilerplate

@billy1624
Copy link
Member Author

Will expand the macro then loll

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 17, 2021

There must exist better ways to improve the ergonomic without using any macro

@billy1624
Copy link
Member Author

billy1624 commented Jun 17, 2021

Can we add a fn in MockDatabase? Take in any Statement and match it against first row of transaction_log

i.e. calling assert_eq!() in MockDatabase API

Is this good?

@billy1624
Copy link
Member Author

Can we add a fn in MockDatabase? Take in any Statement and match it against first row of transaction_log

i.e. calling assert_eq!() in MockDatabase API

Is this good?

See latest commit

@tyt2y3 tyt2y3 merged commit 62fb43c into master Jun 18, 2021
@tyt2y3 tyt2y3 deleted the pagination-test branch August 8, 2021 08:51
SebastienGllmt pushed a commit to dcSpark/sea-orm that referenced this pull request Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pagination API
2 participants