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

Test suite refactor #60

Merged
merged 22 commits into from
Aug 7, 2021
Merged

Test suite refactor #60

merged 22 commits into from
Aug 7, 2021

Conversation

samsamai
Copy link
Contributor

@samsamai samsamai commented Jul 31, 2021

Refactor the test suite and make it run for sqlite, mysql and postgres.

@samsamai
Copy link
Contributor Author

samsamai commented Aug 1, 2021

@tyt2y3 @billy1624
I'm working on getting the integration tests to pass on postgres and I've run into a couple strange issues so far.
The latest can be seen the WIP strange fail with postgres driver commit on this branch, in the file tests/crud/create_order.rs:

.
.
.
    // This fails with "error returned from database: incorrect binary data format in bind parameter 1"
    // generated query is 'SELECT "order"."id", "order"."total", "order"."bakery_id", "order"."customer_id", "order"."placed_at" FROM "order" WHERE "order"."id" = 2 LIMIT 1'
    // let order: Option<order::Model> = Order::find_by_id(order_insert_res.last_insert_id)
    //     .one(db)
    //     .await
    //     .expect("could not find order");

    // this is ok
    let orders: Vec<order::Model> = Order::find_by_id(order_insert_res.last_insert_id)
        .all(db)
        .await
        .unwrap();
    println!("order: {:#?}", orders);
    let order: order::Model = orders[0].clone();
    println!("order: {:#?}", order);
.
.
.

Was wondering if you guys knew why this is happening?

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 1, 2021

What is the generated query of the all() call?
The problem is last_insert_id is i64, while the primary key is i32, so Postgres complains type error.
We should cast last_insert_id back to i32.
But definitely something is wrong if all works but one does not.

@samsamai
Copy link
Contributor Author

samsamai commented Aug 2, 2021

Yes I ran into the problem with last_insert_id being u64 and having to cast it as i32, I would have thought that would be type checked?

But in order to get the tests to pass I also had to do

let customer: Option<customer::Model> = Customer::find_by_id(order_model.customer_id) -> let customer: Option<customer::Model> = Customer::find_by_id(order_model.customer_id as u64)

(Notice the cast as u64). I'm confused, does find_by_id need `u64?

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 2, 2021

I originally believe that the binded value should match the column's native type.
casting to i32 does not work?

@samsamai
Copy link
Contributor Author

samsamai commented Aug 2, 2021

So moving on to some other tests now, I get an issue with count:

SELECT COUNT(*) AS num_items FROM (SELECT "customer"."id", "customer"."name", "customer"."notes" FROM "customer") AS "sub_query"
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Query("error occurred while decoding column \"num_items\": mismatched types; Rust type `i32` (as SQL type `INT4`) is not compatible with SQL type `INT8`")', tests/crud/updates.rs:99:68
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

which happens at this line of code:

    let init_n_customers: usize = Customer::find().count(db).await.unwrap();

@samsamai
Copy link
Contributor Author

samsamai commented Aug 2, 2021

I originally believe that the binded value should match the column's native type.
casting to i32 does not work?

This doesn't work:

    let bakery: Option<bakery::Model> = Bakery::find_by_id(order_model.bakery_id as i32)

This works:

    let bakery: Option<bakery::Model> = Bakery::find_by_id(order_model.bakery_id as i64)

Pretty strange because as find_by_id is used in order places but this problem doesn't happen. ??!?

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 2, 2021

okay so it's same old 32 vs 64 bit problem 🤯
image

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 2, 2021

Apparently we need to change every i32 to i64 and hope MySQL would not complain.
Otherwise we have to conditionally handle whether we'd use i32 or i64 based on the database connection.

@samsamai
Copy link
Contributor Author

samsamai commented Aug 2, 2021

okay so it's same old 32 vs 64 bit problem 🤯

Yes this works:
image

@samsamai
Copy link
Contributor Author

samsamai commented Aug 3, 2021

okay so it's same old 32 vs 64 bit problem 🤯

Yes this works:
image

That fixed the integration test but now the unit tests fail:

image

@samsamai samsamai linked an issue Aug 4, 2021 that may be closed by this pull request
6 tasks
samsamai and others added 2 commits August 5, 2021 21:49
@billy1624
Copy link
Member

Hey @samsamai, I just set the ENV for github actions. And it seems to be running. You can see the CI result here.

Btw... github actions is having some issue now. So, it might take a while to finish all tests lolll.
https://www.githubstatus.com/incidents/7p1nnvkgh96y

@billy1624
Copy link
Member

billy1624 commented Aug 5, 2021

GitHub Actions Error Summary

  • SQLite

    thread 'right_join' panicked at 'called Result::unwrap() on an Err value: Query("error returned from database: RIGHT and FULL OUTER JOINs are not currently supported")', tests/relational_tests.rs:155:10

  • Postgres

    thread 'main' panicked at 'called Result::unwrap() on an Err value: Query("error occurred while decoding column "num_items": mismatched types; Rust type i32 (as SQL type INT4) is not compatible with SQL type INT8")', tests/crud/updates.rs:99:61

All other tests passed

@billy1624
Copy link
Member

billy1624 commented Aug 6, 2021

I am trying to fix the Postgres issues with i32 & i64 now.

@billy1624
Copy link
Member

Hi @samsamai, please review the changes I pushed. Many thanks!!

@samsamai
Copy link
Contributor Author

samsamai commented Aug 6, 2021

Hi @billy1624, thanks for fixing that issue with i32, i64 it was driving me bunkers! Still not sure how the feature flag I had in there wasn't doing the same thing but I need to have a closer look at your code. Cheers

@billy1624
Copy link
Member

Hey @tyt2y3, take a look at this commit, 1eb4900, I made the changes as I encounter some issue with SQLite.

For some reason when when I try_get() for an Option. It's expected to get None if such resulting column is NULL. However, I got Some(T::default()) for some unknown reason.

e.g. https://github.com/SeaQL/sea-orm/runs/3260434167?check_suite_focus=true#step:6:135

@billy1624
Copy link
Member

Hi @billy1624, thanks for fixing that issue with i32, i64 it was driving me bunkers! Still not sure how the feature flag I had in there wasn't doing the same thing but I need to have a closer look at your code. Cheers

Please do check if I keep the your original logic! :)

@samsamai samsamai requested review from tyt2y3 August 7, 2021 10:10
@samsamai
Copy link
Contributor Author

samsamai commented Aug 7, 2021

I think we're ready to go with this.

@tyt2y3 tyt2y3 merged commit 1c73ab0 into master Aug 7, 2021
@tyt2y3 tyt2y3 deleted the ss/test_suite_refactor branch August 8, 2021 08:51
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.

Test Suite
3 participants